Skip to content

Commit

Permalink
Merge #59495
Browse files Browse the repository at this point in the history 
59495: sql: use catalog.TableDescriptor instead of tabledesc.Immutable r=postamar a=postamar

This PR makes the tabledesc.Immutable type private and makes use of the
catalog.TableDescriptor interface instead.

Fixes #59486.

Release note: None 

Co-authored-by: Marius Posta <[email protected]>
  • Loading branch information
craig[bot] and Marius Posta committed Feb 2, 2021
2 parents 43a75bb + b5a36df commit e854509
Show file tree
Hide file tree
Showing 175 changed files with 1,375 additions and 1,293 deletions.
88 changes: 44 additions & 44 deletions pkg/ccl/backupccl/backup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
Expand Down Expand Up @@ -1570,7 +1571,7 @@ func TestBackupRestoreResume(t *testing.T) {
t.Fatal(err)
}
createAndWaitForJob(
t, sqlDB, []descpb.ID{backupTableDesc.ID},
t, sqlDB, []descpb.ID{backupTableDesc.GetID()},
jobspb.BackupDetails{
EndTime: tc.Servers[0].Clock().Now(),
URI: "nodelocal://0/backup",
Expand Down Expand Up @@ -1619,7 +1620,7 @@ func TestBackupRestoreResume(t *testing.T) {
t, sqlDB, []descpb.ID{restoreTableID},
jobspb.RestoreDetails{
DescriptorRewrites: map[descpb.ID]*jobspb.RestoreDetails_DescriptorRewrite{
backupTableDesc.ID: {
backupTableDesc.GetID(): {
ParentID: descpb.ID(restoreDatabaseID),
ID: restoreTableID,
},
Expand Down Expand Up @@ -5211,17 +5212,17 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
tableDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t")
seqDesc := catalogkv.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,
require.True(t, seqDesc.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.Equal(t, tableDesc.GetID(), seqDesc.GetSequenceOpts().SequenceOwner.OwnerTableID,
"unexpected table is sequence owner after restore",
)
require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID,
require.Equal(t, tableDesc.GetPublicColumns()[0].ID, seqDesc.GetSequenceOpts().SequenceOwner.OwnerColumnID,
"unexpected column is sequence owner after restore",
)
require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds),
require.Equal(t, 1, len(tableDesc.GetPublicColumns()[0].OwnsSequenceIds),
"unexpected number of sequences owned by d.t after restore",
)
require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0],
require.Equal(t, seqDesc.GetID(), tableDesc.GetPublicColumns()[0].OwnsSequenceIds[0],
"unexpected ID of sequence owned by table d.t after restore",
)
})
Expand All @@ -5242,7 +5243,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {

newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc)
seqDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq")
require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected owner of restored sequence.")
require.False(t, seqDesc.GetSequenceOpts().HasOwner(), "unexpected owner of restored sequence.")
})

// When just the table is restored by itself, the ownership dependency is
Expand All @@ -5268,7 +5269,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {

tableDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t")

require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds),
require.Equal(t, 0, len(tableDesc.GetPublicColumns()[0].OwnsSequenceIds),
"expected restored table to own 0 sequences",
)

Expand All @@ -5277,7 +5278,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc)

seqDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq")
require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore")
require.False(t, seqDesc.GetSequenceOpts().HasOwner(), "unexpected sequence owner after restore")
})

// Ownership dependencies should be preserved and remapped when restoring
Expand All @@ -5295,17 +5296,17 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
tableDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "t")
seqDesc := catalogkv.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,
require.True(t, seqDesc.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.Equal(t, tableDesc.GetID(), seqDesc.GetSequenceOpts().SequenceOwner.OwnerTableID,
"unexpected table is sequence owner after restore",
)
require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID,
require.Equal(t, tableDesc.GetPublicColumns()[0].ID, seqDesc.GetSequenceOpts().SequenceOwner.OwnerColumnID,
"unexpected column is sequence owner after restore",
)
require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds),
require.Equal(t, 1, len(tableDesc.GetPublicColumns()[0].OwnsSequenceIds),
"unexpected number of sequences owned by d.t after restore",
)
require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0],
require.Equal(t, seqDesc.GetID(), tableDesc.GetPublicColumns()[0].OwnsSequenceIds[0],
"unexpected ID of sequence owned by table d.t after restore",
)
})
Expand Down Expand Up @@ -5344,7 +5345,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
newDB.Exec(t, `RESTORE DATABASE d2 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3)

tableDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t")
require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds),
require.Equal(t, 0, len(tableDesc.GetPublicColumns()[0].OwnsSequenceIds),
"expected restored table to own no sequences.",
)

Expand All @@ -5354,22 +5355,22 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
newDB.Exec(t, `RESTORE DATABASE d3 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3)

seqDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq")
require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore")
require.False(t, seqDesc.GetSequenceOpts().HasOwner(), "unexpected sequence owner after restore")

// Sequence dependencies inside the database should still be preserved.
sd := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2")
td := catalogkv.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,
require.True(t, sd.GetSequenceOpts().HasOwner(), "no owner found for seq2")
require.Equal(t, td.GetID(), sd.GetSequenceOpts().SequenceOwner.OwnerTableID,
"unexpected table owner for sequence seq2 after restore",
)
require.Equal(t, td.GetColumns()[0].ID, sd.SequenceOpts.SequenceOwner.OwnerColumnID,
require.Equal(t, td.GetPublicColumns()[0].ID, sd.GetSequenceOpts().SequenceOwner.OwnerColumnID,
"unexpected column owner for sequence seq2 after restore")
require.Equal(t, 1, len(td.GetColumns()[0].OwnsSequenceIds),
require.Equal(t, 1, len(td.GetPublicColumns()[0].OwnsSequenceIds),
"unexpected number of sequences owned by d3.t after restore",
)
require.Equal(t, sd.ID, td.GetColumns()[0].OwnsSequenceIds[0],
require.Equal(t, sd.GetID(), td.GetPublicColumns()[0].OwnsSequenceIds[0],
"unexpected ID of sequences owned by d3.t",
)
})
Expand All @@ -5389,17 +5390,17 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
tableDesc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t")
seqDesc := catalogkv.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,
require.True(t, seqDesc.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.Equal(t, tableDesc.GetID(), seqDesc.GetSequenceOpts().SequenceOwner.OwnerTableID,
"unexpected table is sequence owner after restore",
)
require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID,
require.Equal(t, tableDesc.GetPublicColumns()[0].ID, seqDesc.GetSequenceOpts().SequenceOwner.OwnerColumnID,
"unexpected column is sequence owner after restore",
)
require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds),
require.Equal(t, 1, len(tableDesc.GetPublicColumns()[0].OwnsSequenceIds),
"unexpected number of sequences owned by d.t after restore",
)
require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0],
require.Equal(t, seqDesc.GetID(), tableDesc.GetPublicColumns()[0].OwnsSequenceIds[0],
"unexpected ID of sequence owned by table d.t after restore",
)

Expand All @@ -5408,30 +5409,30 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
sd := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "seq")
sdSeq2 := catalogkv.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.True(t, sd.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.True(t, sdSeq2.GetSequenceOpts().HasOwner(), "no sequence owner after restore")

require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID,
require.Equal(t, td.GetID(), sd.GetSequenceOpts().SequenceOwner.OwnerTableID,
"unexpected table is sequence owner of d3.seq after restore",
)
require.Equal(t, td.ID, sdSeq2.SequenceOpts.SequenceOwner.OwnerTableID,
require.Equal(t, td.GetID(), sdSeq2.GetSequenceOpts().SequenceOwner.OwnerTableID,
"unexpected table is sequence owner of d3.seq2 after restore",
)

require.Equal(t, td.GetColumns()[0].ID, sd.SequenceOpts.SequenceOwner.OwnerColumnID,
require.Equal(t, td.GetPublicColumns()[0].ID, sd.GetSequenceOpts().SequenceOwner.OwnerColumnID,
"unexpected column is sequence owner of d2.seq after restore",
)
require.Equal(t, td.GetColumns()[0].ID, sdSeq2.SequenceOpts.SequenceOwner.OwnerColumnID,
require.Equal(t, td.GetPublicColumns()[0].ID, sdSeq2.GetSequenceOpts().SequenceOwner.OwnerColumnID,
"unexpected column is sequence owner of d3.seq2 after restore",
)

require.Equal(t, 2, len(td.GetColumns()[0].OwnsSequenceIds),
require.Equal(t, 2, len(td.GetPublicColumns()[0].OwnsSequenceIds),
"unexpected number of sequences owned by d3.t after restore",
)
require.Equal(t, sd.ID, td.GetColumns()[0].OwnsSequenceIds[0],
require.Equal(t, sd.GetID(), td.GetPublicColumns()[0].OwnsSequenceIds[0],
"unexpected ID of sequence owned by table d3.t after restore",
)
require.Equal(t, sdSeq2.ID, td.GetColumns()[0].OwnsSequenceIds[1],
require.Equal(t, sdSeq2.GetID(), td.GetPublicColumns()[0].OwnsSequenceIds[1],
"unexpected ID of sequence owned by table d3.t after restore",
)
})
Expand Down Expand Up @@ -5885,14 +5886,13 @@ func getMockIndexDesc(indexID descpb.IndexID) descpb.IndexDescriptor {

func getMockTableDesc(
tableID descpb.ID, pkIndex descpb.IndexDescriptor, indexes []descpb.IndexDescriptor,
) tabledesc.Immutable {
) catalog.TableDescriptor {
mockTableDescriptor := descpb.TableDescriptor{
ID: tableID,
PrimaryIndex: pkIndex,
Indexes: indexes,
}
mockImmutableTableDesc := tabledesc.MakeImmutable(mockTableDescriptor)
return mockImmutableTableDesc
return tabledesc.NewImmutable(mockTableDescriptor)
}

// Unit tests for the getLogicallyMergedTableSpans() method.
Expand Down Expand Up @@ -5986,7 +5986,7 @@ func TestLogicallyMergedTableSpans(t *testing.T) {
for _, test := range testCases {
t.Run(test.name, func(t *testing.T) {
tableDesc := getMockTableDesc(test.tableID, test.pkIndex, test.indexes)
spans, err := getLogicallyMergedTableSpans(&tableDesc, unusedMap, codec,
spans, err := getLogicallyMergedTableSpans(tableDesc, unusedMap, codec,
hlc.Timestamp{}, test.checkForKVInBoundsOverride)
var mergedSpans []string
for _, span := range spans {
Expand Down Expand Up @@ -6673,7 +6673,7 @@ ALTER TYPE sc.typ ADD VALUE 'hi';
require.EqualValues(t, 2, schemaDesc.Version)

tableDesc := catalogkv.TestingGetTableDescriptorFromSchema(kvDB, keys.SystemSQLCodec, "d", "sc", "tb")
require.EqualValues(t, 2, tableDesc.Version)
require.EqualValues(t, 2, tableDesc.GetVersion())

typeDesc := catalogkv.TestingGetTypeDescriptorFromSchema(kvDB, keys.SystemSQLCodec, "d", "sc", "typ")
require.EqualValues(t, 2, typeDesc.Version)
Expand Down Expand Up @@ -6768,7 +6768,7 @@ CREATE TYPE sc.typ AS ENUM ('hello');
require.Equal(t, descpb.DescriptorState_OFFLINE, schemaDesc.State)

tableDesc := catalogkv.TestingGetTableDescriptorFromSchema(kvDB, keys.SystemSQLCodec, "d", "sc", "tb")
require.Equal(t, descpb.DescriptorState_OFFLINE, tableDesc.State)
require.Equal(t, descpb.DescriptorState_OFFLINE, tableDesc.GetState())

typeDesc := catalogkv.TestingGetTypeDescriptorFromSchema(kvDB, keys.SystemSQLCodec, "d", "sc", "typ")
require.Equal(t, descpb.DescriptorState_OFFLINE, typeDesc.State)
Expand Down Expand Up @@ -6862,10 +6862,10 @@ CREATE TYPE sc.typ AS ENUM ('hello');
require.Equal(t, descpb.DescriptorState_OFFLINE, schemaDesc.State)

publicTableDesc := catalogkv.TestingGetTableDescriptorFromSchema(kvDB, keys.SystemSQLCodec, "newdb", "public", "tb")
require.Equal(t, descpb.DescriptorState_OFFLINE, publicTableDesc.State)
require.Equal(t, descpb.DescriptorState_OFFLINE, publicTableDesc.GetState())

scTableDesc := catalogkv.TestingGetTableDescriptorFromSchema(kvDB, keys.SystemSQLCodec, "newdb", "sc", "tb")
require.Equal(t, descpb.DescriptorState_OFFLINE, scTableDesc.State)
require.Equal(t, descpb.DescriptorState_OFFLINE, scTableDesc.GetState())

typeDesc := catalogkv.TestingGetTypeDescriptorFromSchema(kvDB, keys.SystemSQLCodec, "newdb", "sc", "typ")
require.Equal(t, descpb.DescriptorState_OFFLINE, typeDesc.State)
Expand Down
24 changes: 12 additions & 12 deletions pkg/ccl/backupccl/full_cluster_backup_restore_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,28 +167,28 @@ CREATE TABLE data2.foo (a int);
// Note the absence of the jobs table. Jobs are tested by another test as
// jobs are created during the RESTORE process.
systemTablesToVerify := []string{
systemschema.CommentsTable.Name,
systemschema.LocationsTable.Name,
systemschema.RoleMembersTable.Name,
systemschema.RoleOptionsTable.Name,
systemschema.SettingsTable.Name,
systemschema.TableStatisticsTable.Name,
systemschema.UITable.Name,
systemschema.UsersTable.Name,
systemschema.ZonesTable.Name,
systemschema.ScheduledJobsTable.Name,
systemschema.CommentsTable.GetName(),
systemschema.LocationsTable.GetName(),
systemschema.RoleMembersTable.GetName(),
systemschema.RoleOptionsTable.GetName(),
systemschema.SettingsTable.GetName(),
systemschema.TableStatisticsTable.GetName(),
systemschema.UITable.GetName(),
systemschema.UsersTable.GetName(),
systemschema.ZonesTable.GetName(),
systemschema.ScheduledJobsTable.GetName(),
}

verificationQueries := make([]string, len(systemTablesToVerify))
// Populate the list of tables we expect to be restored as well as queries
// that can be used to ensure that data in those tables is restored.
for i, table := range systemTablesToVerify {
switch table {
case systemschema.TableStatisticsTable.Name:
case systemschema.TableStatisticsTable.GetName():
// createdAt and statisticsID are re-generated on RESTORE.
query := `SELECT "tableID", name, "columnIDs", "rowCount" FROM system.table_statistics`
verificationQueries[i] = query
case systemschema.SettingsTable.Name:
case systemschema.SettingsTable.GetName():
// We don't include the cluster version.
query := fmt.Sprintf("SELECT * FROM system.%s WHERE name <> 'version'", table)
verificationQueries[i] = query
Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/backupccl/restore_job.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ func spansForAllRestoreTableIndexes(
if rawTbl != nil && rawTbl.State != descpb.DescriptorState_DROP {
tbl := tabledesc.NewImmutable(*rawTbl)
for _, idx := range tbl.NonDropIndexes() {
key := tableAndIndex{tableID: tbl.ID, indexID: idx.GetID()}
key := tableAndIndex{tableID: tbl.GetID(), indexID: idx.GetID()}
if !added[key] {
if err := sstIntervalTree.Insert(intervalSpan(tbl.IndexSpan(codec, idx.GetID())), false); err != nil {
panic(errors.NewAssertionErrorWithWrappedErrf(err, "IndexSpan"))
Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/backupccl/show_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ ORDER BY object_type, object_name`, full)
// Create tables with the same ID as data.tableA to ensure that comments
// from different tables in the restoring cluster don't appear.
tableA := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "data", "tablea")
for i := keys.MinUserDescID; i < int(tableA.ID); i++ {
for i := keys.MinUserDescID; i < int(tableA.GetID()); i++ {
tableName := fmt.Sprintf("foo%d", i)
sqlDBRestore.Exec(t, fmt.Sprintf("CREATE TABLE %s ();", tableName))
sqlDBRestore.Exec(t, fmt.Sprintf("COMMENT ON TABLE %s IS 'table comment'", tableName))
Expand Down
Loading

0 comments on commit e854509

Please sign in to comment.