Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: minor fixes for queries that perform locking #141964

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

opt: minor fixes for queries that perform locking #141964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5ee6e7
opt: only set volatile for scans that perform locking
DrewKimball Feb 25, 2025
6389fbd
opt: set volatility for locking operators
DrewKimball Feb 25, 2025
96ac12d
opt: remove Mutation tag from lock operators
DrewKimball Feb 25, 2025
78a4515
plpgsql: don't make cursors with locking holdable
DrewKimball Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions pkg/ccl/logictestccl/testdata/logic_test/plpgsql_cursor
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1936,8 +1936,8 @@ $$ LANGUAGE PLpgSQL;

subtest hold_setting

# Test the open_plpgsql_cursor_with_hold setting, which causes PL/pgSQL cursors
# to remain open after their transaction closes.
# Test the close_cursors_at_commit setting, which determines whether PL/pgSQL
# cursors remain open after their transaction closes.
statement ok
CREATE PROCEDURE make_curs(curs REFCURSOR) AS $$
BEGIN
Expand All @@ -1954,7 +1954,7 @@ query T
SELECT name FROM pg_cursors;
----

# The cursors should be visible within the explicit transcation.
# The cursors should be visible within the explicit transaction.
statement ok
BEGIN;
CALL make_curs('foo');
Expand Down Expand Up @@ -2059,6 +2059,44 @@ query T
SELECT name FROM pg_cursors;
----

# Test a holdable cursor opened in a DO block.
statement ok
DO $$
DECLARE
curs REFCURSOR := 'foo';
BEGIN
OPEN curs FOR SELECT * FROM xy ORDER BY x;
END
$$;

query II
FETCH FROM foo;
----
1 2

query T
SELECT name FROM pg_cursors;
----
foo

statement ok
CLOSE foo;

# If the query contains locking, the cursor is not holdable, and should be
# closed when the transaction ends.
statement ok
DO $$
DECLARE
curs REFCURSOR := 'foo';
BEGIN
OPEN curs FOR SELECT * FROM xy FOR UPDATE;
END
$$;

query T
SELECT name FROM pg_cursors;
----

statement ok
RESET close_cursors_at_commit;

Expand Down
17 changes: 17 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/read_committed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,8 @@ user testuser
statement ok
COMMIT

subtest end

subtest regression_130661

statement ok
Expand All @@ -577,3 +579,18 @@ statement ok
COMMIT

subtest end

subtest regression_141961

statement ok
BEGIN ISOLATION LEVEL READ COMMITTED;

query I
SELECT * FROM (WITH foo AS (SELECT * FROM t FOR UPDATE LIMIT 1) SELECT 1);
----
1

statement ok
ROLLBACK;

subtest end
1 change: 1 addition & 0 deletions pkg/sql/opt/exec/execbuilder/testdata/select_for_update_read_committed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ vectorized: true
│ │ estimated row count: 0 (missing stats)
│ │ table: supermarket
│ │ set: aisle
│ │ auto commit
│ │
│ └── • render
│ │ columns: (person, aisle, starts_with, ends_with, aisle_new)
Expand Down
35 changes: 34 additions & 1 deletion pkg/sql/opt/memo/logical_props_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func (b *logicalPropsBuilder) buildScanProps(scan *ScanExpr, rel *props.Relation
// Side Effects
// ------------
// A Locking option is a side-effect (we don't want to elide this scan).
if !scan.Locking.IsNoOp() {
if scan.Locking.IsLocking() {
rel.VolatilitySet.AddVolatile()
}

Expand Down Expand Up @@ -501,6 +501,13 @@ func (b *logicalPropsBuilder) buildIndexJoinProps(indexJoin *IndexJoinExpr, rel
inputProps := indexJoin.Input.Relational()
md := b.mem.Metadata()

// Side Effects
// ------------
// A Locking option is a side effect.
if indexJoin.Locking.IsLocking() {
rel.VolatilitySet.AddVolatile()
}

// Output Columns
// --------------
rel.OutputCols = indexJoin.Cols
Expand Down Expand Up @@ -541,6 +548,13 @@ func (b *logicalPropsBuilder) buildIndexJoinProps(indexJoin *IndexJoinExpr, rel
}

func (b *logicalPropsBuilder) buildLookupJoinProps(join *LookupJoinExpr, rel *props.Relational) {
// Side Effects
// ------------
// A Locking option is a side effect.
if join.Locking.IsLocking() {
rel.VolatilitySet.AddVolatile()
}

b.buildJoinProps(join, rel)
if join.Locking.WaitPolicy == tree.LockWaitSkipLocked {
// SKIP LOCKED can act like a filter. The minimum cardinality of a scan
Expand All @@ -553,10 +567,24 @@ func (b *logicalPropsBuilder) buildLookupJoinProps(join *LookupJoinExpr, rel *pr
func (b *logicalPropsBuilder) buildInvertedJoinProps(
join *InvertedJoinExpr, rel *props.Relational,
) {
// Side Effects
// ------------
// A Locking option is a side effect.
if join.Locking.IsLocking() {
rel.VolatilitySet.AddVolatile()
}

b.buildJoinProps(join, rel)
}

func (b *logicalPropsBuilder) buildZigzagJoinProps(join *ZigzagJoinExpr, rel *props.Relational) {
// Side Effects
// ------------
// A Locking option is a side effect.
if join.LeftLocking.IsLocking() || join.RightLocking.IsLocking() {
rel.VolatilitySet.AddVolatile()
}

b.buildJoinProps(join, rel)
}

Expand Down Expand Up @@ -1583,6 +1611,11 @@ func (b *logicalPropsBuilder) buildMutationProps(mutation RelExpr, rel *props.Re
func (b *logicalPropsBuilder) buildLockProps(lock *LockExpr, rel *props.Relational) {
BuildSharedProps(lock, &rel.Shared, b.evalCtx)

// Side Effects
// ------------
// Locking is a side effect.
rel.VolatilitySet.AddVolatile()

private := lock.Private().(*LockPrivate)
inputProps := lock.Child(0).(RelExpr).Relational()

Expand Down
33 changes: 12 additions & 21 deletions pkg/sql/opt/norm/testdata/rules/limit
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1604,7 +1604,7 @@ lock abcde
├── lock columns: (8-12)
├── locking: for-update
├── cardinality: [0 - 1]
├── volatile, mutations
├── volatile
├── key: ()
├── fd: ()-->(1-5)
└── index-join abcde
Expand All @@ -1626,7 +1626,7 @@ lock abcde
├── key columns: a:1
├── lock columns: (8-12)
├── locking: for-update
├── volatile, mutations
├── volatile
├── key: (1)
├── fd: ()-->(4), (1)-->(2,3,5)
└── index-join abcde
Expand All @@ -1651,29 +1651,27 @@ SELECT * FROM abcde WHERE d IS NULL LIMIT 1 FOR UPDATE SKIP LOCKED
limit
├── columns: a:1!null b:2 c:3!null d:4 e:5
├── cardinality: [0 - 1]
├── volatile, mutations
├── volatile
├── key: ()
├── fd: ()-->(1-5)
├── lock abcde
│ ├── columns: a:1!null b:2 c:3!null d:4 e:5
│ ├── key columns: a:1
│ ├── lock columns: (8-12)
│ ├── locking: for-update,skip-locked
│ ├── volatile, mutations
│ ├── volatile
│ ├── key: (1)
│ ├── fd: ()-->(4), (1)-->(2,3,5)
│ ├── limit hint: 1.00
│ └── index-join abcde
│ ├── columns: a:1!null b:2 c:3!null d:4 e:5
│ ├── locking: none,skip-locked
│ ├── volatile
│ ├── key: (1)
│ ├── fd: ()-->(4), (1)-->(2,3,5)
│ ├── limit hint: 1.00
│ └── scan abcde@abcde_c_idx,partial
│ ├── columns: a:1!null c:3!null
│ ├── locking: none,skip-locked
│ ├── volatile
│ ├── key: (1)
│ ├── fd: (1)-->(3)
│ └── limit hint: 1.00
Expand All @@ -1684,28 +1682,25 @@ SELECT a FROM abcde WHERE d IS NULL OFFSET 1 FOR UPDATE SKIP LOCKED
----
offset
├── columns: a:1!null
├── volatile, mutations
├── volatile
├── key: (1)
├── lock abcde
│ ├── columns: a:1!null
│ ├── key columns: a:1
│ ├── lock columns: (8-12)
│ ├── locking: for-update,skip-locked
│ ├── volatile, mutations
│ ├── volatile
│ ├── key: (1)
│ └── project
│ ├── columns: a:1!null
│ ├── volatile
│ ├── key: (1)
│ └── project
│ ├── columns: d:4 a:1!null
│ ├── volatile
│ ├── key: (1)
│ ├── fd: ()-->(4)
│ ├── scan abcde@abcde_c_idx,partial
│ │ ├── columns: a:1!null
│ │ ├── locking: none,skip-locked
│ │ ├── volatile
│ │ └── key: (1)
│ └── projections
│ └── CAST(NULL AS TIMESTAMP) [as=d:4]
Expand All @@ -1722,7 +1717,7 @@ lock abcde
├── lock columns: (8-12)
├── locking: for-update,durability-guaranteed
├── cardinality: [0 - 1]
├── volatile, mutations
├── volatile
├── key: ()
├── fd: ()-->(1-5)
└── index-join abcde
Expand All @@ -1744,7 +1739,7 @@ lock abcde
├── key columns: a:1
├── lock columns: (8-12)
├── locking: for-update,durability-guaranteed
├── volatile, mutations
├── volatile
├── key: (1)
├── fd: ()-->(4), (1)-->(2,3,5)
└── index-join abcde
Expand All @@ -1769,29 +1764,27 @@ SELECT * FROM abcde WHERE d IS NULL LIMIT 1 FOR UPDATE SKIP LOCKED
limit
├── columns: a:1!null b:2 c:3!null d:4 e:5
├── cardinality: [0 - 1]
├── volatile, mutations
├── volatile
├── key: ()
├── fd: ()-->(1-5)
├── lock abcde
│ ├── columns: a:1!null b:2 c:3!null d:4 e:5
│ ├── key columns: a:1
│ ├── lock columns: (8-12)
│ ├── locking: for-update,skip-locked,durability-guaranteed
│ ├── volatile, mutations
│ ├── volatile
│ ├── key: (1)
│ ├── fd: ()-->(4), (1)-->(2,3,5)
│ ├── limit hint: 1.00
│ └── index-join abcde
│ ├── columns: a:1!null b:2 c:3!null d:4 e:5
│ ├── locking: none,skip-locked
│ ├── volatile
│ ├── key: (1)
│ ├── fd: ()-->(4), (1)-->(2,3,5)
│ ├── limit hint: 1.00
│ └── scan abcde@abcde_c_idx,partial
│ ├── columns: a:1!null c:3!null
│ ├── locking: none,skip-locked
│ ├── volatile
│ ├── key: (1)
│ ├── fd: (1)-->(3)
│ └── limit hint: 1.00
Expand All @@ -1802,27 +1795,25 @@ SELECT * FROM abcde WHERE d IS NULL OFFSET 1 FOR UPDATE SKIP LOCKED
----
offset
├── columns: a:1!null b:2 c:3!null d:4 e:5
├── volatile, mutations
├── volatile
├── key: (1)
├── fd: ()-->(4), (1)-->(2,3,5)
├── lock abcde
│ ├── columns: a:1!null b:2 c:3!null d:4 e:5
│ ├── key columns: a:1
│ ├── lock columns: (8-12)
│ ├── locking: for-update,skip-locked,durability-guaranteed
│ ├── volatile, mutations
│ ├── volatile
│ ├── key: (1)
│ ├── fd: ()-->(4), (1)-->(2,3,5)
│ └── index-join abcde
│ ├── columns: a:1!null b:2 c:3!null d:4 e:5
│ ├── locking: none,skip-locked
│ ├── volatile
│ ├── key: (1)
│ ├── fd: ()-->(4), (1)-->(2,3,5)
│ └── scan abcde@abcde_c_idx,partial
│ ├── columns: a:1!null c:3!null
│ ├── locking: none,skip-locked
│ ├── volatile
│ ├── key: (1)
│ └── fd: (1)-->(3)
└── 1
2 changes: 1 addition & 1 deletion pkg/sql/opt/norm/testdata/rules/mutation
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ lock t
├── key columns: k:1
├── lock columns: (12-20)
├── locking: for-share
├── volatile, mutations
├── volatile
├── key: (1)
├── fd: ()-->(2)
└── select
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/ops/mutation.opt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ define UniqueChecksItemPrivate {
# The Lock operator does not necessarily have to be at the top of a plan. It
# could appear within a subtree of the plan, and in this case acts as an
# optimization barrier to ensure the correct rows are locked.
[Relational, Mutation]
[Relational]
define Lock {
Input RelExpr
_ LockPrivate
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/xform/testdata/external/tpcc-read-committed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ lock stock
├── lock columns: (20-36)
├── locking: for-update,durability-guaranteed
├── cardinality: [0 - 5]
├── volatile, mutations
├── volatile
├── key: (1)
├── fd: ()-->(2), (1)-->(3,8,14-17)
├── ordering: +1 opt(2) [actual: +1]
Expand Down Expand Up @@ -834,7 +834,7 @@ lock new_order
├── lock columns: (6-8)
├── locking: for-update,durability-guaranteed
├── cardinality: [0 - 1]
├── volatile, mutations
├── volatile
├── key: ()
├── fd: ()-->(1-3)
└── scan new_order
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/xform/testdata/rules/limit
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ lock partial_index_const
├── lock columns: (7-10)
├── locking: for-update
├── cardinality: [0 - 5]
├── volatile, mutations
├── volatile
├── key: (4)
├── fd: ()-->(2), (4)-->(1,3)
└── project
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/xform/testdata/rules/select
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10434,6 +10434,7 @@ inner-join (lookup b)
│ ├── right fixed columns: [9] = ['\x376300012a0400']
│ ├── left locking: for-update
│ ├── right locking: for-update
│ ├── volatile
│ └── filters (true)
└── filters (true)

Expand Down
Loading