Merge #149444 #149838 #150142 #150154

149444: schemachanger: implement ALTER COLUMN [SET | DROP] ON UPDATE in declarative r=rafiss a=rafiss

This adds support for setting and dropping ON UPDATE expressions to the
declarative schema changer. No new elements were needed, so this was
pretty straightforward.


fixes #139604

Release note: None


149838: ui: fix bug where "Drop Unused Index" recommendations are not populat… r=kyle-a-wong a=kyle-a-wong

…ed on Schema Insights

When hard refreshing on the Schema Index tab of the insights page, "Drop Unused Index" recommendations don't populate. This is happening due to a race condition in db console, where db console needs cluster settings from the redux store to query for unused indexes. On a hard refresh, the cluster settings aren't in the redux store when querying for these indexes, so it uses the default cluster setting. To fix, the query no longer depends on cluster settings from the reduxstore. Instead, thequery joins on crdb_internal.cluster_settings.

Epic: CC-30965
Fixes: CC-31265
Release note (ui change): Fix bug where "Drop Unused Index" recommendations are not populated on the schema index page.

150142: restore: delete comments after failed or cancelled cluster restore r=dt a=msbutler

This patch ensures that after a failed or canceled cluster restore, comments from dropped descriptors are removed.

Informs #149453

Release note: none

150154: gceworker.sh: update default worker image r=mw5h a=mw5h

Ubuntu 20.04 LTS is past EOL and no longer available. DevInf has made
20.04 available in the cockroach-workers project as 'crl-ubuntu-2004'.
We also no longer suppoert image families, so this patch changes the
image project to 'cockroach-workers' and points at the new image.

Epic: none
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Matt White <[email protected]>

Author: 5 people

pkg/backup/backup_test.go

@@ -11184,3 +11184,60 @@ CREATE TABLE child_pk (k INT8 PRIMARY KEY REFERENCES parent);
 		sqlDB.Exec(t, `DROP DATABASE test`)
 	}
 }
+
+func TestRestoreFailureDeletesComments(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	_, sqlDB, cleanupFn := backupRestoreTestSetupEmpty(t, singleNode, "", InitManualReplication, base.TestClusterArgs{})
+	defer cleanupFn()
+
+	// Set pause point for after the system tables have been published.
+	sqlDB.Exec(t, `SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.after_cleanup_temp_system_tables'`)
+
+	commentCountQuery := `SELECT count(*) FROM system.comments`
+
+	var count int
+	sqlDB.QueryRow(t, commentCountQuery).Scan(&count)
+	require.Equal(t, 0, count)
+
+	// Create a database with tables, types, and schemas that have comments
+	sqlDB.Exec(t, `CREATE DATABASE test_db`)
+	sqlDB.Exec(t, `USE test_db`)
+
+	sqlDB.Exec(t, `CREATE TYPE custom_type AS ENUM ('val1', 'val2')`)
+	sqlDB.Exec(t, `COMMENT ON TYPE custom_type IS 'This is a custom type comment'`)
+
+	sqlDB.Exec(t, `CREATE SCHEMA test_schema`)
+	sqlDB.Exec(t, `COMMENT ON SCHEMA test_schema IS 'This is a schema comment'`)
+
+	sqlDB.Exec(t, `CREATE TABLE test_schema.test_table (id INT PRIMARY KEY, name STRING)`)
+	sqlDB.Exec(t, `COMMENT ON TABLE test_schema.test_table IS 'This is a table comment'`)
+
+	sqlDB.Exec(t, `COMMENT ON DATABASE test_db IS 'This is a database comment'`)
+
+	sqlDB.QueryRow(t, commentCountQuery).Scan(&count)
+	require.Equal(t, 4, count)
+
+	sqlDB.Exec(t, `BACKUP INTO 'nodelocal://1/test_backup'`)
+
+	sqlDB.Exec(t, `USE system`)
+
+	sqlDB.Exec(t, `DROP DATABASE test_db CASCADE`)
+	sqlDB.QueryRow(t, commentCountQuery).Scan(&count)
+	require.Equal(t, 0, count)
+
+	var jobID jobspb.JobID
+	sqlDB.QueryRow(t, `RESTORE FROM LATEST IN 'nodelocal://1/test_backup' WITH detached`).Scan(&jobID)
+	jobutils.WaitForJobToPause(t, sqlDB, jobID)
+
+	sqlDB.QueryRow(t, commentCountQuery).Scan(&count)
+	require.Equal(t, 4, count)
+
+	// Cancel the restore job
+	sqlDB.Exec(t, `CANCEL JOB $1`, jobID)
+	jobutils.WaitForJobToCancel(t, sqlDB, jobID)
+
+	sqlDB.QueryRow(t, commentCountQuery).Scan(&count)
+	require.Equal(t, 0, count)
+}

pkg/backup/restore_job.go

@@ -37,6 +37,7 @@ import (
 	"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/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen"
@@ -2843,6 +2844,10 @@ func (r *restoreResumer) dropDescriptors(
 		tablesToGC = append(tablesToGC, tableToDrop.ID)
 		tableToDrop.SetDropped()
 
+		if err := descsCol.DeleteTableComments(ctx, kvTrace, b, tableToDrop.ID); err != nil {
+			return err
+		}
+
 		// Drop any schedules we may have implicitly created.
 		if tableToDrop.HasRowLevelTTL() {
 			scheduleID := tableToDrop.RowLevelTTL.ScheduleID
@@ -2898,6 +2903,14 @@ func (r *restoreResumer) dropDescriptors(
 		}
 		mutType.SetDropped()
 
+		if err := descsCol.DeleteCommentInBatch(
+			ctx,
+			kvTrace,
+			b,
+			catalogkeys.MakeCommentKey(uint32(typDesc.ID), 0, catalogkeys.TypeCommentType)); err != nil {
+			return err
+		}
+
 		if err := descsCol.DeleteNamespaceEntryToBatch(ctx, kvTrace, typDesc, b); err != nil {
 			return err
 		}
@@ -2993,6 +3006,14 @@ func (r *restoreResumer) dropDescriptors(
 			entry.db = mutParent.(*dbdesc.Mutable)
 		}
 
+		if err := descsCol.DeleteCommentInBatch(
+			ctx,
+			kvTrace,
+			b,
+			catalogkeys.MakeCommentKey(uint32(schemaDesc.GetID()), 0, catalogkeys.SchemaCommentType)); err != nil {
+			return err
+		}
+
 		// Delete schema entries in descriptor and namespace system tables.
 		if err := descsCol.DeleteNamespaceEntryToBatch(ctx, kvTrace, mutSchema, b); err != nil {
 			return err
@@ -3076,6 +3097,14 @@ func (r *restoreResumer) dropDescriptors(
 				return err
 			}
 		}
+		if err := descsCol.DeleteCommentInBatch(
+			ctx,
+			kvTrace,
+			b,
+			catalogkeys.MakeCommentKey(uint32(dbDesc.GetID()), 0, catalogkeys.DatabaseCommentType)); err != nil {
+			return err
+		}
+
 		if err := descsCol.DeleteNamespaceEntryToBatch(ctx, kvTrace, db, b); err != nil {
 			return err
 		}

pkg/ccl/schemachangerccl/sctestbackupccl/backup_base_generated_test.go

@@ -1000,10 +1000,9 @@ func applyColumnMutation(
 					fk.OnUpdate != semenumpb.ForeignKeyAction_RESTRICT {
 					return pgerror.Newf(
 						pgcode.InvalidColumnDefinition,
-						"column %s(%d) cannot have both an ON UPDATE expression and a foreign"+
-							" key ON UPDATE action",
+						"column cannot specify both ON UPDATE expression and a foreign"+
+							" key ON UPDATE action for column %q",
 						col.GetName(),
-						col.GetID(),
 					)
 				}
 			}

pkg/sql/alter_table.go

@@ -1010,6 +1010,11 @@ func rewriteSchemaChangerState(
 				// just drop the target.
 				removeElementAtCurrentIdx()
 				continue
+			case *scpb.ColumnOnUpdateExpression:
+				// IF there is any dependency missing for column ON UPDATE expression,
+				// we just drop the target.
+				removeElementAtCurrentIdx()
+				continue
 			case *scpb.SequenceOwner:
 				// If a sequence owner is missing the sequence, then the sequence
 				// was already dropped and this element can be safely removed.

pkg/sql/catalog/rewrite/rewrite.go

@@ -167,7 +167,7 @@ CREATE TABLE test_fk_invalid (p STRING PRIMARY KEY, j STRING ON UPDATE 'test', C
 statement ok
 CREATE TABLE alter_fk (p STRING PRIMARY KEY, j STRING REFERENCES test_fk_base (j) ON UPDATE CASCADE)
 
-statement error pq: column j\(2\) cannot have both an ON UPDATE expression and a foreign key ON UPDATE action
+statement error cannot specify both ON UPDATE expression and a foreign key ON UPDATE action for column "j"
 ALTER TABLE alter_fk ALTER COLUMN j SET ON UPDATE 'failure'
 
 statement ok

pkg/sql/logictest/testdata/logic_test/on_update

@@ -10,6 +10,7 @@ go_library(
         "alter_table_alter_column_drop_not_null.go",
         "alter_table_alter_column_set_default.go",
         "alter_table_alter_column_set_not_null.go",
+        "alter_table_alter_column_set_on_update.go",
         "alter_table_alter_column_type.go",
         "alter_table_alter_primary_key.go",
         "alter_table_drop_column.go",

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel

@@ -38,6 +38,7 @@ var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{
 	reflect.TypeOf((*tree.AlterTableDropConstraint)(nil)):     {fn: alterTableDropConstraint, on: true, checks: nil},
 	reflect.TypeOf((*tree.AlterTableValidateConstraint)(nil)): {fn: alterTableValidateConstraint, on: true, checks: nil},
 	reflect.TypeOf((*tree.AlterTableSetDefault)(nil)):         {fn: alterTableSetDefault, on: true, checks: nil},
+	reflect.TypeOf((*tree.AlterTableSetOnUpdate)(nil)):        {fn: alterTableSetOnUpdate, on: true, checks: isV254Active},
 	reflect.TypeOf((*tree.AlterTableAlterColumnType)(nil)):    {fn: alterTableAlterColumnType, on: true, checks: nil},
 	reflect.TypeOf((*tree.AlterTableSetRLSMode)(nil)):         {fn: alterTableSetRLSMode, on: true, checks: isV252Active},
 	reflect.TypeOf((*tree.AlterTableDropNotNull)(nil)):        {fn: alterTableDropNotNull, on: true, checks: isV253Active},

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go

@@ -80,7 +80,7 @@ func panicIfInvalidNonComputedColumnExpr(
 	}
 
 	colType := mustRetrieveColumnTypeElem(b, tbl.TableID, col.ColumnID)
-	typedNewExpr, _, err := sanitizeColumnExpression(context.Background(), b.SemaCtx(), newExpr, colType, schemaChange)
+	typedNewExpr, _, err := sanitizeColumnExpression(b, b.SemaCtx(), newExpr, colType, schemaChange)
 	if err != nil {
 		panic(err)
 	}

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_default.go

@@ -0,0 +1,51 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package scbuildstmt
+
+import (
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+)
+
+func alterTableSetOnUpdate(
+	b BuildCtx,
+	tn *tree.TableName,
+	tbl *scpb.Table,
+	stmt tree.Statement,
+	t *tree.AlterTableSetOnUpdate,
+) {
+	alterColumnPreChecks(b, tn, tbl, t.Column)
+	colID := getColumnIDFromColumnName(b, tbl.TableID, t.Column, true /* required */)
+	col := mustRetrieveColumnElem(b, tbl.TableID, colID)
+	oldOnUpdateExpr := retrieveColumnOnUpdateExpressionElem(b, tbl.TableID, colID)
+	colType := mustRetrieveColumnTypeElem(b, tbl.TableID, colID)
+
+	// Block alters on system columns.
+	panicIfSystemColumn(col, t.Column.String())
+
+	// Block disallowed operations on computed columns.
+	panicIfComputedColumn(b, tn.ObjectName, colType, t.Column.String(), t.Expr)
+
+	// For DROP ON UPDATE.
+	if t.Expr == nil {
+		if oldOnUpdateExpr != nil {
+			b.Drop(oldOnUpdateExpr)
+		}
+		return
+	}
+
+	// If our target column already has an ON UPDATE expression, we want to drop it first.
+	if oldOnUpdateExpr != nil {
+		b.Drop(oldOnUpdateExpr)
+	}
+	typedExpr := panicIfInvalidNonComputedColumnExpr(b, tbl, tn.ToUnresolvedObjectName(), col, t.Column.String(), t.Expr, tree.ColumnOnUpdateExpr)
+
+	b.Add(&scpb.ColumnOnUpdateExpression{
+		TableID:    tbl.TableID,
+		ColumnID:   colID,
+		Expression: *b.WrapExpression(tbl.TableID, typedExpr),
+	})
+}