Merge #144958 #145456

144958: backup: fail compaction jobs upon encountering range keys  r=msbutler a=kev-cao

Previously if a compaction job encountered a range key, it would erroneously continue the job and succeed. As the current implementation of the backup compaction iterator skips range keys, if the compaction job encounters an SST file that contains range keys, it should fail the job.

Epic: None

Release note: Compaction jobs now properly fail upon encountering range keys.

145456: sql: prohibit jsonpath as element in composite type r=yuzefovich a=yuzefovich

This commit ties a few loose ends around the jsonpath data type:
- it prohibits creation of a composite type with jsonpath as an element (it should be fine, but we're doing this out of caution until we have encoding for jsonpath)
- it adds a validation for each element of a composite type for whether it can be used as a column type (under an assumption that if a type cannot be used as a column type, then any composite types consisting of that type cannot either)
- it also adds the issue number to the errors.

Informs: #144910.
Fixes: #144911.

Release note: None

Co-authored-by: Kevin Cao <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: 3 people

pkg/backup/compaction_job.go

@@ -534,7 +534,7 @@ func (c compactionChain) createCompactionManifest(
 	// range keys.
 	lastBackup := c.lastBackup()
 	if len(lastBackup.Tenants) != 0 {
-		return nil, errors.New("backup compactions does not yet support range keys")
+		return nil, errors.New("backup compactions does not support range keys")
 	}
 	if err := checkCoverage(ctx, c.lastBackup().Spans, c.backupChain); err != nil {
 		return nil, err

pkg/backup/compaction_processor.go

@@ -367,6 +367,11 @@ func openSSTs(
 	storeFiles := make([]storageccl.StoreFile, 0, len(entry.Files))
 	for idx := range entry.Files {
 		file := entry.Files[idx]
+		if file.HasRangeKeys {
+			// TODO (kev-cao): Come back and update this to range keys when
+			// SSTSinkKeyWriter has been updated to support range keys.
+			return mergedSST{}, errors.New("backup compactions does not support range keys")
+		}
 		dir, err := execCfg.DistSQLSrv.ExternalStorage(ctx, file.Dir)
 		if err != nil {
 			return mergedSST{}, err
@@ -375,8 +380,6 @@ func openSSTs(
 		storeFiles = append(storeFiles, storageccl.StoreFile{Store: dir, FilePath: file.Path})
 	}
 	iterOpts := storage.IterOptions{
-		// TODO (kev-cao): Come back and update this to range keys when
-		// SSTSinkKeyWriter has been updated to support range keys.
 		KeyTypes:   storage.IterKeyTypePointsOnly,
 		LowerBound: keys.LocalMax,
 		UpperBound: keys.MaxKey,

pkg/backup/datadriven_test.go

@@ -397,6 +397,10 @@ func (d *datadrivenTestState) getSQLDBForVC(
 //   - expect-pausepoint: expects the backup job to end up in a paused state because
 //     of a pausepoint error.
 //
+//   - aost: expects a tag referencing a previously saved cluster timestamp
+//     using `save-cluster-ts`. It then runs the backup as of the saved cluster
+//     timestamp.
+//
 //   - "restore" [args]
 //     Executes restore specific operations.
 //
@@ -410,13 +414,29 @@ func (d *datadrivenTestState) getSQLDBForVC(
 //   - aost: expects a tag referencing a previously saved cluster timestamp
 //     using `save-cluster-ts`. It then runs the restore as of the saved cluster
 //     timestamp.
+
+//   - "compact" [args]
+//     Executes compaction specific operations.
+//
+//     Supported arguments:
+//
+//   - tag=<tag>: tag the compaction job to reference it in the future.
+//
+//   - expect-pausepoint: expects the restore job to end up in a paused state because
+//     of a pausepoint error.
+//
+//   - start: expects a tag referencing a previously saved cluster timestamp
+//     using `save-cluster-ts`.
+//
+//   - end: expects a tag referencing a previously saved cluster timestamp
+//     using `save-cluster-ts`.
 //
 //   - "schema" [args]
 //     Executes schema change specific operations.
 //
 //     Supported arguments:
 //
-//   - tag=<tag>: tag the schema change job to reference it in the future.
+// - tag=<tag>: tag the schema change job to reference it in the future.
 //
 //   - expect-pausepoint: expects the schema change job to end up in a paused state because
 //     of a pausepoint error.
@@ -426,9 +446,9 @@ func (d *datadrivenTestState) getSQLDBForVC(
 //
 //     Supported arguments:
 //
-//   - type: kv request type. Currently, only DeleteRange is supported
+// - type: kv request type. Currently, only DeleteRange is supported
 //
-//   - target: SQL target. Currently, only table names are supported.
+// - target: SQL target. Currently, only table names are supported.
 //
 //   - "corrupt-backup" uri=<collectionUri>
 //     Finds the latest backup in the provided collection uri an flips a bit in one SST in the backup
@@ -797,6 +817,18 @@ func runTestDataDriven(t *testing.T, testFilePathFromWorkspace string) {
 			return ""
 
 		case "backup":
+			if d.HasArg("aost") {
+				var aost string
+				d.ScanArgs(t, "aost", &aost)
+				var ts string
+				var ok bool
+				if ts, ok = ds.clusterTimestamps[aost]; !ok {
+					t.Fatalf("no cluster timestamp found for %s", aost)
+				}
+				// Replace the ts tag with the actual timestamp.
+				d.Input = strings.Replace(d.Input, aost,
+					fmt.Sprintf("'%s'", ts), 1)
+			}
 			return execWithTagAndPausePoint(jobspb.TypeBackup)
 
 		case "import":
@@ -818,6 +850,29 @@ func runTestDataDriven(t *testing.T, testFilePathFromWorkspace string) {
 			}
 			return execWithTagAndPausePoint(jobspb.TypeRestore)
 
+		case "compact":
+			if !d.HasArg("start") || !d.HasArg("end") {
+				t.Fatalf("must specify start and end for compaction")
+			}
+			var start, end string
+			d.ScanArgs(t, "start", &start)
+			d.ScanArgs(t, "end", &end)
+			var startTs, endTs string
+			var ok bool
+			if startTs, ok = ds.clusterTimestamps[start]; !ok {
+				t.Fatalf("no cluster timestamp found for %s", start)
+			}
+			if endTs, ok = ds.clusterTimestamps[end]; !ok {
+				t.Fatalf("no cluster timestamp found for %s", end)
+			}
+			// Replace the ts tag with the actual timestamp.
+			// ds.clusterTimestamps stores a stringified nanosecond epoch.
+			d.Input = strings.Replace(d.Input, start,
+				fmt.Sprintf("'%s'::DECIMAL", startTs), 1)
+			d.Input = strings.Replace(d.Input, end,
+				fmt.Sprintf("'%s'::DECIMAL", endTs), 1)
+
+			return execWithTagAndPausePoint(jobspb.TypeBackup)
 		case "new-schema-change":
 			return execWithTagAndPausePoint(jobspb.TypeNewSchemaChange)
 

pkg/backup/testdata/backup-restore/rangekeys

@@ -29,8 +29,11 @@ exec-sql
 INSERT INTO foo VALUES (3,'z');
 ----
 
-exec-sql
-BACKUP INTO 'nodelocal://1/test-root/';
+save-cluster-ts tag=start
+----
+
+backup aost=start
+BACKUP INTO 'nodelocal://1/test-root/' AS OF SYSTEM TIME start;
 ----
 
 exec-sql
@@ -61,15 +64,27 @@ exec-sql
 INSERT INTO foo VALUES (4,'a'),(5,'b');
 ----
 
+# Create two incremental backups to test compaction
+exec-sql
+BACKUP INTO LATEST IN 'nodelocal://1/test-root/';
+----
+
+exec-sql
+INSERT INTO foo VALUES (6,'a'),(7,'b');
+----
+
 kv request=DeleteRange target=baz
 ----
 
 exec-sql
 INSERT INTO baz VALUES (33,'zz');
 ----
 
-exec-sql
-BACKUP INTO LATEST IN 'nodelocal://1/test-root/';
+save-cluster-ts tag=end
+----
+
+backup aost=end
+BACKUP INTO LATEST IN 'nodelocal://1/test-root/' AS OF SYSTEM TIME end;
 ----
 
 exec-sql
@@ -84,9 +99,25 @@ regex matches error
 query-sql
 SELECT count(*) from orig1.foo
 ----
-3
+5
 
 query-sql
 SELECT count(*) from orig1.baz
 ----
 1
+
+let $backup_path
+SHOW BACKUPS IN 'nodelocal://1/test-root/';
+----
+
+compact start=start end=end tag=comp_job
+SELECT crdb_internal.backup_compaction('BACKUP INTO LATEST IN ''nodelocal://1/test-root/''', '$backup_path', start, end);
+----
+
+job tag=comp_job wait-for-state=failed
+----
+
+query-sql regex=(compactions does not support range keys)
+SELECT error FROM [SHOW JOBS] WHERE job_type = 'BACKUP' AND status = 'failed' ORDER BY created DESC LIMIT 1;
+----
+true

pkg/sql/catalog/colinfo/col_type_info.go

@@ -97,6 +97,10 @@ func ValidateColumnDefType(ctx context.Context, st *cluster.Settings, t *types.T
 			return unimplemented.NewWithIssueDetailf(23468, t.String(),
 				"arrays of JSON unsupported as column type")
 		}
+		if t.ArrayContents().Family() == types.JsonpathFamily {
+			return unimplemented.NewWithIssueDetailf(144910, t.String(),
+				"arrays of jsonpath unsupported as column type")
+		}
 		if err := types.CheckArrayElementType(t.ArrayContents()); err != nil {
 			return err
 		}
@@ -109,13 +113,22 @@ func ValidateColumnDefType(ctx context.Context, st *cluster.Settings, t *types.T
 		types.TSQueryFamily, types.TSVectorFamily, types.PGLSNFamily, types.PGVectorFamily, types.RefCursorFamily:
 	// These types are OK.
 
+	case types.JsonpathFamily:
+		return unimplemented.NewWithIssueDetailf(144910, t.String(),
+			"jsonpath unsupported as column type")
+
 	case types.TupleFamily:
 		if !t.UserDefined() {
 			return pgerror.New(pgcode.InvalidTableDefinition, "cannot use anonymous record type as table column")
 		}
 		if t.TypeMeta.ImplicitRecordType {
 			return unimplemented.NewWithIssue(70099, "cannot use table record type as table column")
 		}
+		for _, typ := range t.TupleContents() {
+			if err := ValidateColumnDefType(ctx, st, typ); err != nil {
+				return err
+			}
+		}
 
 	default:
 		return pgerror.Newf(pgcode.InvalidTableDefinition,

pkg/sql/create_type.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/enum"
+	"github.com/cockroachdb/cockroach/pkg/sql/oidext"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
@@ -400,6 +401,10 @@ func CreateEnumTypeDesc(
 	}).BuildCreatedMutableType(), nil
 }
 
+var jsonpathInCompositeErr = unimplemented.NewWithIssueDetailf(
+	144910, "", "jsonpath cannot be used in a composite type",
+)
+
 // createCompositeTypeDesc creates a new composite type descriptor.
 func createCompositeTypeDesc(
 	params runParams,
@@ -426,6 +431,10 @@ func createCompositeTypeDesc(
 		if typ.Identical(types.Trigger) {
 			return nil, tree.CannotAcceptTriggerErr
 		}
+		if typ.Oid() == oidext.T_jsonpath || typ.Oid() == oidext.T__jsonpath {
+			// TODO(#144910): this is unsupported for now, out of caution.
+			return nil, jsonpathInCompositeErr
+		}
 		if err = tree.CheckUnsupportedType(params.ctx, &params.p.semaCtx, typ); err != nil {
 			return nil, err
 		}

pkg/sql/logictest/testdata/logic_test/jsonpath

@@ -10,12 +10,15 @@ SELECT '$.a'::JSONPATH
 ----
 $."a"
 
-statement error pq: value type jsonpath cannot be used for table columns
+statement error pq: unimplemented: jsonpath unsupported as column type
 CREATE TABLE a (j JSONPATH)
 
-statement error pq: value type jsonpath cannot be used for table columns
+statement error pq: unimplemented: arrays of jsonpath unsupported as column type
 CREATE TABLE a (j JSONPATH[])
 
+statement error pq: unimplemented: jsonpath cannot be used in a composite type
+CREATE TYPE typ AS (j JSONPATH);
+
 query T
 SELECT '$'::JSONPATH
 ----

pkg/sql/sem/tree/create.go

@@ -370,7 +370,7 @@ type CreateType struct {
 	Variety  CreateTypeVariety
 	// EnumLabels is set when this represents a CREATE TYPE ... AS ENUM statement.
 	EnumLabels EnumValueList
-	// CompositeTypeList is set when this repesnets a CREATE TYPE ... AS ( )
+	// CompositeTypeList is set when this represents a CREATE TYPE ... AS ( )
 	// statement.
 	CompositeTypeList []CompositeTypeElem
 	// IfNotExists is true if IF NOT EXISTS was requested.