From 8da38d766b156c84fec1f49fa74f6eb4940c0c82 Mon Sep 17 00:00:00 2001 From: Vivek Menezes Date: Mon, 15 May 2017 11:54:40 -0400 Subject: [PATCH] sql: allow schema changes on tables in state: TableDescriptor_ADD A new table descriptor is placed in the non-public TableDescriptor_ADD state when it refers to another table through an FK or interleaved reference, and can only be made public once the other tables back reference is visible across the cluster. This change allows schema change operations on such non-public tables, while preserving the non-public behavior for table leases. fixes #13505 --- pkg/sql/create.go | 4 ++-- pkg/sql/data_source.go | 10 +++++---- pkg/sql/descriptor.go | 3 ++- pkg/sql/drop.go | 2 +- pkg/sql/rename.go | 2 +- pkg/sql/show.go | 4 ++-- pkg/sql/show_fingerprints.go | 3 ++- pkg/sql/table.go | 35 +++++++++++++++++++---------- pkg/sql/testdata/logic_test/fk | 41 +++++++++++++++++++++++++++++++++- 9 files changed, 79 insertions(+), 25 deletions(-) diff --git a/pkg/sql/create.go b/pkg/sql/create.go index 8f5781772a92..5568a192c5ed 100644 --- a/pkg/sql/create.go +++ b/pkg/sql/create.go @@ -144,7 +144,7 @@ func (p *planner) CreateIndex(ctx context.Context, n *parser.CreateIndex) (planN return nil, err } - tableDesc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn) + tableDesc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn, true /*allowAdding*/) if err != nil { return nil, err } @@ -1053,7 +1053,7 @@ func addInterleave( return err } - parentTable, err := mustGetTableDesc(ctx, txn, vt, tn) + parentTable, err := mustGetTableDesc(ctx, txn, vt, tn, true /*allowAdding*/) if err != nil { return err } diff --git a/pkg/sql/data_source.go b/pkg/sql/data_source.go index fec5492ba78c..ed1c490a22b9 100644 --- a/pkg/sql/data_source.go +++ b/pkg/sql/data_source.go @@ -492,16 +492,18 @@ func (p *planner) getTableScanOrViewPlan( hints *parser.IndexHints, scanVisibility scanVisibility, ) (planDataSource, error) { - descFunc := p.session.leases.getTableLease + var desc *sqlbase.TableDescriptor + var err error if p.avoidCachedDescriptors { // AS OF SYSTEM TIME queries need to fetch the table descriptor at the // specified time, and never lease anything. The proto transaction already // has its timestamps set correctly so getTableOrViewDesc will fetch with // the correct timestamp. - descFunc = mustGetTableOrViewDesc + desc, err = mustGetTableOrViewDesc( + ctx, p.txn, p.getVirtualTabler(), tn, false /*allowAdding*/) + } else { + desc, err = p.session.leases.getTableLease(ctx, p.txn, p.getVirtualTabler(), tn) } - - desc, err := descFunc(ctx, p.txn, p.getVirtualTabler(), tn) if err != nil { return planDataSource{}, err } diff --git a/pkg/sql/descriptor.go b/pkg/sql/descriptor.go index d1a990f03b68..381376ba73a1 100644 --- a/pkg/sql/descriptor.go +++ b/pkg/sql/descriptor.go @@ -268,7 +268,8 @@ func getDescriptorsFromTargetList( return nil, err } for i := range tables { - descriptor, err := mustGetTableOrViewDesc(ctx, txn, vt, &tables[i]) + descriptor, err := mustGetTableOrViewDesc( + ctx, txn, vt, &tables[i], true /*allowAdding*/) if err != nil { return nil, err } diff --git a/pkg/sql/drop.go b/pkg/sql/drop.go index 3ec12e9c50e2..1d428b225aff 100644 --- a/pkg/sql/drop.go +++ b/pkg/sql/drop.go @@ -243,7 +243,7 @@ func (p *planner) DropIndex(ctx context.Context, n *parser.DropIndex) (planNode, return nil, err } - tableDesc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn) + tableDesc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn, true /*allowAdding*/) if err != nil { return nil, err } diff --git a/pkg/sql/rename.go b/pkg/sql/rename.go index bcff9246a550..20e8d6575e45 100644 --- a/pkg/sql/rename.go +++ b/pkg/sql/rename.go @@ -258,7 +258,7 @@ func (p *planner) RenameIndex(ctx context.Context, n *parser.RenameIndex) (planN return nil, err } - tableDesc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn) + tableDesc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn, true /*allowAdding*/) if err != nil { return nil, err } diff --git a/pkg/sql/show.go b/pkg/sql/show.go index bd0a0b511633..77d4f57cb98f 100644 --- a/pkg/sql/show.go +++ b/pkg/sql/show.go @@ -364,7 +364,7 @@ func (p *planner) ShowCreateTable( return nil, err } - desc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn) + desc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn, true /*allowAdding*/) if err != nil { return nil, err } @@ -819,7 +819,7 @@ func (p *planner) ShowConstraints( return nil, err } - desc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn) + desc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn, true /*allowAdding*/) if err != nil { return nil, err } diff --git a/pkg/sql/show_fingerprints.go b/pkg/sql/show_fingerprints.go index 247c27a35413..7d92b99ec89d 100644 --- a/pkg/sql/show_fingerprints.go +++ b/pkg/sql/show_fingerprints.go @@ -68,7 +68,8 @@ func (p *planner) ShowFingerprints( txn.SetFixedTimestamp(ts) var err error - tableDesc, err = mustGetTableDesc(ctx, txn, p.getVirtualTabler(), tn) + tableDesc, err = mustGetTableDesc( + ctx, txn, p.getVirtualTabler(), tn, false /*allowAdding*/) return err }); err != nil { return nil, err diff --git a/pkg/sql/table.go b/pkg/sql/table.go index 90c647b1d71c..b5ecf5bcbd87 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -158,9 +158,10 @@ func getViewDesc( } // mustGetTableOrViewDesc returns a table descriptor for either a table or -// view, or an error if the descriptor is not found. +// view, or an error if the descriptor is not found. allowAdding when set allows +// a table descriptor in the ADD state to also be returned. func mustGetTableOrViewDesc( - ctx context.Context, txn *client.Txn, vt VirtualTabler, tn *parser.TableName, + ctx context.Context, txn *client.Txn, vt VirtualTabler, tn *parser.TableName, allowAdding bool, ) (*sqlbase.TableDescriptor, error) { desc, err := getTableOrViewDesc(ctx, txn, vt, tn) if err != nil { @@ -170,15 +171,18 @@ func mustGetTableOrViewDesc( return nil, sqlbase.NewUndefinedTableError(tn.String()) } if err := filterTableState(desc); err != nil { - return nil, err + if !allowAdding && err != errTableAdding { + return nil, err + } } return desc, nil } // mustGetTableDesc returns a table descriptor for a table, or an error if -// the descriptor is not found. +// the descriptor is not found. allowAdding when set allows a table descriptor +// in the ADD state to also be returned. func mustGetTableDesc( - ctx context.Context, txn *client.Txn, vt VirtualTabler, tn *parser.TableName, + ctx context.Context, txn *client.Txn, vt VirtualTabler, tn *parser.TableName, allowAdding bool, ) (*sqlbase.TableDescriptor, error) { desc, err := getTableDesc(ctx, txn, vt, tn) if err != nil { @@ -188,13 +192,15 @@ func mustGetTableDesc( return nil, sqlbase.NewUndefinedTableError(tn.String()) } if err := filterTableState(desc); err != nil { - return nil, err + if !allowAdding && err != errTableAdding { + return nil, err + } } return desc, nil } // mustGetViewDesc returns a table descriptor for a view, or an error if the -// descriptor is not found. +// descriptor is not found or descriptor.Dropped(). func mustGetViewDesc( ctx context.Context, txn *client.Txn, vt VirtualTabler, tn *parser.TableName, ) (*sqlbase.TableDescriptor, error) { @@ -246,13 +252,10 @@ func (lc *LeaseCollection) getTableLease( // so they cannot be leased. Instead, we simply return the static // descriptor and rely on the immutability privileges set on the // descriptors to cause upper layers to reject mutations statements. - tbl, err := mustGetTableDesc(ctx, txn, vt, tn) + tbl, err := mustGetTableDesc(ctx, txn, vt, tn, false /*allowAdding*/) if err != nil { return nil, err } - if err := filterTableState(tbl); err != nil { - return nil, err - } return tbl, nil } @@ -447,6 +450,12 @@ func (p *planner) writeTableDesc(ctx context.Context, tableDesc *sqlbase.TableDe if isVirtualDescriptor(tableDesc) { panic(fmt.Sprintf("Virtual Descriptors cannot be stored, found: %v", tableDesc)) } + // Some statements setTestingVerifyMetadata to verify the descriptor they + // have written, but if they are followed by other statements that modify + // the descriptor the verification of the overwritten descriptor cannot be + // done. + p.session.setTestingVerifyMetadata(nil) + return p.txn.Put( ctx, sqlbase.MakeDescMetadataKey(tableDesc.GetID()), sqlbase.WrapDescriptor(tableDesc), ) @@ -572,7 +581,9 @@ func (p *planner) findTableContainingIndex( result = nil for i := range tns { tn := &tns[i] - tableDesc, err := mustGetTableDesc(ctx, p.txn, p.getVirtualTabler(), tn) + tableDesc, err := mustGetTableDesc( + ctx, p.txn, p.getVirtualTabler(), tn, true, /*allowAdding*/ + ) if err != nil { return nil, err } diff --git a/pkg/sql/testdata/logic_test/fk b/pkg/sql/testdata/logic_test/fk index 5ccc4fe1f93e..4d9be5dee50e 100644 --- a/pkg/sql/testdata/logic_test/fk +++ b/pkg/sql/testdata/logic_test/fk @@ -675,7 +675,46 @@ statement ok BEGIN TRANSACTION statement ok -CREATE TABLE refers (a INT REFERENCES referee); +CREATE TABLE refers ( + a INT REFERENCES referee, + b INT, + INDEX b_idx (b) +) + +# Add some schema changes within the same transaction to verify that a +# table that isn't yet public can be modified. +statement ok +CREATE INDEX foo ON refers (a) + +statement ok +ALTER INDEX refers@b_idx RENAME TO another_idx + +query TT +SHOW CREATE TABLE refers +---- +refers CREATE TABLE refers ( +a INT NULL, +b INT NULL, +INDEX another_idx (b ASC), +CONSTRAINT fk_a_ref_referee FOREIGN KEY (a) REFERENCES referee (id), +FAMILY "primary" (a, b, rowid) +) + +statement ok +DROP INDEX refers@another_idx + +statement ok +COMMIT + +# CREATE AND DROP a table with a fk in the same transaction. +statement ok +BEGIN TRANSACTION + +statement ok +CREATE TABLE refers1 (a INT REFERENCES referee); + +statement ok +DROP TABLE refers1 statement ok COMMIT