From 546bf5af011a1a8490a99ae63c0a4fafe5348358 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Wed, 6 May 2020 19:09:21 -0400 Subject: [PATCH] sql: use SQL sequence as DescIDGenerator for non-system tenants Fixes #48513. This commit creates a new SQL sequence called "descriptor_id_seq" that is used by all non-system tenants to allocate descriptor IDs. This is necessary because non-system tenants should not be using the global `DescIDGenerator` key to allocate IDs. The next commit includes tests that exercise the MetadataSchema portion of this change. --- pkg/ccl/backupccl/backup_test.go | 2 +- pkg/ccl/backupccl/restore_planning.go | 8 ++--- pkg/ccl/importccl/import_stmt.go | 2 +- pkg/keys/constants.go | 1 + pkg/keys/sql.go | 17 ++++++++++ pkg/sql/create_sequence.go | 21 +++++------- pkg/sql/create_table.go | 2 +- pkg/sql/create_type.go | 2 +- pkg/sql/create_view.go | 2 +- pkg/sql/descriptor.go | 6 ++-- pkg/sql/sqlbase/metadata.go | 8 ++++- pkg/sql/sqlbase/structured.go | 4 +++ pkg/sql/sqlbase/system.go | 47 ++++++++++++++++++++++++- pkg/sql/temporary_schema.go | 2 +- pkg/sql/tests/system_table_test.go | 1 + pkg/sql/testutils.go | 49 ++++++++++++++++++--------- pkg/sql/truncate.go | 2 +- 17 files changed, 131 insertions(+), 45 deletions(-) diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 578853ecb8bf..2d531872bf1a 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -1103,7 +1103,7 @@ func TestBackupRestoreResume(t *testing.T) { sqlDB.Exec(t, `BACKUP DATABASE DATA TO $1`, restoreDir) sqlDB.Exec(t, `CREATE DATABASE restoredb`) restoreDatabaseID := sqlutils.QueryDatabaseID(t, sqlDB.DB, "restoredb") - restoreTableID, err := sql.GenerateUniqueDescID(ctx, tc.Servers[0].DB()) + restoreTableID, err := sql.GenerateUniqueDescID(ctx, tc.Servers[0].DB(), keys.SystemSQLCodec) if err != nil { t.Fatal(err) } diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index 16df9430d4fa..7348cddffa74 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -204,14 +204,14 @@ func allocateTableRewrites( // since for clusters with many descrirptors we'd want to avoid // incrementing it 10,000+ times. for i := uint32(0); i <= numberOfIncrements; i++ { - _, err = sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB) + _, err = sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) if err != nil { return nil, err } } // Generate one more desc ID for the ID of the temporary system db. - tempSysDBID, err = sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB) + tempSysDBID, err = sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) if err != nil { return nil, err } @@ -338,7 +338,7 @@ func allocateTableRewrites( if descriptorCoverage == tree.AllDescriptors { newID = db.ID } else { - newID, err = sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB) + newID, err = sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) if err != nil { return nil, err } @@ -372,7 +372,7 @@ func allocateTableRewrites( // Generate new IDs for the tables that need to be remapped. for _, table := range tablesToRemap { - newTableID, err := sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB) + newTableID, err := sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) if err != nil { return nil, err } diff --git a/pkg/ccl/importccl/import_stmt.go b/pkg/ccl/importccl/import_stmt.go index b5db2bfc4c3f..bffdb2d11569 100644 --- a/pkg/ccl/importccl/import_stmt.go +++ b/pkg/ccl/importccl/import_stmt.go @@ -871,7 +871,7 @@ func prepareNewTableDescsForIngestion( tableRewrites := make(backupccl.TableRewriteMap) seqVals := make(map[sqlbase.ID]int64, len(tables)) for _, tableDesc := range tables { - id, err := sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB) + id, err := sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) if err != nil { return nil, err } diff --git a/pkg/keys/constants.go b/pkg/keys/constants.go index e76fdd201eb0..4408171787c2 100644 --- a/pkg/keys/constants.go +++ b/pkg/keys/constants.go @@ -332,6 +332,7 @@ const ( UsersTableID = 4 ZonesTableID = 5 SettingsTableID = 6 + DescIDSequenceID = 7 // IDs for the important columns and indexes in the zones table live here to // avoid introducing a dependency on sql/sqlbase throughout the codebase. diff --git a/pkg/keys/sql.go b/pkg/keys/sql.go index d5ade3a7d798..e24e4c6c75ee 100644 --- a/pkg/keys/sql.go +++ b/pkg/keys/sql.go @@ -87,6 +87,13 @@ var SystemSQLCodec = MakeSQLCodec(roachpb.SystemTenantID) // surrounding context. var TODOSQLCodec = MakeSQLCodec(roachpb.SystemTenantID) +// systemTenant returns whether the encoder is bound to the system tenant. This +// information should not be exposed, but is used internally to the encoder when +// key encoding for the system tenant differs from that of all other tenants. +func (e sqlEncoder) systemTenant() bool { + return len(e.TenantPrefix()) == 0 +} + // TenantPrefix returns the key prefix used for the tenants's data. func (e sqlEncoder) TenantPrefix() roachpb.Key { return *e.buf @@ -124,6 +131,16 @@ func (e sqlEncoder) SequenceKey(tableID uint32) roachpb.Key { return k } +// DescIDSequenceKey returns the key used for the descriptor ID sequence. +func (e sqlEncoder) DescIDSequenceKey() roachpb.Key { + if e.systemTenant() { + // To maintain backwards compatibility, the system tenant uses a + // separate, non-SQL, key to store its descriptor ID sequence. + return DescIDGenerator + } + return e.SequenceKey(DescIDSequenceID) +} + // ZoneKeyPrefix returns the key prefix for id's row in the system.zones table. func (e sqlEncoder) ZoneKeyPrefix(id uint32) roachpb.Key { k := e.IndexPrefix(ZonesTableID, ZonesTablePrimaryIndexID) diff --git a/pkg/sql/create_sequence.go b/pkg/sql/create_sequence.go index 742dd86cbe89..480bf6216205 100644 --- a/pkg/sql/create_sequence.go +++ b/pkg/sql/create_sequence.go @@ -82,7 +82,7 @@ func doCreateSequence( opts tree.SequenceOptions, jobDesc string, ) error { - id, err := GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB) + id, err := GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB, params.p.ExecCfg().Codec) if err != nil { return err } @@ -157,11 +157,6 @@ func (*createSequenceNode) Next(runParams) (bool, error) { return false, nil } func (*createSequenceNode) Values() tree.Datums { return tree.Datums{} } func (*createSequenceNode) Close(context.Context) {} -const ( - sequenceColumnID = 1 - sequenceColumnName = "value" -) - // MakeSequenceTableDesc creates a sequence descriptor. func MakeSequenceTableDesc( sequenceName string, @@ -187,25 +182,25 @@ func MakeSequenceTableDesc( // Mimic a table with one column, "value". desc.Columns = []sqlbase.ColumnDescriptor{ { - ID: 1, - Name: sequenceColumnName, + ID: sqlbase.SequenceColumnID, + Name: sqlbase.SequenceColumnName, Type: types.Int, }, } desc.PrimaryIndex = sqlbase.IndexDescriptor{ ID: keys.SequenceIndexID, Name: sqlbase.PrimaryKeyIndexName, - ColumnIDs: []sqlbase.ColumnID{sqlbase.ColumnID(1)}, - ColumnNames: []string{sequenceColumnName}, + ColumnIDs: []sqlbase.ColumnID{sqlbase.SequenceColumnID}, + ColumnNames: []string{sqlbase.SequenceColumnName}, ColumnDirections: []sqlbase.IndexDescriptor_Direction{sqlbase.IndexDescriptor_ASC}, } desc.Families = []sqlbase.ColumnFamilyDescriptor{ { ID: keys.SequenceColumnFamilyID, - ColumnIDs: []sqlbase.ColumnID{1}, - ColumnNames: []string{sequenceColumnName}, + ColumnIDs: []sqlbase.ColumnID{sqlbase.SequenceColumnID}, + ColumnNames: []string{sqlbase.SequenceColumnName}, Name: "primary", - DefaultColumnID: sequenceColumnID, + DefaultColumnID: sqlbase.SequenceColumnID, }, } diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index d82a01a33a28..2d03e51da7ec 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -261,7 +261,7 @@ func (n *createTableNode) startExec(params runParams) error { } } - id, err := GenerateUniqueDescID(params.ctx, params.extendedEvalCtx.ExecCfg.DB) + id, err := GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB, params.p.ExecCfg().Codec) if err != nil { return err } diff --git a/pkg/sql/create_type.go b/pkg/sql/create_type.go index 05bd562fa6d1..b76c8cb9c981 100644 --- a/pkg/sql/create_type.go +++ b/pkg/sql/create_type.go @@ -91,7 +91,7 @@ func getCreateTypeParams( if err != nil { return nil, 0, err } - id, err := GenerateUniqueDescID(params.ctx, params.extendedEvalCtx.ExecCfg.DB) + id, err := GenerateUniqueDescID(params.ctx, params.ExecCfg().DB, params.ExecCfg().Codec) if err != nil { return nil, 0, err } diff --git a/pkg/sql/create_view.go b/pkg/sql/create_view.go index 87898d223ef1..e3f4df6974d8 100644 --- a/pkg/sql/create_view.go +++ b/pkg/sql/create_view.go @@ -128,7 +128,7 @@ func (n *createViewNode) startExec(params runParams) error { } } else { // If we aren't replacing anything, make a new table descriptor. - id, err := GenerateUniqueDescID(params.ctx, params.extendedEvalCtx.ExecCfg.DB) + id, err := GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB, params.p.ExecCfg().Codec) if err != nil { return err } diff --git a/pkg/sql/descriptor.go b/pkg/sql/descriptor.go index 71e456a17edb..cd491df3f820 100644 --- a/pkg/sql/descriptor.go +++ b/pkg/sql/descriptor.go @@ -55,9 +55,9 @@ var MaxDefaultDescriptorID = keys.MaxReservedDescID + sqlbase.ID(len(DefaultUser // GenerateUniqueDescID returns the next available Descriptor ID and increments // the counter. The incrementing is non-transactional, and the counter could be // incremented multiple times because of retries. -func GenerateUniqueDescID(ctx context.Context, db *kv.DB) (sqlbase.ID, error) { +func GenerateUniqueDescID(ctx context.Context, db *kv.DB, codec keys.SQLCodec) (sqlbase.ID, error) { // Increment unique descriptor counter. - newVal, err := kv.IncrementValRetryable(ctx, db, keys.DescIDGenerator, 1) + newVal, err := kv.IncrementValRetryable(ctx, db, codec.DescIDSequenceKey(), 1) if err != nil { return sqlbase.InvalidID, err } @@ -92,7 +92,7 @@ func (p *planner) createDatabase( return false, err } - id, err := GenerateUniqueDescID(ctx, p.ExecCfg().DB) + id, err := GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) if err != nil { return false, err } diff --git a/pkg/sql/sqlbase/metadata.go b/pkg/sql/sqlbase/metadata.go index ac519fa6d934..a4ee9aa3feb8 100644 --- a/pkg/sql/sqlbase/metadata.go +++ b/pkg/sql/sqlbase/metadata.go @@ -95,6 +95,12 @@ func MakeMetadataSchema( return ms } +// forSystemTenant returns whether this MetadataSchema is associated with the +// system tenant or with a secondary tenant. +func (ms *MetadataSchema) forSystemTenant() bool { + return ms.codec == keys.SystemSQLCodec +} + // AddDescriptor adds a new non-config descriptor to the system schema. func (ms *MetadataSchema) AddDescriptor(parentID ID, desc DescriptorProto) { if id := desc.GetID(); id > keys.MaxReservedDescID { @@ -137,7 +143,7 @@ func (ms MetadataSchema) GetInitialValues() ([]roachpb.KeyValue, []roachpb.RKey) value := roachpb.Value{} value.SetInt(int64(keys.MinUserDescID)) ret = append(ret, roachpb.KeyValue{ - Key: keys.DescIDGenerator, + Key: ms.codec.DescIDSequenceKey(), Value: value, }) diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index e0c1a9367498..7c1b835a08ae 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -206,6 +206,10 @@ const InvalidMutationID MutationID = 0 const ( // PrimaryKeyIndexName is the name of the index for the primary key. PrimaryKeyIndexName = "primary" + // SequenceColumnID is the ID of the sole column in a sequence. + SequenceColumnID = 1 + // SequenceColumnName is the name of the sole column in a sequence. + SequenceColumnName = "value" ) // ErrMissingColumns indicates a table with no columns. diff --git a/pkg/sql/sqlbase/system.go b/pkg/sql/sqlbase/system.go index 2a021c6ac9b9..8c1d45b9aa4e 100644 --- a/pkg/sql/sqlbase/system.go +++ b/pkg/sql/sqlbase/system.go @@ -12,6 +12,7 @@ package sqlbase import ( "fmt" + "math" "time" "github.com/cockroachdb/cockroach/pkg/config/zonepb" @@ -96,6 +97,9 @@ CREATE TABLE system.settings ( "valueType" STRING, FAMILY (name, value, "lastUpdated", "valueType") );` + + DescIDSequenceSchema = ` +CREATE SEQUENCE system.descriptor_id_seq;` ) // These system tables are not part of the system config. @@ -290,7 +294,7 @@ create table system.statement_diagnostics( func pk(name string) IndexDescriptor { return IndexDescriptor{ - Name: "primary", + Name: PrimaryKeyIndexName, ID: 1, Unique: true, ColumnNames: []string{name}, @@ -315,6 +319,7 @@ var SystemAllowedPrivileges = map[ID]privilege.List{ // the use of a validating, logging accessor, so we'll go ahead and tolerate // read-only privs to make that migration possible later. keys.SettingsTableID: privilege.ReadWriteData, + keys.DescIDSequenceID: privilege.ReadData, keys.LeaseTableID: privilege.ReadWriteData, keys.EventLogTableID: privilege.ReadWriteData, keys.RangeEventTableID: privilege.ReadWriteData, @@ -567,6 +572,40 @@ var ( FormatVersion: InterleavedFormatVersion, NextMutationID: 1, } + + // DescIDSequence is the descriptor for the descriptor ID sequence. + DescIDSequence = TableDescriptor{ + Name: "descriptor_id_seq", + ID: keys.DescIDSequenceID, + ParentID: keys.SystemDatabaseID, + UnexposedParentSchemaID: keys.PublicSchemaID, + Version: 1, + Columns: []ColumnDescriptor{ + {Name: SequenceColumnName, ID: SequenceColumnID, Type: types.Int}, + }, + Families: []ColumnFamilyDescriptor{{ + Name: "primary", + ID: keys.SequenceColumnFamilyID, + ColumnNames: []string{SequenceColumnName}, + ColumnIDs: []ColumnID{SequenceColumnID}, + DefaultColumnID: SequenceColumnID, + }}, + PrimaryIndex: IndexDescriptor{ + ID: keys.SequenceIndexID, + Name: PrimaryKeyIndexName, + ColumnIDs: []ColumnID{SequenceColumnID}, + ColumnNames: []string{SequenceColumnName}, + ColumnDirections: []IndexDescriptor_Direction{IndexDescriptor_ASC}, + }, + SequenceOpts: &TableDescriptor_SequenceOpts{ + Increment: 1, + MinValue: 1, + MaxValue: math.MaxInt64, + Start: 1, + }, + Privileges: NewCustomSuperuserPrivilegeDescriptor(SystemAllowedPrivileges[keys.DescIDSequenceID]), + FormatVersion: InterleavedFormatVersion, + } ) // These system TableDescriptor literals should match the descriptor that @@ -1486,6 +1525,12 @@ func addSystemDescriptorsToSchema(target *MetadataSchema) { target.AddDescriptor(keys.SystemDatabaseID, &ZonesTable) target.AddDescriptor(keys.SystemDatabaseID, &SettingsTable) + // Only add the descriptor ID sequence if this is a non-system tenant. + // System tenants use the global DescIDGenerator key. See #48513. + if !target.forSystemTenant() { + target.AddDescriptor(keys.SystemDatabaseID, &DescIDSequence) + } + // Add all the other system tables. target.AddDescriptor(keys.SystemDatabaseID, &LeaseTable) target.AddDescriptor(keys.SystemDatabaseID, &EventLogTable) diff --git a/pkg/sql/temporary_schema.go b/pkg/sql/temporary_schema.go index 4a080d822e2f..0b69569a7815 100644 --- a/pkg/sql/temporary_schema.go +++ b/pkg/sql/temporary_schema.go @@ -83,7 +83,7 @@ var ( ) func createTempSchema(params runParams, sKey sqlbase.DescriptorKey) (sqlbase.ID, error) { - id, err := GenerateUniqueDescID(params.ctx, params.extendedEvalCtx.ExecCfg.DB) + id, err := GenerateUniqueDescID(params.ctx, params.p.ExecCfg().DB, params.p.ExecCfg().Codec) if err != nil { return sqlbase.InvalidID, err } diff --git a/pkg/sql/tests/system_table_test.go b/pkg/sql/tests/system_table_test.go index dd291786665d..31282bedcc84 100644 --- a/pkg/sql/tests/system_table_test.go +++ b/pkg/sql/tests/system_table_test.go @@ -112,6 +112,7 @@ func TestSystemTableLiterals(t *testing.T) { {keys.UITableID, sqlbase.UITableSchema, sqlbase.UITable}, {keys.JobsTableID, sqlbase.JobsTableSchema, sqlbase.JobsTable}, {keys.SettingsTableID, sqlbase.SettingsTableSchema, sqlbase.SettingsTable}, + {keys.DescIDSequenceID, sqlbase.DescIDSequenceSchema, sqlbase.DescIDSequence}, {keys.WebSessionsTableID, sqlbase.WebSessionsTableSchema, sqlbase.WebSessionsTable}, {keys.TableStatisticsTableID, sqlbase.TableStatisticsTableSchema, sqlbase.TableStatisticsTable}, {keys.LocationsTableID, sqlbase.LocationsTableSchema, sqlbase.LocationsTable}, diff --git a/pkg/sql/testutils.go b/pkg/sql/testutils.go index e945f332caf8..c8fc111b7e82 100644 --- a/pkg/sql/testutils.go +++ b/pkg/sql/testutils.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/util/hlc" + "github.com/cockroachdb/errors" ) // CreateTestTableDescriptor converts a SQL string to a table for test purposes. @@ -38,22 +39,38 @@ func CreateTestTableDescriptor( } semaCtx := tree.MakeSemaContext() evalCtx := tree.MakeTestingEvalContext(st) - desc, err := MakeTableDesc( - ctx, - nil, /* txn */ - nil, /* vt */ - st, - stmt.AST.(*tree.CreateTable), - parentID, keys.PublicSchemaID, id, - hlc.Timestamp{}, /* creationTime */ - privileges, - nil, /* affected */ - &semaCtx, - &evalCtx, - &sessiondata.SessionData{}, /* sessionData */ - false, /* temporary */ - ) - return desc.TableDescriptor, err + switch n := stmt.AST.(type) { + case *tree.CreateTable: + desc, err := MakeTableDesc( + ctx, + nil, /* txn */ + nil, /* vt */ + st, + n, + parentID, keys.PublicSchemaID, id, + hlc.Timestamp{}, /* creationTime */ + privileges, + nil, /* affected */ + &semaCtx, + &evalCtx, + &sessiondata.SessionData{}, /* sessionData */ + false, /* temporary */ + ) + return desc.TableDescriptor, err + case *tree.CreateSequence: + desc, err := MakeSequenceTableDesc( + n.Name.Table(), + n.Options, + parentID, keys.PublicSchemaID, id, + hlc.Timestamp{}, /* creationTime */ + privileges, + false, /* temporary */ + nil, /* params */ + ) + return desc.TableDescriptor, err + default: + return sqlbase.TableDescriptor{}, errors.Errorf("unexpected AST %T", stmt.AST) + } } // StmtBufReader is an exported interface for reading a StmtBuf. diff --git a/pkg/sql/truncate.go b/pkg/sql/truncate.go index 53c754af5ba8..a1a2ad756f06 100644 --- a/pkg/sql/truncate.go +++ b/pkg/sql/truncate.go @@ -221,7 +221,7 @@ func (p *planner) truncateTable( return err } - newID, err := GenerateUniqueDescID(ctx, p.ExecCfg().DB) + newID, err := GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec) if err != nil { return err }