Merge #155780 #155821 #156483

155780: roachtest: more logging on deleted sst in OR recovery r=msbutler a=kev-cao

This commit adds more logging on the SST that is deleted in the OR recovery roachtest to determine exactly what backed up span and data was deleted.

Epic: None

Release note: None

155821: optbuilder, sql: fix stable sql api edge cases r=angles-n-daemons a=angles-n-daemons

The logictests have flushed out a handful of situations in which the stable sql api gates block unexpectedly. Examples of this include unsafe access in safe builtin execution, nested delegates, and other similar use cases. What was required to fix them was to update the way we unguard the unsafe internals during query planning.

Fixes: #154675
Epic: CRDB-55267
Release note: None

156483: logical: do not allow the REFCURSOR type r=jeffswenson a=jeffswenson

This removes LDR support for the REFCURSOR type. The crud writer does not support it because REFCURSOR does not support the equality SQL operator.

This removal is unlikely to impact any users. The REFCURSOR is a weird type that should not show up in a durable table. If the user really needs this type for some reason, they could store they type as a STRING and convert it back to a REFCURSOR when reading from the table.

Epic: CRDB-48647
Release note: none

Co-authored-by: Kevin Cao <[email protected]>
Co-authored-by: Brian Dillmann <[email protected]>
Co-authored-by: Jeff Swenson <[email protected]>

Author: 4 people

pkg/cmd/roachtest/tests/mixed_version_backup.go

@@ -2383,9 +2383,11 @@ func (d *BackupRestoreTestDriver) deleteUserTableSST(
 	ctx context.Context, l *logger.Logger, db *gosql.DB, collection *backupCollection,
 ) error {
 	var sstPath string
+	var startPretty, endPretty string
+	var sizeBytes, numRows int
 	if err := db.QueryRowContext(
 		ctx,
-		`SELECT path FROM
+		`SELECT path, start_pretty, end_pretty, size_bytes, rows FROM
 		(
 			SELECT row_number() OVER (), * FROM
 			[SHOW BACKUP FILES FROM LATEST IN $1]
@@ -2394,14 +2396,17 @@ func (d *BackupRestoreTestDriver) deleteUserTableSST(
 		ORDER BY row_number DESC
 		LIMIT 1`,
 		collection.uri(),
-	).Scan(&sstPath); err != nil {
+	).Scan(&sstPath, &startPretty, &endPretty, &sizeBytes, &numRows); err != nil {
 		return errors.Wrapf(err, "failed to get SST path from %s", collection.uri())
 	}
 	uri, err := url.Parse(collection.uri())
 	if err != nil {
 		return errors.Wrapf(err, "failed to parse backup collection URI %q", collection.uri())
 	}
-	l.Printf("deleting sst %s at %s", sstPath, uri)
+	l.Printf(
+		"deleting sst %s at %s: covering %d bytes and %d rows in [%s, %s)",
+		sstPath, uri, sizeBytes, numRows, startPretty, endPretty,
+	)
 	storage, err := cloud.ExternalStorageFromURI(
 		ctx,
 		collection.uri(),

pkg/crosscluster/logical/logical_replication_job_test.go

@@ -2833,6 +2833,11 @@ func TestLogicalReplicationCreationChecks(t *testing.T) {
 	jobutils.WaitForJobToCancel(t, dbA, jobAID)
 	replicationtestutils.WaitForAllProducerJobsToFail(t, dbB)
 
+	// Check that REFCURSOR columns are not allowed.
+	dbA.Exec(t, "CREATE TABLE tab_with_refcursor (pk INT PRIMARY KEY, curs REFCURSOR)")
+	dbB.Exec(t, "CREATE TABLE b.tab_with_refcursor (pk INT PRIMARY KEY, curs REFCURSOR)")
+	expectErr(t, "tab_with_refcursor", "cannot create logical replication stream: column curs is a RefCursor")
+
 	// Add different default values to to the source and dest, verify the stream
 	// can be created, and that the default value is sent over the wire.
 	dbA.Exec(t, "CREATE TABLE tab2 (pk INT PRIMARY KEY, payload STRING DEFAULT 'cat')")

pkg/sql/catalog/tabledesc/logical_replication_helpers.go

@@ -58,6 +58,22 @@ func CheckLogicalReplicationCompatibility(
 		return pgerror.Wrapf(err, pgcode.InvalidTableDefinition, cannotLDRMsg)
 	}
 
+	if err := checkForbiddenTypes(dst); err != nil {
+		return pgerror.Wrapf(err, pgcode.InvalidTableDefinition, cannotLDRMsg)
+	}
+
+	return nil
+}
+
+func checkForbiddenTypes(dst *descpb.TableDescriptor) error {
+	for _, col := range dst.Columns {
+		// RefCursor is not supported by LDR because it has no definition of
+		// equality. It's a weird type that should never exist in a durable table
+		// since a cursor is a session scoped entity.
+		if col.Type.Family() == types.RefCursorFamily {
+			return errors.Newf("column %s is a RefCursor", col.Name)
+		}
+	}
 	return nil
 }
 

pkg/sql/opt/optbuilder/builder.go

@@ -593,6 +593,11 @@ func (b *Builder) maybeTrackUserDefinedTypeDepsForViews(texpr tree.TypedExpr) {
 // DisableUnsafeInternalCheck is used to disable the check that the
 // prevents external users from accessing unsafe internals.
 func (b *Builder) DisableUnsafeInternalCheck() func() {
+	// Already in the middle of a disabled section.
+	if b.skipUnsafeInternalsCheck {
+		return func() {}
+	}
+
 	b.skipUnsafeInternalsCheck = true
 	var cleanup func()
 	if b.catalog != nil {

pkg/sql/opt/optbuilder/routine.go

@@ -51,6 +51,11 @@ func (b *Builder) buildUDF(
 		))
 	}
 
+	// builtins should have access to unsafe internals
+	if o.Type == tree.BuiltinRoutine {
+		defer b.DisableUnsafeInternalCheck()()
+	}
+
 	// Check for execution privileges for user-defined overloads. Built-in
 	// overloads do not need to be checked.
 	if o.Type == tree.UDFRoutine {

pkg/sql/opt_catalog.go

@@ -744,13 +744,9 @@ func (oc *optCatalog) codec() keys.SQLCodec {
 	return oc.planner.ExecCfg().Codec
 }
 
-// DisableUnsafeInternalCheck sets the planners skipUnsafeInternalsCheck
-// to true, and returns a function which reverses it to false.
+// DisableUnsafeInternalCheck forwards the call to the planner.
 func (oc *optCatalog) DisableUnsafeInternalCheck() func() {
-	oc.planner.skipUnsafeInternalsCheck = true
-	return func() {
-		oc.planner.skipUnsafeInternalsCheck = false
-	}
+	return oc.planner.DisableUnsafeInternalsCheck()
 }
 
 // optView is a wrapper around catalog.TableDescriptor that implements

pkg/sql/planner.go

@@ -676,6 +676,19 @@ func (p *planner) InternalSQLTxn() descs.Txn {
 	return &p.internalSQLTxn
 }
 
+// DisableUnsafeInternalCheck sets the skipUnsafeInternalsCheck property
+// to true, and returns a function which reverses it to false.
+func (p *planner) DisableUnsafeInternalsCheck() func() {
+	if p.skipUnsafeInternalsCheck {
+		return func() {}
+	}
+
+	p.skipUnsafeInternalsCheck = true
+	return func() {
+		p.skipUnsafeInternalsCheck = false
+	}
+}
+
 func (p *planner) regionsProvider() *regions.Provider {
 	if txn := p.InternalSQLTxn(); txn != nil {
 		_ = txn.Regions() // force initialization

pkg/sql/unsafesql/unsafesql_test.go

@@ -231,6 +231,16 @@ func TestAccessCheckServer(t *testing.T) {
 			Passes:     false,
 			LogsDenied: true,
 		},
+		{
+			// test nested delegates
+			Query:  "SHOW JOBS WHEN COMPLETE SELECT job_id FROM [SHOW JOBS]",
+			Passes: true,
+		},
+		{
+			// unexpected unsafe builtin access
+			Query:  "SELECT col_description(0, 0)",
+			Passes: true,
+		},
 	} {
 		t.Run(fmt.Sprintf("query=%s,internal=%t,allowUnsafe=%t", test.Query, test.Internal, test.AllowUnsafeInternals), func(t *testing.T) {
 			accessedSpy.Reset()