Merge #81372

81372: sql: Fixed issue that sequence is sometimes referenced by name r=Xiang-Gu a=Xiang-Gu

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.

fixes: #77145, #81333
Release note: None

Co-authored-by: Xiang Gu <[email protected]>

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

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

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
 		}
 	}

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
 					}

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

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

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