From 5000c3c6a38794e02035205403ecb8339c3c8e6f Mon Sep 17 00:00:00 2001 From: Xiang Gu Date: Tue, 17 May 2022 10:46:42 -0400 Subject: [PATCH] sql: Fixed issue that sequence is sometimes referenced by name Previously, if a column's on-update expression is defined to use a sequence (e.g. ON UPDATE nextval('s')), it is stored with the sequence name rather than its id (this can be observed via `SHOW CREATE` or inspecting the corresponding table descriptor in `system.descriptor`). This can be problematic with 'RENAME SEQUENCE AS'. This PR fixes this issue to ensure sequences, when used in default or on-update expression of a column, is always referenced using its ID. Release note: None --- pkg/sql/add_column.go | 4 +- pkg/sql/alter_table.go | 21 ++++----- pkg/sql/catalog/tabledesc/table.go | 19 ++++++-- pkg/sql/create_table.go | 4 +- .../logictest/testdata/logic_test/alter_table | 43 +++++++++++++++++++ .../testdata/logic_test/create_table | 43 +++++++++++++++++++ pkg/sql/sequence.go | 12 +++++- 7 files changed, 128 insertions(+), 18 deletions(-) diff --git a/pkg/sql/add_column.go b/pkg/sql/add_column.go index b15e9a4d6c75..cee5cbcda560 100644 --- a/pkg/sql/add_column.go +++ b/pkg/sql/add_column.go @@ -164,9 +164,9 @@ func (p *planner) addColumnImpl( // If the new column has a DEFAULT or an ON UPDATE expression that uses a // sequence, add references between its descriptor and this column descriptor. - if err := cdd.ForEachTypedExpr(func(expr tree.TypedExpr) error { + if err := cdd.ForEachTypedExpr(func(expr tree.TypedExpr, colExprKind tabledesc.ColExprKind) error { changedSeqDescs, err := maybeAddSequenceDependencies( - params.ctx, params.ExecCfg().Settings, params.p, n.tableDesc, col, expr, nil, + params.ctx, params.ExecCfg().Settings, params.p, n.tableDesc, col, expr, nil, colExprKind, ) if err != nil { return err diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 881bc11d28ce..1d4248c3b4e8 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -1160,19 +1160,19 @@ func updateSequenceDependencies( seqDescsToUpdate = truncated } for _, colExpr := range []struct { - name string - exists func() bool - get func() string + colExprKind tabledesc.ColExprKind + exists func() bool + get func() string }{ { - name: "DEFAULT", - exists: colDesc.HasDefault, - get: colDesc.GetDefaultExpr, + colExprKind: tabledesc.DefaultExpr, + exists: colDesc.HasDefault, + get: colDesc.GetDefaultExpr, }, { - name: "ON UPDATE", - exists: colDesc.HasOnUpdate, - get: colDesc.GetOnUpdateExpr, + colExprKind: tabledesc.OnUpdateExpr, + exists: colDesc.HasOnUpdate, + get: colDesc.GetOnUpdateExpr, }, } { if !colExpr.exists() { @@ -1187,7 +1187,7 @@ func updateSequenceDependencies( params, untypedExpr, colDesc, - "DEFAULT", + string(colExpr.colExprKind), ) if err != nil { return err @@ -1201,6 +1201,7 @@ func updateSequenceDependencies( colDesc.ColumnDesc(), typedExpr, nil, /* backrefs */ + colExpr.colExprKind, ) if err != nil { return err diff --git a/pkg/sql/catalog/tabledesc/table.go b/pkg/sql/catalog/tabledesc/table.go index 37ec032e7e5f..28db026625ff 100644 --- a/pkg/sql/catalog/tabledesc/table.go +++ b/pkg/sql/catalog/tabledesc/table.go @@ -57,15 +57,28 @@ type ColumnDefDescs struct { // hash-sharded index or primary key. const MaxBucketAllowed = 2048 +// ColExprKind is an enum type of possible expressions on a column +// (e.g. 'DEFAULT' expression or 'ON UPDATE' expression). +type ColExprKind string + +const ( + // DefaultExpr means the expression is a DEFAULT expression. + DefaultExpr ColExprKind = "DEFAULT" + // OnUpdateExpr means the expression is a ON UPDATE expression. + OnUpdateExpr ColExprKind = "ON UPDATE" +) + // ForEachTypedExpr iterates over each typed expression in this struct. -func (cdd *ColumnDefDescs) ForEachTypedExpr(fn func(tree.TypedExpr) error) error { +func (cdd *ColumnDefDescs) ForEachTypedExpr( + fn func(expr tree.TypedExpr, colExprKind ColExprKind) error, +) error { if cdd.ColumnTableDef.HasDefaultExpr() { - if err := fn(cdd.DefaultExpr); err != nil { + if err := fn(cdd.DefaultExpr, DefaultExpr); err != nil { return err } } if cdd.ColumnTableDef.HasOnUpdateExpr() { - if err := fn(cdd.OnUpdateExpr); err != nil { + if err := fn(cdd.OnUpdateExpr, OnUpdateExpr); err != nil { return err } } diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index a2e8066825ce..ad4169d6cf3f 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -2103,9 +2103,9 @@ func NewTableDesc( for i := range n.Defs { if _, ok := n.Defs[i].(*tree.ColumnTableDef); ok { if cdd[i] != nil { - if err := cdd[i].ForEachTypedExpr(func(expr tree.TypedExpr) error { + if err := cdd[i].ForEachTypedExpr(func(expr tree.TypedExpr, colExprKind tabledesc.ColExprKind) error { changedSeqDescs, err := maybeAddSequenceDependencies( - ctx, st, vt, &desc, &desc.Columns[colIdx], expr, affected) + ctx, st, vt, &desc, &desc.Columns[colIdx], expr, affected, colExprKind) if err != nil { return err } diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 2c004025a723..38b640bb566c 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -2508,3 +2508,46 @@ select (select count(*) from t81448@t81448_b_key bk inner join t81448_b cp on (c statement ok drop table t81448, t81448_b + +subtest sequence_is_referenced_by_ID + +statement ok +DROP TABLE IF EXISTS tbl + +statement ok +CREATE TABLE tbl (i INT PRIMARY KEY) + +statement ok +CREATE SEQUENCE IF NOT EXISTS s1 + +statement ok +CREATE SEQUENCE IF NOT EXISTS s2 + +statement ok +ALTER TABLE tbl ADD COLUMN j INT NOT NULL DEFAULT nextval('s1') ON UPDATE nextval('s2') + +query TT +SHOW CREATE TABLE tbl +---- +tbl CREATE TABLE public.tbl ( + i INT8 NOT NULL, + j INT8 NOT NULL DEFAULT nextval('public.s1'::REGCLASS) ON UPDATE nextval('public.s2'::REGCLASS), + CONSTRAINT tbl_pkey PRIMARY KEY (i ASC) +) + +# Now use `ALTER COLUMN` to swap the use of sequence 's1' and 's2' + +statement ok +ALTER TABLE tbl ALTER COLUMN j SET DEFAULT nextval('s2') + +statement ok +ALTER TABLE tbl ALTER COLUMN j SET ON UPDATE nextval('s1') + +query TT +SHOW CREATE TABLE tbl +---- +tbl CREATE TABLE public.tbl ( + i INT8 NOT NULL, + j INT8 NOT NULL DEFAULT nextval('public.s2'::REGCLASS) ON UPDATE nextval('public.s1'::REGCLASS), + CONSTRAINT tbl_pkey PRIMARY KEY (i ASC) +) diff --git a/pkg/sql/logictest/testdata/logic_test/create_table b/pkg/sql/logictest/testdata/logic_test/create_table index 622377a987cc..72cb15ecdb50 100644 --- a/pkg/sql/logictest/testdata/logic_test/create_table +++ b/pkg/sql/logictest/testdata/logic_test/create_table @@ -944,3 +944,46 @@ CREATE TABLE t22 (a int) WITH (sql_stats_automatic_collection_fraction_stale_row statement error pq: invalid integer value for sql_stats_automatic_collection_min_stale_rows: cannot be set to a negative value: -1 CREATE TABLE t22 (a int) WITH (sql_stats_automatic_collection_min_stale_rows = -1) + +subtest sequence_is_referenced_by_ID + +statement ok +CREATE SEQUENCE IF NOT EXISTS s + +statement ok +DROP TABLE IF EXISTS tbl + +statement ok +CREATE TABLE tbl (i INT PRIMARY KEY, j INT NOT NULL ON UPDATE nextval('s'), FAMILY f1 (i, j)) + +query TT +SHOW CREATE TABLE tbl +---- +tbl CREATE TABLE public.tbl ( + i INT8 NOT NULL, + j INT8 NOT NULL ON UPDATE nextval('public.s'::REGCLASS), + CONSTRAINT tbl_pkey PRIMARY KEY (i ASC), + FAMILY f1 (i, j) +) + +statement ok +CREATE SEQUENCE IF NOT EXISTS s1 + +statement ok +CREATE SEQUENCE IF NOT EXISTS s2 + +statement ok +DROP TABLE IF EXISTS tbl + +statement ok +CREATE TABLE tbl (i INT PRIMARY KEY, j INT NOT NULL DEFAULT nextval('s1') ON UPDATE nextval('s2'), FAMILY f1 (i, j)) + +query TT +SHOW CREATE TABLE tbl +---- +tbl CREATE TABLE public.tbl ( + i INT8 NOT NULL, + j INT8 NOT NULL DEFAULT nextval('public.s1'::REGCLASS) ON UPDATE nextval('public.s2'::REGCLASS), + CONSTRAINT tbl_pkey PRIMARY KEY (i ASC), + FAMILY f1 (i, j) +) diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 068975077955..a039197416a8 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -814,6 +814,8 @@ func addSequenceOwner( // if the column has a DEFAULT expression that uses one or more sequences. (Usually just one, // e.g. `DEFAULT nextval('my_sequence')`. // The passed-in column descriptor is mutated, and the modified sequence descriptors are returned. +// `colExprKind`, either 'DEFAULT' or "ON UPDATE", tells which expression `expr` is, so we can +// correctly modify `col` (see issue #81333). func maybeAddSequenceDependencies( ctx context.Context, st *cluster.Settings, @@ -822,6 +824,7 @@ func maybeAddSequenceDependencies( col *descpb.ColumnDescriptor, expr tree.TypedExpr, backrefs map[descpb.ID]*tabledesc.Mutable, + colExprKind tabledesc.ColExprKind, ) ([]*tabledesc.Mutable, error) { seqIdentifiers, err := seqexpr.GetUsedSequences(expr) if err != nil { @@ -903,7 +906,14 @@ func maybeAddSequenceDependencies( return nil, err } s := tree.Serialize(newExpr) - col.DefaultExpr = &s + switch colExprKind { + case tabledesc.DefaultExpr: + col.DefaultExpr = &s + case tabledesc.OnUpdateExpr: + col.OnUpdateExpr = &s + default: + return nil, errors.AssertionFailedf("colExprKind must be either 'DEFAULT' or 'ON UPDATE'; got %v", colExprKind) + } } return seqDescs, nil