Merge #50949 #51005 #51037 #51071

50949: backupccl: handle sequence ownership remapping logic for restore r=pbardea a=arulajmani

Previously, we would restore sequence descriptors/table descriptors as
is, without remapping the ownership dependency to the new IDs. This
meant sequence ownership was broken post restore. This PR fixes that.

When sequences with owners or tables that have columns that own
sequences are restored:
- If both the owned sequence and the owning table are being restored,
the ownership is remapped.
- If just the sequence is restored, the user receives an error to use
the `skip_missing_sequence_owners` flag. If the flag is provided, the
sequence is restored without any owner.
- If just the table is restored, the user receives an error to use the
`skip_missing_sequence_owners` flag. If the flag is provided, the table
is restored with the table column that previously owned the sequence no
longer owning that sequence.

Fixes #50781

Release note (enterprise change): Restore now has a new option
`skip_missing_sequence_owners` that must be supplied when restoring
only the table/sequence that was previously
a sequence owner/owned by a table.
Additionally, this fixes a bug where ownership relationships would not
be remapped after a restore.

51005: build: fix package reported by acceptance and compose r=otan a=tbg

The acceptance and compose tests were previously reporting `emptypkg` as
their package, which would be used to create issues.

We need to pass the "real" package to go so that it shows up in the
JSON output.

Closes #51003.

Release note: None

51037: sql: change volatility of timezone with TIME and TIMETZ to stable r=mgartner a=mgartner

sql: change volatility of timezone with TIME and TIMETZ to stable

The `timezone(STRING, TIME)` and `timezone(STRING, TIMETZ)` function
overloads were originally marked as volatile to copy Postgres's
volatility for the `timezone(STRING, TIMETZ)` overload (Postgres does
not implement `timezone (STRING, TIME)`.

Postgres's volatility settings for all overloads of `timezone` are
below.

       Name   |          Argument data types          | Volatility
    ----------+---------------------------------------+------------------
     timezone | interval, time with time zone         | immutable   zone
     timezone | interval, timestamp with time zone    | immutable
     timezone | interval, timestamp without time zone | immutable
     timezone | text, time with time zone             | volatile    zone
     timezone | text, timestamp with time zone        | immutable
     timezone | text, timestamp without time zone     | immutable

This single overload is singled out as volatile by Postgres because it
is dependent on C's `time(NULL)`. See the Postgres commit originally
marking this overload volatility, and the lines of code showing the
dependence:

postgres/postgres@35979e6
https://github.com/postgres/postgres/blob/ac3a4866c0bf1d7a14009f18d3b42ffcb063a7e9/src/backend/utils/adt/date.c#L2816

CRDB does not have the same issue. All `timezone` overloads have the
same volatility, stable. This commit marks them as such.

Additional context: #48756 (comment)

This commit also moves the `IgnoreVolatilityCheck` field from function
definitions to overloads. This allows overloads of the same function to
be ignored when asserting that the volalitity of an overload matches
Postgres's assigned volatility.

Release note: None


51071: roachtest: fix acceptance/decommission r=darinpp a=darinpp

The added --self flag makes the test fail with 20.1 and 19.2

With 20.1 the flag is not supported and so the expected
result should reflect that the step where a node decommissions self
will not happen.
With 19.2 the match that --self is not supported doesn't work
as the error message capitalization differs.

The fix changes the expected result and corrects the match of --self not
supported to work with 19.2 as well

Release note: None

Co-authored-by: arulajmani <[email protected]>
Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Darin <[email protected]>

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"

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"

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