From 6c0823f8e67816bbcfdc1cb156264d386a5d8e22 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Thu, 19 Oct 2023 15:03:35 -0600 Subject: [PATCH 1/4] sql: disallow refcursor comparisons during type-checking This patch adds back the comparison functions for the `REFCURSOR` data type, since there are various points within the codebase where we rely on their existence. Users are still not allowed to use the comparison functions, but now this is checked during type-checking of the AST, rather than at execution-time. This should avoid internal errors from places in planning and execution that expect comparison overloads to exist, but also maintains parity with postgres. Fixes #112365 Fixes #112642 Fixes #112362 Fixes #112368 Release note: None --- docs/generated/sql/operators.md | 6 +- .../logictest/testdata/logic_test/refcursor | 66 +++++++++++-------- pkg/sql/sem/tree/eval.go | 9 +-- pkg/sql/sem/tree/type_check.go | 63 ++++++++++++++++++ 4 files changed, 111 insertions(+), 33 deletions(-) diff --git a/docs/generated/sql/operators.md b/docs/generated/sql/operators.md index 9f01f3508286..b7c2af9b6893 100644 --- a/docs/generated/sql/operators.md +++ b/docs/generated/sql/operators.md @@ -193,6 +193,7 @@ oid < intbool oid < oidbool pg_lsn < pg_lsnbool +refcursor < refcursorbool string < stringbool string[] < string[]bool time < timebool @@ -257,6 +258,7 @@ oid <= intbool oid <= oidbool pg_lsn <= pg_lsnbool +refcursor <= refcursorbool string <= stringbool string[] <= string[]bool time <= timebool @@ -320,6 +322,7 @@ oid = intbool oid = oidbool pg_lsn = pg_lsnbool +refcursor = refcursorbool string = stringbool string[] = string[]bool time = timebool @@ -400,6 +403,7 @@ jsonb IN tuplebool oid IN tuplebool pg_lsn IN tuplebool +refcursor IN tuplebool string IN tuplebool time IN tuplebool timestamp IN tuplebool @@ -447,7 +451,7 @@ oid IS NOT DISTINCT FROM intbool oid IS NOT DISTINCT FROM oidbool pg_lsn IS NOT DISTINCT FROM pg_lsnbool -refcursor IS NOT DISTINCT FROM unknownbool +refcursor IS NOT DISTINCT FROM refcursorbool string IS NOT DISTINCT FROM stringbool string[] IS NOT DISTINCT FROM string[]bool time IS NOT DISTINCT FROM timebool diff --git a/pkg/sql/logictest/testdata/logic_test/refcursor b/pkg/sql/logictest/testdata/logic_test/refcursor index 3e1431529696..14081d8ed30b 100644 --- a/pkg/sql/logictest/testdata/logic_test/refcursor +++ b/pkg/sql/logictest/testdata/logic_test/refcursor @@ -394,64 +394,79 @@ NULL NULL NULL foo NULL NULL NULL NULL NULL NULL NULL NULL # REFCURSOR doesn't support any comparisons (with an exception mentioned below). subtest comparisons -# TODO(drewk): The error code should be 42883. -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR = 'foo'::REFCURSOR; +# TODO(drewk): The error code should be 42883. statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR = 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR = NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR < 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR < 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR < NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR > 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR > 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR > NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR <= 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR <= 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR <= NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::TEXT; +statement error pgcode 42883 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR BETWEEN 'foo'::REFCURSOR AND 'bar'::REFCURSOR; + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR BETWEEN 'foo'::TEXT AND 'bar'::TEXT; + +statement error pgcode 42883 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR BETWEEN NULL AND NULL; + +statement error pgcode 42883 pq: unsupported comparison operator +SELECT NULLIF('foo'::REFCURSOR, 'bar'::REFCURSOR); + +statement error pgcode 42883 pq: unsupported comparison operator +SELECT NULLIF('foo'::REFCURSOR, NULL); + # Regression test for #112010 - REFCURSOR should allow IS NULL and # IS NOT NULL comparisons. subtest is_null @@ -514,17 +529,17 @@ true true # Comparison with column is not allowed. -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM l FROM implicit_types; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM l FROM implicit_types; # Comparison with typed NULL is not allowed. -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM (NULL::REFCURSOR); -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (NULL::REFCURSOR); # Comparison with CASE expression is not allowed. @@ -534,23 +549,18 @@ SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); -# The CASE operator is folded into an untyped NULL, so it's allowed. -# TODO(drewk): Postgres doesn't allow this case. -query B +# The CASE operator is folded into an untyped NULL. +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); ----- -true -query B +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); ----- -false -# The CASE operator is folded into a typed NULL, so it's not allowed. -statement error pgcode 22023 pq: unsupported comparison operator +# The CASE operator is folded into a typed NULL. +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); # Regression test for #112099 - REFCURSOR is not valid as an index column. diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index e9784271e94f..5146bf2d4d5e 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -1542,6 +1542,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeEqFn(types.Jsonb, types.Jsonb, volatility.Immutable), makeEqFn(types.Oid, types.Oid, volatility.Leakproof), makeEqFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeEqFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeEqFn(types.String, types.String, volatility.Leakproof), makeEqFn(types.Time, types.Time, volatility.Leakproof), makeEqFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1601,6 +1602,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeLtFn(types.Interval, types.Interval, volatility.Leakproof), makeLtFn(types.Oid, types.Oid, volatility.Leakproof), makeLtFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeLtFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeLtFn(types.String, types.String, volatility.Leakproof), makeLtFn(types.Time, types.Time, volatility.Leakproof), makeLtFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1659,6 +1661,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeLeFn(types.Interval, types.Interval, volatility.Leakproof), makeLeFn(types.Oid, types.Oid, volatility.Leakproof), makeLeFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeLeFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeLeFn(types.String, types.String, volatility.Leakproof), makeLeFn(types.Time, types.Time, volatility.Leakproof), makeLeFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1738,6 +1741,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeIsFn(types.Jsonb, types.Jsonb, volatility.Immutable), makeIsFn(types.Oid, types.Oid, volatility.Leakproof), makeIsFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeIsFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeIsFn(types.String, types.String, volatility.Leakproof), makeIsFn(types.Time, types.Time, volatility.Leakproof), makeIsFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1775,10 +1779,6 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeIsFn(types.Void, types.Unknown, volatility.Leakproof), makeIsFn(types.Unknown, types.Void, volatility.Leakproof), - // REFCURSOR cannot be compared with itself, but as a special case, it can - // be compared with NULL using IS NOT DISTINCT FROM. - makeIsFn(types.RefCursor, types.Unknown, volatility.Leakproof), - // Tuple comparison. { LeftType: types.AnyTuple, @@ -1809,6 +1809,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeEvalTupleIn(types.Jsonb, volatility.Leakproof), makeEvalTupleIn(types.Oid, volatility.Leakproof), makeEvalTupleIn(types.PGLSN, volatility.Leakproof), + makeEvalTupleIn(types.RefCursor, volatility.Leakproof), makeEvalTupleIn(types.String, volatility.Leakproof), makeEvalTupleIn(types.Time, volatility.Leakproof), makeEvalTupleIn(types.TimeTZ, volatility.Leakproof), diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 5def848679c2..cbea07a08fa5 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -939,6 +939,11 @@ func (expr *ComparisonExpr) TypeCheck( expr.Left, expr.Right, ) + if err == nil { + err = checkRefCursorComparison( + expr.SubOperator.Symbol, leftTyped.ResolvedType(), rightTyped.ResolvedType(), + ) + } } else { leftTyped, rightTyped, cmpOp, alwaysNull, err = typeCheckComparisonOp( ctx, @@ -947,6 +952,11 @@ func (expr *ComparisonExpr) TypeCheck( expr.Left, expr.Right, ) + if err == nil { + err = checkRefCursorComparison( + expr.Operator.Symbol, leftTyped.ResolvedType(), rightTyped.ResolvedType(), + ) + } } if err != nil { return nil, err @@ -1535,6 +1545,10 @@ func (expr *NullIfExpr) TypeCheck( leftType, op, rightType) return nil, decorateTypeCheckError(err, "incompatible NULLIF expressions") } + err = checkRefCursorComparison(treecmp.EQ, leftType, rightType) + if err != nil { + return nil, err + } expr.Expr1, expr.Expr2 = typedSubExprs[0], typedSubExprs[1] expr.typ = retType @@ -1616,10 +1630,20 @@ func (expr *RangeCond) TypeCheck( ctx context.Context, semaCtx *SemaContext, desired *types.T, ) (TypedExpr, error) { leftFromTyped, fromTyped, _, _, err := typeCheckComparisonOp(ctx, semaCtx, treecmp.MakeComparisonOperator(treecmp.GT), expr.Left, expr.From) + if err == nil { + err = checkRefCursorComparison( + treecmp.GT, leftFromTyped.ResolvedType(), fromTyped.ResolvedType(), + ) + } if err != nil { return nil, err } leftToTyped, toTyped, _, _, err := typeCheckComparisonOp(ctx, semaCtx, treecmp.MakeComparisonOperator(treecmp.LT), expr.Left, expr.To) + if err == nil { + err = checkRefCursorComparison( + treecmp.LT, leftToTyped.ResolvedType(), toTyped.ResolvedType(), + ) + } if err != nil { return nil, err } @@ -3478,3 +3502,42 @@ func CheckUnsupportedType(ctx context.Context, semaCtx *SemaContext, typ *types. } return semaCtx.UnsupportedTypeChecker.CheckType(ctx, typ) } + +// checkRefCursorComparison checks whether the given types are or contain the +// REFCURSOR data type, which is invalid for comparison. We don't simply remove +// the relevant comparison overloads because we rely on their existence in +// various locations throughout the codebase. +func checkRefCursorComparison(op treecmp.ComparisonOperatorSymbol, left, right *types.T) error { + if (op == treecmp.IsNotDistinctFrom || op == treecmp.IsDistinctFrom) && + (left.Family() == types.RefCursorFamily && right == types.Unknown) { + // Special case: "REFCURSOR IS [NOT] DISTINCT FROM NULL" is allowed. + return nil + } + if left.Family() == types.RefCursorFamily || right.Family() == types.RefCursorFamily { + return pgerror.Newf(pgcode.UndefinedFunction, + "unsupported comparison operator: %s %s %s", left, op, right, + ) + } + var checkRecursive func(*types.T) bool + checkRecursive = func(typ *types.T) bool { + switch typ.Family() { + case types.RefCursorFamily: + return true + case types.TupleFamily: + for _, t := range typ.TupleContents() { + if checkRecursive(t) { + return true + } + } + case types.ArrayFamily: + return checkRecursive(typ.ArrayContents()) + } + return false + } + if checkRecursive(left) || checkRecursive(right) { + return pgerror.Newf(pgcode.UndefinedFunction, + "could not identify a comparison function for type refcursor", + ) + } + return nil +} From f466df5abd8815a7b3cf47c46e88cb1dcd7b1658 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Mon, 23 Oct 2023 14:16:44 -0400 Subject: [PATCH 2/4] kv: demonstrate broken lock acquisition replay with writes in same batch See failures in #112174. This commit adds test cases that reproduce the error. Release note: None --- .../testdata/mvcc_histories/replicated_locks | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/pkg/storage/testdata/mvcc_histories/replicated_locks b/pkg/storage/testdata/mvcc_histories/replicated_locks index 3aa9ed5a87ae..660427221a78 100644 --- a/pkg/storage/testdata/mvcc_histories/replicated_locks +++ b/pkg/storage/testdata/mvcc_histories/replicated_locks @@ -891,3 +891,88 @@ data: "k1"/5.000000000,0 -> /BYTES/v1 data: "k2"/5.000000000,0 -> /BYTES/v2 data: "k3"/5.000000000,0 -> /BYTES/v3 stats: key_count=3 key_bytes=45 val_count=3 val_bytes=21 live_count=3 live_bytes=66 + +# Replay lock acquisitions. + +run stats ok +txn_begin t=C ts=12,0 +---- +>> at end: +txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 + +# Replay lock acquisition in batch with no other operations. +run stats ok +with t=C + txn_step seq=1 + acquire_lock k=k1 str=shared + # Replay. + txn_step seq=1 + acquire_lock k=k1 str=shared +---- +>> acquire_lock k=k1 str=shared t=C +stats: lock_count=+1 lock_bytes=+69 lock_age=+88 +>> acquire_lock k=k1 str=shared t=C +stats: no change +>> at end: +txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 +lock (Replicated): "k1"/Shared -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true +stats: key_count=3 key_bytes=45 val_count=3 val_bytes=21 live_count=3 live_bytes=66 lock_count=1 lock_bytes=69 lock_age=88 + +# Replay lock acquisition with an acquisition with a stronger strength in the +# same batch. +run stats error +with t=C + txn_step seq=1 + acquire_lock k=k1 str=shared + txn_step seq=2 + acquire_lock k=k1 str=exclusive + # Replay. + txn_step seq=1 + acquire_lock k=k1 str=shared + txn_step seq=2 + acquire_lock k=k1 str=exclusive +---- +>> acquire_lock k=k1 str=shared t=C +stats: no change +>> acquire_lock k=k1 str=exclusive t=C +stats: lock_count=+1 lock_bytes=+69 lock_age=+88 +>> acquire_lock k=k1 str=shared t=C +stats: no change +>> at end: +txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 +lock (Replicated): "k1"/Exclusive -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=2} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true +lock (Replicated): "k1"/Shared -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true +stats: key_count=3 key_bytes=45 val_count=3 val_bytes=21 live_count=3 live_bytes=66 lock_count=2 lock_bytes=138 lock_age=176 +error: (*withstack.withStack:) cannot acquire lock with strength Shared at seq number 1, already held at higher seq number 2 + +# Replay lock acquisition with a write to the same key in the same batch. +run stats error +with t=C + txn_step seq=1 + acquire_lock k=k2 str=shared + txn_step seq=2 + put k=k2 v=v2_2 + # Replay. + txn_step seq=1 + acquire_lock k=k2 str=shared + txn_step seq=2 + put k=k2 v=v2_2 +---- +>> acquire_lock k=k2 str=shared t=C +stats: lock_count=+1 lock_bytes=+69 lock_age=+88 +>> put k=k2 v=v2_2 t=C +stats: key_bytes=+12 val_count=+1 val_bytes=+59 live_bytes=+52 gc_bytes_age=+1672 intent_count=+1 intent_bytes=+21 lock_count=+1 lock_age=+88 +>> acquire_lock k=k2 str=shared t=C +stats: no change +>> at end: +txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 +data: "k1"/5.000000000,0 -> /BYTES/v1 +meta: "k2"/0,0 -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=2} ts=12.000000000,0 del=false klen=12 vlen=9 mergeTs= txnDidNotUpdateMeta=true +data: "k2"/12.000000000,0 -> /BYTES/v2_2 +data: "k2"/5.000000000,0 -> /BYTES/v2 +data: "k3"/5.000000000,0 -> /BYTES/v3 +lock (Replicated): "k1"/Exclusive -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=2} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true +lock (Replicated): "k1"/Shared -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true +lock (Replicated): "k2"/Shared -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true +stats: key_count=3 key_bytes=57 val_count=4 val_bytes=80 live_count=3 live_bytes=118 gc_bytes_age=1672 intent_count=1 intent_bytes=21 lock_count=4 lock_bytes=207 lock_age=352 +error: (*withstack.withStack:) cannot acquire lock with strength Shared at seq number 1, already held at higher seq number 2 From e2cfb7946c865461c041288a403fd0eb9b4e446a Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Mon, 23 Oct 2023 14:37:00 -0400 Subject: [PATCH 3/4] kv: tolerate lock acquisition replay with writes in same batch Informs #112221. Informs #112174. Informs #112173. Informs #111984. Informs #111893. Informs #111564. Informs #111530. This commit fixes the handling of replayed batches that contain a replicated lock acquisition and a later write to the same key. In such cases, the write at the higher sequence number should not be detected as an error during the replay. Instead, it should simply be ignored. Release note: None --- pkg/storage/mvcc.go | 20 ++++++++++++++++++- .../testdata/mvcc_histories/replicated_locks | 14 +++++++------ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 38d8567a0e17..c86bad0f3e81 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -5645,7 +5645,25 @@ func MVCCAcquireLock( // Acquiring at new epoch. rolledBack = true } else if foundLock.Txn.Sequence > txn.Sequence { - // Acquiring at same epoch and old sequence number. + // Acquiring at same epoch and an old sequence number. + // + // If the found lock has a different strength than the acquisition then we + // ignore it and continue. We are likely part of a replayed batch where a + // later request in the batch acquired a lock with a higher strength (or + // performed an intent write) on the same key. + if iterStr != str { + continue + } + // If the found lock has the same strength as the acquisition then this is + // an unexpected case. We are likely part of a replayed batch and either: + // 1. the lock was reacquired at a later sequence number and the minimum + // acquisition sequence number was not properly retained (bug!). See + // below about why we preserve the earliest non-rolled back sequence + // number for each lock strength. + // 2. this acquisition's sequence number was rolled back and the lock was + // subsequently acquired again at a higher sequence number. In such + // cases, we can return an error as the client is no longer waiting for + // a response. return errors.Errorf( "cannot acquire lock with strength %s at seq number %d, "+ "already held at higher seq number %d", diff --git a/pkg/storage/testdata/mvcc_histories/replicated_locks b/pkg/storage/testdata/mvcc_histories/replicated_locks index 660427221a78..74870779febf 100644 --- a/pkg/storage/testdata/mvcc_histories/replicated_locks +++ b/pkg/storage/testdata/mvcc_histories/replicated_locks @@ -920,7 +920,7 @@ stats: key_count=3 key_bytes=45 val_count=3 val_bytes=21 live_count=3 live_bytes # Replay lock acquisition with an acquisition with a stronger strength in the # same batch. -run stats error +run stats ok with t=C txn_step seq=1 acquire_lock k=k1 str=shared @@ -938,15 +938,16 @@ stats: no change stats: lock_count=+1 lock_bytes=+69 lock_age=+88 >> acquire_lock k=k1 str=shared t=C stats: no change +>> acquire_lock k=k1 str=exclusive t=C +stats: no change >> at end: -txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 +txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=2} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 lock (Replicated): "k1"/Exclusive -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=2} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true lock (Replicated): "k1"/Shared -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true stats: key_count=3 key_bytes=45 val_count=3 val_bytes=21 live_count=3 live_bytes=66 lock_count=2 lock_bytes=138 lock_age=176 -error: (*withstack.withStack:) cannot acquire lock with strength Shared at seq number 1, already held at higher seq number 2 # Replay lock acquisition with a write to the same key in the same batch. -run stats error +run stats ok with t=C txn_step seq=1 acquire_lock k=k2 str=shared @@ -964,8 +965,10 @@ stats: lock_count=+1 lock_bytes=+69 lock_age=+88 stats: key_bytes=+12 val_count=+1 val_bytes=+59 live_bytes=+52 gc_bytes_age=+1672 intent_count=+1 intent_bytes=+21 lock_count=+1 lock_age=+88 >> acquire_lock k=k2 str=shared t=C stats: no change +>> put k=k2 v=v2_2 t=C +stats: no change >> at end: -txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 +txn: "C" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=2} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 data: "k1"/5.000000000,0 -> /BYTES/v1 meta: "k2"/0,0 -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=2} ts=12.000000000,0 del=false klen=12 vlen=9 mergeTs= txnDidNotUpdateMeta=true data: "k2"/12.000000000,0 -> /BYTES/v2_2 @@ -975,4 +978,3 @@ lock (Replicated): "k1"/Exclusive -> txn={id=00000003 key=/Min iso=Serializable lock (Replicated): "k1"/Shared -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true lock (Replicated): "k2"/Shared -> txn={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=1} ts=12.000000000,0 del=false klen=0 vlen=0 mergeTs= txnDidNotUpdateMeta=true stats: key_count=3 key_bytes=57 val_count=4 val_bytes=80 live_count=3 live_bytes=118 gc_bytes_age=1672 intent_count=1 intent_bytes=21 lock_count=4 lock_bytes=207 lock_age=352 -error: (*withstack.withStack:) cannot acquire lock with strength Shared at seq number 1, already held at higher seq number 2 From e9e9b338cbc8acb134e53ba267e8fb9788820a37 Mon Sep 17 00:00:00 2001 From: maryliag Date: Mon, 23 Oct 2023 14:04:50 -0400 Subject: [PATCH 4/4] sql: fix for crash with null elements on jsonb_array_to_string_array Fixes #112829 The builtin `jsonb_array_to_string_array` was crashing, when it had `null` elements. This commit handles this case, removing them from the final array. Release note: None --- docs/generated/sql/functions.md | 2 +- pkg/sql/logictest/testdata/logic_test/array | 20 ++++++++++++++++++++ pkg/sql/sem/builtins/builtins.go | 10 ++++++---- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/docs/generated/sql/functions.md b/docs/generated/sql/functions.md index 037cc3ba1b24..c2524c7b8b76 100644 --- a/docs/generated/sql/functions.md +++ b/docs/generated/sql/functions.md @@ -337,7 +337,7 @@ Immutable cardinality(input: anyelement[]) → int

Calculates the number of elements contained in input

Immutable -jsonb_array_to_string_array(input: jsonb) → string[]

Convert a JSONB array into a string array.

+jsonb_array_to_string_array(input: jsonb) → string[]

Convert a JSONB array into a string array, removing ‘null’ elements.

Immutable string_to_array(str: string, delimiter: string) → string[]

Split a string into components on a delimiter.

Immutable diff --git a/pkg/sql/logictest/testdata/logic_test/array b/pkg/sql/logictest/testdata/logic_test/array index 57c5cf027be5..68914e0d1377 100644 --- a/pkg/sql/logictest/testdata/logic_test/array +++ b/pkg/sql/logictest/testdata/logic_test/array @@ -1215,6 +1215,26 @@ SELECT jsonb_array_to_string_array(j -> 'a') FROM str_arr ORDER BY j ASC query error input argument must be JSON array type SELECT jsonb_array_to_string_array(j -> 'b') FROM str_arr ORDER BY j ASC +query T +SELECT jsonb_array_to_string_array('[1, 2, 3]':::JSONB) +---- +{1,2,3} + +query T +SELECT jsonb_array_to_string_array('[true, false, false, true]':::JSONB) +---- +{true,false,false,true} + +query T +SELECT jsonb_array_to_string_array('[null]':::JSONB) +---- +{} + +query T +SELECT jsonb_array_to_string_array('["foo", null]':::JSONB) +---- +{foo} + # Regression test for #23429. statement ok diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 832b762bdccb..3f8e163bb04e 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -3566,14 +3566,16 @@ value if you rely on the HLC for accuracy.`, if err != nil { return nil, err } - err = strArray.Append(tree.NewDString(*str)) - if err != nil { - return nil, err + if str != nil { + err = strArray.Append(tree.NewDString(*str)) + if err != nil { + return nil, err + } } } return strArray, nil }, - Info: "Convert a JSONB array into a string array.", + Info: "Convert a JSONB array into a string array, removing 'null' elements.", Volatility: volatility.Immutable, CalledOnNullInput: true, }),