Merge cockroachdb#112897 cockroachdb#113166 cockroachdb#113286

112897: builtins: return expected error for null arguments to PLpgSQL builtins r=DrewKimball a=DrewKimball

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 cockroachdb#112895

Release note: None

113166: opt: disallow mutations on virtual tables r=DrewKimball a=DrewKimball

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 cockroachdb#111414
Fixes cockroachdb#111644

Release note: None

113286: kvserver: deflake test base requeue r=arulajmani a=kvoli

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: cockroachdb#113045
Release note: None

Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: Austen McClernon <[email protected]>

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
 	}

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

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"))

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 [<table_id>(<col1_id>,<col2_id>) AS <alias>] ...

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"))

pkg/sql/sem/builtins/builtins.go

@@ -8560,7 +8560,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])))
 			},
@@ -8582,6 +8584,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])