From b38ea4a78de99f7198dc81ea8155b3428eb6116b Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Mon, 23 Oct 2023 12:55:12 -0600 Subject: [PATCH 1/3] builtins: return expected error for null arguments to PLpgSQL builtins This patch adds checks for NULL arguments to the `crdb_internal.plpgsql_close` and `crdb_internal.plpgsql_fetch` builtin functions so that they will now return an expected error, rather than an assertion failure. Fixes #112895 Release note: None --- .../testdata/logic_test/plpgsql_builtins | 27 +++++++++++++++++++ pkg/sql/sem/builtins/builtins.go | 11 +++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/pkg/sql/logictest/testdata/logic_test/plpgsql_builtins b/pkg/sql/logictest/testdata/logic_test/plpgsql_builtins index 8cc93a520c0b..63d5368f703d 100644 --- a/pkg/sql/logictest/testdata/logic_test/plpgsql_builtins +++ b/pkg/sql/logictest/testdata/logic_test/plpgsql_builtins @@ -627,3 +627,30 @@ DECLARE foo CURSOR FOR SELECT * FROM xy ORDER BY x DESC; statement error pgcode 22023 pq: invalid fetch/move direction: 10 SELECT crdb_internal.plpgsql_fetch('foo', 10, 0, (NULL::INT, NULL::INT)); + +statement ok +ABORT; + +# Regression test for 112895. The plpgsql_close and plpgsql_fetch builtin +# functions should return an expected error for NULL arguments. +subtest null_error + +statement error pgcode 22004 pq: FETCH statement option cannot be null +SELECT crdb_internal.plpgsql_fetch(NULL, 0, 1, (NULL::INT, NULL::INT)); + +statement error pgcode 22004 pq: FETCH statement option cannot be null +SELECT crdb_internal.plpgsql_fetch('foo', NULL, 1, (NULL::INT, NULL::INT)); + +statement error pgcode 22004 pq: FETCH statement option cannot be null +SELECT crdb_internal.plpgsql_fetch('foo', 0, NULL, (NULL::INT, NULL::INT)); + +statement error pgcode 22004 pq: FETCH statement option cannot be null +SELECT crdb_internal.plpgsql_fetch('foo', 0, 1, NULL); + +statement error pgcode 22004 pq: FETCH statement option cannot be null +SELECT crdb_internal.plpgsql_fetch(NULL, NULL, NULL, NULL); + +statement error pgcode 22004 pq: cursor name for CLOSE statement cannot be null +SELECT crdb_internal.plpgsql_close(NULL); + +subtest end diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 832b762bdccb..74e0a7bad2de 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -8553,7 +8553,9 @@ specified store on the node it's run from. One of 'mvccGC', 'merge', 'split', ReturnType: tree.FixedReturnType(types.Int), Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { if args[0] == tree.DNull { - return nil, errors.AssertionFailedf("expected non-null argument for plpgsql_close") + return nil, pgerror.New( + pgcode.NullValueNotAllowed, "cursor name for CLOSE statement cannot be null", + ) } return tree.DNull, evalCtx.Planner.PLpgSQLCloseCursor(tree.Name(tree.MustBeDString(args[0]))) }, @@ -8575,6 +8577,13 @@ specified store on the node it's run from. One of 'mvccGC', 'merge', 'split', }, ReturnType: tree.IdentityReturnType(3), Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { + for i := range args { + if args[i] == tree.DNull { + return nil, pgerror.New( + pgcode.NullValueNotAllowed, "FETCH statement option cannot be null", + ) + } + } cursorName := tree.MustBeDString(args[0]) cursorDir := tree.MustBeDInt(args[1]) cursorCount := tree.MustBeDInt(args[2]) From ed6c2d526165f2ef6e882cefcd7b88375c762c25 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Thu, 26 Oct 2023 13:04:58 -0600 Subject: [PATCH 2/3] opt: disallow mutations on virtual tables This patch adds a defensive check to verify that the target of an insert, delete, or update is not a virtual table. Note that virtual tables already disallow all access privileges apart from `SELECT`, but we've seen internal errors due to an (unknown) code path where that check is failing. This patch doesn't add tests because the privilege check is expected to fail before the virtual table check. Fixes #111414 Fixes #111644 Release note: None --- pkg/sql/opt/optbuilder/delete.go | 6 ++++++ pkg/sql/opt/optbuilder/insert.go | 6 ++++++ pkg/sql/opt/optbuilder/update.go | 6 ++++++ 3 files changed, 18 insertions(+) diff --git a/pkg/sql/opt/optbuilder/delete.go b/pkg/sql/opt/optbuilder/delete.go index c4a83613d8d2..5cfeb246fc62 100644 --- a/pkg/sql/opt/optbuilder/delete.go +++ b/pkg/sql/opt/optbuilder/delete.go @@ -63,6 +63,12 @@ func (b *Builder) buildDelete(del *tree.Delete, inScope *scope) (outScope *scope // Find which table we're working on, check the permissions. tab, depName, alias, refColumns := b.resolveTableForMutation(del.Table, privilege.DELETE) + if tab.IsVirtualTable() { + panic(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "cannot delete from view \"%s\"", tab.Name(), + )) + } + if refColumns != nil { panic(pgerror.Newf(pgcode.Syntax, "cannot specify a list of column IDs with DELETE")) diff --git a/pkg/sql/opt/optbuilder/insert.go b/pkg/sql/opt/optbuilder/insert.go index 0bf1fe5b6ff9..0a9ce5e86018 100644 --- a/pkg/sql/opt/optbuilder/insert.go +++ b/pkg/sql/opt/optbuilder/insert.go @@ -184,6 +184,12 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope // Find which table we're working on, check the permissions. tab, depName, alias, refColumns := b.resolveTableForMutation(ins.Table, privilege.INSERT) + if tab.IsVirtualTable() { + panic(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "cannot insert into view \"%s\"", tab.Name(), + )) + } + // It is possible to insert into specific columns using table reference // syntax: // INSERT INTO [(,) AS ] ... diff --git a/pkg/sql/opt/optbuilder/update.go b/pkg/sql/opt/optbuilder/update.go index c70400421c04..c60c98a245ab 100644 --- a/pkg/sql/opt/optbuilder/update.go +++ b/pkg/sql/opt/optbuilder/update.go @@ -77,6 +77,12 @@ func (b *Builder) buildUpdate(upd *tree.Update, inScope *scope) (outScope *scope // Find which table we're working on, check the permissions. tab, depName, alias, refColumns := b.resolveTableForMutation(upd.Table, privilege.UPDATE) + if tab.IsVirtualTable() { + panic(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "cannot update view \"%s\"", tab.Name(), + )) + } + if refColumns != nil { panic(pgerror.Newf(pgcode.Syntax, "cannot specify a list of column IDs with UPDATE")) From 0d80e6140e01b132f290ad16e987e0c829ba5635 Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Mon, 30 Oct 2023 14:16:05 +0000 Subject: [PATCH 3/3] kvserver: deflake test base requeue It was possible for there to be concurrent r/w to the testing `err` value in `TestBaseQueueRequeue` if the succeeds soon loop exited quickly enough. Defer the atomic `processed` increment, so that it blocks the succeeds soon loop from exiting before reading the test `err`. Fixes: #113045 Release note: None --- pkg/kv/kvserver/queue_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kv/kvserver/queue_test.go b/pkg/kv/kvserver/queue_test.go index 8ed69c0c4651..484aaa2f26e7 100644 --- a/pkg/kv/kvserver/queue_test.go +++ b/pkg/kv/kvserver/queue_test.go @@ -65,7 +65,7 @@ func (tq *testQueueImpl) shouldQueue( func (tq *testQueueImpl) process( _ context.Context, _ *Replica, _ spanconfig.StoreReader, ) (bool, error) { - atomic.AddInt32(&tq.processed, 1) + defer atomic.AddInt32(&tq.processed, 1) if tq.err != nil { return false, tq.err }