diff --git a/build/teamcity-acceptance.sh b/build/teamcity-acceptance.sh index d614cf9861f8..6950bfcaeb63 100755 --- a/build/teamcity-acceptance.sh +++ b/build/teamcity-acceptance.sh @@ -38,8 +38,11 @@ tc_start_block "Run acceptance tests" # is the only way to get sane `-json` behavior (test2json has various shortcomings when tests # time out, etc). So we have it build `emptypkg` (which has no deps, duh) but really will run # the thing we care about, 'acceptance.test'. +# +# Note that ./pkg/acceptance without the tag is an empty test package, so it's fine to compile +# outside of builder. run_json_test env TZ=America/New_York stdbuf -eL -oL go test \ -mod=vendor -json -timeout 30m -v \ - -exec "../../../build/teamcity-go-test-precompiled.sh ./pkg/acceptance/acceptance.test" ./pkg/testutils/emptypkg \ + -exec "../../build/teamcity-go-test-precompiled.sh ./pkg/acceptance/acceptance.test" ./pkg/acceptance \ -l "$TMPDIR" tc_end_block "Run acceptance tests" diff --git a/build/teamcity-compose.sh b/build/teamcity-compose.sh index 3ca6d76cefc3..9842a82fdf35 100755 --- a/build/teamcity-compose.sh +++ b/build/teamcity-compose.sh @@ -23,6 +23,7 @@ tc_end_block "Compile compose tests" tc_start_block "Run compose tests" # NB: we're cheating go test into invoking our `compose.test` over the one it -# builds itself. +# builds itself. Note that ./pkg/compose without tags builds an empty test +# binary. run_json_test go test -json -v -timeout 30m -exec ../../build/teamcity-go-test-precompiled.sh ./compose.test ./pkg/compose tc_end_block "Run compose tests" diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 349a028024b3..16b52a495e24 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -3621,6 +3621,260 @@ func TestBackupRestoreSequence(t *testing.T) { }) } +func TestBackupRestoreSequenceOwnership(t *testing.T) { + defer leaktest.AfterTest(t)() + const numAccounts = 1 + _, _, origDB, dir, cleanupFn := BackupRestoreTestSetup(t, singleNode, numAccounts, InitNone) + defer cleanupFn() + args := base.TestServerArgs{ExternalIODir: dir} + + // Setup for sequence ownership backup/restore tests in the same database. + backupLoc := LocalFoo + `/d` + origDB.Exec(t, `CREATE DATABASE d`) + origDB.Exec(t, `CREATE TABLE d.t(a int)`) + origDB.Exec(t, `CREATE SEQUENCE d.seq OWNED BY d.t.a`) + origDB.Exec(t, `BACKUP DATABASE d TO $1`, backupLoc) + + // When restoring a database which has a owning table and an owned sequence, + // the ownership relationship should be preserved and remapped post restore. + t.Run("test restoring database should preserve ownership dependency", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.Exec(t, `RESTORE DATABASE d FROM $1`, backupLoc) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t") + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq") + + require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner after restore", + ) + require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner after restore", + ) + require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d.t after restore", + ) + require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d.t after restore", + ) + }) + + // When restoring a sequence that is owned by a table, but the owning table + // does not exist, the user must specify the `skip_missing_sequence_owners` + // flag. When supplied, the sequence should be restored without an owner. + t.Run("test restoring sequence when table does not exist", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + newDB.Exec(t, `CREATE DATABASE d`) + newDB.Exec(t, `USE d`) + newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner`, + `RESTORE TABLE seq FROM $1`, backupLoc) + + newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc) + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq") + require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected owner of restored sequence.") + }) + + // When just the table is restored by itself, the ownership dependency is + // removed as the sequence doesn't exist. When the sequence is restored + // after that, it requires the `skip_missing_sequence_owners` flag as + // the table isn't being restored with it, and when provided, the sequence + // shouldn't have an owner. + t.Run("test restoring table then sequence should remove ownership dependency", + func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + newDB.Exec(t, `CREATE DATABASE d`) + newDB.Exec(t, `USE d`) + newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner table`, + `RESTORE TABLE seq FROM $1`, backupLoc) + + newDB.ExpectErr(t, `pq: cannot restore table "t" without referenced sequence`, + `RESTORE TABLE t FROM $1`, backupLoc) + newDB.Exec(t, `RESTORE TABLE t FROM $1 WITH skip_missing_sequence_owners`, backupLoc) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t") + + require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "expected restored table to own 0 sequences", + ) + + newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner table`, + `RESTORE TABLE seq FROM $1`, backupLoc) + newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc) + + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq") + require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore") + }) + + // Ownership dependencies should be preserved and remapped when restoring + // both the owned sequence and owning table into a different database. + t.Run("test restoring all tables into a different database", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.Exec(t, `CREATE DATABASE restore_db`) + newDB.Exec(t, `RESTORE d.* FROM $1 WITH into_db='restore_db'`, backupLoc) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "t") + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "seq") + + require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner after restore", + ) + require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner after restore", + ) + require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d.t after restore", + ) + require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d.t after restore", + ) + }) + + // Setup for cross-database ownership backup-restore tests. + backupLocD2D3 := LocalFoo + `/d2d3` + + origDB.Exec(t, `CREATE DATABASE d2`) + origDB.Exec(t, `CREATE TABLE d2.t(a int)`) + + origDB.Exec(t, `CREATE DATABASE d3`) + origDB.Exec(t, `CREATE TABLE d3.t(a int)`) + + origDB.Exec(t, `CREATE SEQUENCE d2.seq OWNED BY d3.t.a`) + + origDB.Exec(t, `CREATE SEQUENCE d3.seq OWNED BY d2.t.a`) + origDB.Exec(t, `CREATE SEQUENCE d3.seq2 OWNED BY d3.t.a`) + + origDB.Exec(t, `BACKUP DATABASE d2, d3 TO $1`, backupLocD2D3) + + // When restoring a database that has a sequence which is owned by a table + // in another database, the user must supply the + // `skip_missing_sequence_owners` flag. When supplied, the cross-database + // ownership dependency should be removed. + t.Run("test restoring two databases removes cross-database ownership dependency", + func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.ExpectErr(t, "pq: cannot restore sequence \"seq\" without referenced owner|"+ + "pq: cannot restore table \"t\" without referenced sequence", + `RESTORE DATABASE d2 FROM $1`, backupLocD2D3) + newDB.Exec(t, `RESTORE DATABASE d2 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t") + require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "expected restored table to own no sequences.", + ) + + newDB.ExpectErr(t, "pq: cannot restore sequence \"seq\" without referenced owner|"+ + "pq: cannot restore table \"t\" without referenced sequence", + `RESTORE DATABASE d3 FROM $1`, backupLocD2D3) + newDB.Exec(t, `RESTORE DATABASE d3 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3) + + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq") + require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore") + + // Sequence dependencies inside the database should still be preserved. + sd := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2") + td := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "t") + + require.True(t, sd.SequenceOpts.HasOwner(), "no owner found for seq2") + require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table owner for sequence seq2 after restore", + ) + require.Equal(t, td.GetColumns()[0].ID, sd.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column owner for sequence seq2 after restore") + require.Equal(t, 1, len(td.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d3.t after restore", + ) + require.Equal(t, sd.ID, td.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequences owned by d3.t", + ) + }) + + // When restoring both the databases that contain a cross database ownership + // dependency, we should preserve and remap the ownership dependencies. + t.Run("test restoring both databases at the same time", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.Exec(t, `RESTORE DATABASE d2, d3 FROM $1`, backupLocD2D3) + + // d2.t owns d3.seq should be preserved. + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t") + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq") + + require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner after restore", + ) + require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner after restore", + ) + require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d.t after restore", + ) + require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d.t after restore", + ) + + // d3.t owns d2.seq and d3.seq2 should be preserved. + td := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "t") + sd := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "seq") + sdSeq2 := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2") + + require.True(t, sd.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.True(t, sdSeq2.SequenceOpts.HasOwner(), "no sequence owner after restore") + + require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner of d3.seq after restore", + ) + require.Equal(t, td.ID, sdSeq2.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner of d3.seq2 after restore", + ) + + require.Equal(t, td.GetColumns()[0].ID, sd.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner of d2.seq after restore", + ) + require.Equal(t, td.GetColumns()[0].ID, sdSeq2.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner of d3.seq2 after restore", + ) + + require.Equal(t, 2, len(td.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d3.t after restore", + ) + require.Equal(t, sd.ID, td.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d3.t after restore", + ) + require.Equal(t, sdSeq2.ID, td.GetColumns()[0].OwnsSequenceIds[1], + "unexpected ID of sequence owned by table d3.t after restore", + ) + }) +} + func TestBackupRestoreShowJob(t *testing.T) { defer leaktest.AfterTest(t)() diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index a901325df3c9..54c4cc74271b 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -42,10 +42,11 @@ import ( type DescRewriteMap map[sqlbase.ID]*jobspb.RestoreDetails_DescriptorRewrite const ( - restoreOptIntoDB = "into_db" - restoreOptSkipMissingFKs = "skip_missing_foreign_keys" - restoreOptSkipMissingSequences = "skip_missing_sequences" - restoreOptSkipMissingViews = "skip_missing_views" + restoreOptIntoDB = "into_db" + restoreOptSkipMissingFKs = "skip_missing_foreign_keys" + restoreOptSkipMissingSequences = "skip_missing_sequences" + restoreOptSkipMissingSequenceOwners = "skip_missing_sequence_owners" + restoreOptSkipMissingViews = "skip_missing_views" // The temporary database system tables will be restored into for full // cluster backups. @@ -53,11 +54,12 @@ const ( ) var restoreOptionExpectValues = map[string]sql.KVStringOptValidate{ - restoreOptIntoDB: sql.KVStringOptRequireValue, - restoreOptSkipMissingFKs: sql.KVStringOptRequireNoValue, - restoreOptSkipMissingSequences: sql.KVStringOptRequireNoValue, - restoreOptSkipMissingViews: sql.KVStringOptRequireNoValue, - backupOptEncPassphrase: sql.KVStringOptRequireValue, + restoreOptIntoDB: sql.KVStringOptRequireValue, + restoreOptSkipMissingFKs: sql.KVStringOptRequireNoValue, + restoreOptSkipMissingSequences: sql.KVStringOptRequireNoValue, + restoreOptSkipMissingSequenceOwners: sql.KVStringOptRequireNoValue, + restoreOptSkipMissingViews: sql.KVStringOptRequireNoValue, + backupOptEncPassphrase: sql.KVStringOptRequireValue, } // rewriteViewQueryDBNames rewrites the passed table's ViewQuery replacing all @@ -218,6 +220,29 @@ func allocateDescriptorRewrites( } } } + for _, seqID := range col.OwnsSequenceIds { + if _, ok := tablesByID[seqID]; !ok { + if _, ok := opts[restoreOptSkipMissingSequenceOwners]; !ok { + return nil, errors.Errorf( + "cannot restore table %q without referenced sequence %d (or %q option)", + table.Name, seqID, restoreOptSkipMissingSequenceOwners) + } + } + } + } + + // Handle sequence ownership dependencies. + if table.IsSequence() && table.SequenceOpts.HasOwner() { + if _, ok := tablesByID[table.SequenceOpts.SequenceOwner.OwnerTableID]; !ok { + if _, ok := opts[restoreOptSkipMissingSequenceOwners]; !ok { + return nil, errors.Errorf( + "cannot restore sequence %q without referenced owner table %d (or %q option)", + table.Name, + table.SequenceOpts.SequenceOwner.OwnerTableID, + restoreOptSkipMissingSequenceOwners, + ) + } + } } } @@ -765,25 +790,51 @@ func RewriteTableDescs( } } + if table.IsSequence() && table.SequenceOpts.HasOwner() { + if ownerRewrite, ok := descriptorRewrites[table.SequenceOpts.SequenceOwner.OwnerTableID]; ok { + table.SequenceOpts.SequenceOwner.OwnerTableID = ownerRewrite.ID + } else { + // The sequence's owner table is not being restored, thus we simply + // remove the ownership dependency. To get here, the user must have + // specified 'skip_missing_sequence_owners', otherwise we would have + // errored out in allocateDescriptorRewrites. + table.SequenceOpts.SequenceOwner = sqlbase.TableDescriptor_SequenceOpts_SequenceOwner{} + } + } + // rewriteCol is a closure that performs the ID rewrite logic on a column. rewriteCol := func(col *sqlbase.ColumnDescriptor) error { // Rewrite the types.T's IDs present in the column. rewriteIDsInTypesT(col.Type, descriptorRewrites) - var newSeqRefs []sqlbase.ID + var newUsedSeqRefs []sqlbase.ID for _, seqID := range col.UsesSequenceIds { if rewrite, ok := descriptorRewrites[seqID]; ok { - newSeqRefs = append(newSeqRefs, rewrite.ID) + newUsedSeqRefs = append(newUsedSeqRefs, rewrite.ID) } else { // The referenced sequence isn't being restored. // Strip the DEFAULT expression and sequence references. // To get here, the user must have specified 'skip_missing_sequences' -- // otherwise, would have errored out in allocateDescriptorRewrites. - newSeqRefs = []sqlbase.ID{} + newUsedSeqRefs = []sqlbase.ID{} col.DefaultExpr = nil break } } - col.UsesSequenceIds = newSeqRefs + col.UsesSequenceIds = newUsedSeqRefs + + var newOwnedSeqRefs []sqlbase.ID + for _, seqID := range col.OwnsSequenceIds { + // We only add the sequence ownership dependency if the owned sequence + // is being restored. + // If the owned sequence is not being restored, the user must have + // specified 'skip_missing_sequence_owners' to get here, otherwise + // we would have errored out in allocateDescriptorRewrites. + if rewrite, ok := descriptorRewrites[seqID]; ok { + newOwnedSeqRefs = append(newOwnedSeqRefs, rewrite.ID) + } + } + col.OwnsSequenceIds = newOwnedSeqRefs + return nil } diff --git a/pkg/cmd/roachtest/decommission.go b/pkg/cmd/roachtest/decommission.go index 602d0c8c1cd8..a8513e1d2328 100644 --- a/pkg/cmd/roachtest/decommission.go +++ b/pkg/cmd/roachtest/decommission.go @@ -444,7 +444,7 @@ func runDecommissionAcceptance(ctx context.Context, t *test, c *cluster) { // Until 20.2, --self is not supported. // TODO(knz): Remove this alternative when roachtest does not // test any version lower than 20.2. - if strings.Contains(cmdOutput, "ERROR: unknown flag: --self") { + if strings.Contains(cmdOutput, ": unknown flag: --self") { t.l.Printf("--self not supported; skipping recommission with --self") selfFlagSupported = false } else { @@ -710,6 +710,12 @@ WHERE "eventType" IN ($1, $2) ORDER BY timestamp`, {"node_decommissioned", "1"}, } + if !selfFlagSupported { + // If `--self` is not supported then we don't expect to see + // node 1 self decommission and recommission (step 3 and 4). + expMatrix = append(expMatrix[:2], expMatrix[4:]...) + } + if !reflect.DeepEqual(matrix, expMatrix) { t.Fatalf("unexpected diff(matrix, expMatrix):\n%s\n%s\nvs.\n%s", pretty.Diff(matrix, expMatrix), matrix, expMatrix) } diff --git a/pkg/sql/sem/builtins/all_builtins_test.go b/pkg/sql/sem/builtins/all_builtins_test.go index 25d968e2d093..57a3178f55ab 100644 --- a/pkg/sql/sem/builtins/all_builtins_test.go +++ b/pkg/sql/sem/builtins/all_builtins_test.go @@ -140,10 +140,10 @@ func TestOverloadsVolatilityMatchesPostgres(t *testing.T) { // Check each builtin against Postgres. for name, builtin := range builtins { - if builtin.props.IgnoreVolatilityCheck { - continue - } for idx, overload := range builtin.overloads { + if overload.IgnoreVolatilityCheck { + continue + } postgresVolatility, found := findOverloadVolatility(name, overload) if !found { continue diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index a01cef2e2eb7..ff47909f9e78 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -249,11 +249,6 @@ var builtins = map[string]builtinDefinition{ "concat": makeBuiltin( tree.FunctionProperties{ NullableArgs: true, - // In Postgres concat can take any arguments, converting them to their - // text representation. Since the text representation can depend on the - // context (e.g. timezone), the function is Stable. In our case, we only - // take String inputs so our version is Immutable. - IgnoreVolatilityCheck: true, }, tree.Overload{ Types: tree.VariadicType{VarType: types.String}, @@ -275,17 +270,17 @@ var builtins = map[string]builtinDefinition{ }, Info: "Concatenates a comma-separated list of strings.", Volatility: tree.VolatilityImmutable, + // In Postgres concat can take any arguments, converting them to + // their text representation. Since the text representation can + // depend on the context (e.g. timezone), the function is Stable. In + // our case, we only take String inputs so our version is Immutable. + IgnoreVolatilityCheck: true, }, ), "concat_ws": makeBuiltin( tree.FunctionProperties{ NullableArgs: true, - // In Postgres concat_ws can take any arguments, converting them to their - // text representation. Since the text representation can depend on the - // context (e.g. timezone), the function is Stable. In our case, we only - // take String inputs so our version is Immutable. - IgnoreVolatilityCheck: true, }, tree.Overload{ Types: tree.VariadicType{VarType: types.String}, @@ -321,11 +316,16 @@ var builtins = map[string]builtinDefinition{ "subsequent arguments. \n\nFor example `concat_ws('!','wow','great')` " + "returns `wow!great`.", Volatility: tree.VolatilityImmutable, + // In Postgres concat_ws can take any arguments, converting them to + // their text representation. Since the text representation can + // depend on the context (e.g. timezone), the function is Stable. In + // our case, we only take String inputs so our version is Immutable. + IgnoreVolatilityCheck: true, }, ), // https://www.postgresql.org/docs/10/static/functions-string.html#FUNCTIONS-STRING-OTHER - "convert_from": makeBuiltin(tree.FunctionProperties{Category: categoryString, IgnoreVolatilityCheck: true}, + "convert_from": makeBuiltin(tree.FunctionProperties{Category: categoryString}, tree.Overload{ Types: tree.ArgTypes{{"str", types.Bytes}, {"enc", types.String}}, ReturnType: tree.FixedReturnType(types.String), @@ -353,11 +353,12 @@ var builtins = map[string]builtinDefinition{ }, Info: "Decode the bytes in `str` into a string using encoding `enc`. " + "Supports encodings 'UTF8' and 'LATIN1'.", - Volatility: tree.VolatilityImmutable, + Volatility: tree.VolatilityImmutable, + IgnoreVolatilityCheck: true, }), // https://www.postgresql.org/docs/10/static/functions-string.html#FUNCTIONS-STRING-OTHER - "convert_to": makeBuiltin(tree.FunctionProperties{Category: categoryString, IgnoreVolatilityCheck: true}, + "convert_to": makeBuiltin(tree.FunctionProperties{Category: categoryString}, tree.Overload{ Types: tree.ArgTypes{{"str", types.String}, {"enc", types.String}}, ReturnType: tree.FixedReturnType(types.Bytes), @@ -385,7 +386,8 @@ var builtins = map[string]builtinDefinition{ }, Info: "Encode the string `str` as a byte array using encoding `enc`. " + "Supports encodings 'UTF8' and 'LATIN1'.", - Volatility: tree.VolatilityImmutable, + Volatility: tree.VolatilityImmutable, + IgnoreVolatilityCheck: true, }), // https://www.postgresql.org/docs/9.0/functions-binarystring.html#FUNCTIONS-BINARYSTRING-OTHER @@ -2498,10 +2500,11 @@ may increase either contention or retry errors, or both.`, durationDelta := time.Duration(-beforeOffsetSecs) * time.Second return tree.NewDTimeTZ(timetz.MakeTimeTZFromTime(tTime.In(loc).Add(durationDelta))), nil }, - Info: "Treat given time without time zone as located in the specified time zone.", - // TODO(mgartner): This overload might be stable, not volatile. - // See: https://github.com/cockroachdb/cockroach/pull/48756#issuecomment-627672686 - Volatility: tree.VolatilityVolatile, + Info: "Treat given time without time zone as located in the specified time zone.", + Volatility: tree.VolatilityStable, + // This overload's volatility does not match Postgres. See + // https://github.com/cockroachdb/cockroach/pull/51037 for details. + IgnoreVolatilityCheck: true, }, tree.Overload{ Types: tree.ArgTypes{ @@ -2523,10 +2526,11 @@ may increase either contention or retry errors, or both.`, tTime := tArg.TimeTZ.ToTime() return tree.NewDTimeTZ(timetz.MakeTimeTZFromTime(tTime.In(loc))), nil }, - Info: "Convert given time with time zone to the new time zone.", - // TODO(mgartner): This overload might be stable, not volatile. - // See: https://github.com/cockroachdb/cockroach/pull/48756#issuecomment-627672686 - Volatility: tree.VolatilityVolatile, + Info: "Convert given time with time zone to the new time zone.", + Volatility: tree.VolatilityStable, + // This overload's volatility does not match Postgres. See + // https://github.com/cockroachdb/cockroach/pull/51037 for details. + IgnoreVolatilityCheck: true, }, ), diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 8b3307eedb16..527f5f839306 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -138,9 +138,6 @@ func makeTypeIOBuiltin(argTypes tree.TypeList, returnType *types.T) builtinDefin return builtinDefinition{ props: tree.FunctionProperties{ Category: categoryCompatibility, - // Ignore validity checks for typeio builtins. - // We don't implement these anyway, and they are very hard to special case. - IgnoreVolatilityCheck: true, }, overloads: []tree.Overload{ { @@ -151,6 +148,10 @@ func makeTypeIOBuiltin(argTypes tree.TypeList, returnType *types.T) builtinDefin }, Info: notUsableInfo, Volatility: tree.VolatilityVolatile, + // Ignore validity checks for typeio builtins. We don't + // implement these anyway, and they are very hard to special + // case. + IgnoreVolatilityCheck: true, }, }, } diff --git a/pkg/sql/sem/tree/function_definition.go b/pkg/sql/sem/tree/function_definition.go index e715d9d75bb2..2cc0f790453b 100644 --- a/pkg/sql/sem/tree/function_definition.go +++ b/pkg/sql/sem/tree/function_definition.go @@ -84,11 +84,6 @@ type FunctionProperties struct { // with the FmtParsable directive. AmbiguousReturnType bool - // IgnoreVolatilityCheck ignores checking the functions overloads against - // the Postgres ones at test time. - // This should be used with caution. - IgnoreVolatilityCheck bool - // HasSequenceArguments is true if the builtin function takes in a sequence // name (string) and can be used in a scalar expression. // TODO(richardjcai): When implicit casting is supported, these builtins diff --git a/pkg/sql/sem/tree/overload.go b/pkg/sql/sem/tree/overload.go index 6a5fb979e931..4718beb4c4ff 100644 --- a/pkg/sql/sem/tree/overload.go +++ b/pkg/sql/sem/tree/overload.go @@ -80,6 +80,11 @@ type Overload struct { // SpecializedVecBuiltin is used to let the vectorized engine // know when an Overload has a specialized vectorized operator. SpecializedVecBuiltin SpecializedVectorizedBuiltin + + // IgnoreVolatilityCheck ignores checking the functions overload's + // volatility against Postgres's volatility at test time. + // This should be used with caution. + IgnoreVolatilityCheck bool } // params implements the overloadImpl interface. diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 1c17e2e54d03..c78cee2a9db6 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -331,7 +331,7 @@ func removeSequenceOwnerIfExists( sequenceID sqlbase.ID, opts *sqlbase.TableDescriptor_SequenceOpts, ) error { - if opts.SequenceOwner.Equal(sqlbase.TableDescriptor_SequenceOpts_SequenceOwner{}) { + if !opts.HasOwner() { return nil } tableDesc, err := p.Tables().GetMutableTableVersionByID(ctx, opts.SequenceOwner.OwnerTableID, p.txn) diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index e6a75d1c5ca4..7c33c41e8c10 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -4547,3 +4547,8 @@ func (desc ColumnDescriptor) GetLogicalColumnID() ColumnID { return desc.ID } + +// HasOwner returns true if the sequence options indicate an owner exists. +func (opts *TableDescriptor_SequenceOpts) HasOwner() bool { + return !opts.SequenceOwner.Equal(TableDescriptor_SequenceOpts_SequenceOwner{}) +} diff --git a/pkg/testutils/emptypkg/emptypkg.go b/pkg/testutils/emptypkg/emptypkg.go deleted file mode 100644 index a5bfde5d9e75..000000000000 --- a/pkg/testutils/emptypkg/emptypkg.go +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright 2020 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -// Package emptypkg is empty. This is useful as a workaround, see build/teamcity-acceptance.sh for -// an example. -package emptypkg diff --git a/pkg/testutils/emptypkg/emptypkg_test.go b/pkg/testutils/emptypkg/emptypkg_test.go deleted file mode 100644 index 5e913dbd3ec6..000000000000 --- a/pkg/testutils/emptypkg/emptypkg_test.go +++ /dev/null @@ -1,11 +0,0 @@ -// Copyright 2020 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package emptypkg