Merge #134411

134411: sql/schemachanger: Prevent Temporary Disappearance of Altered Columns r=spilchen a=spilchen

Previously, during an ALTER COLUMN ... TYPE operation, the column being modified could briefly disappear, leading to "column does not exist" errors for any queries attempting to access it. This issue arose from the incorrect ordering of dependency rules, which has now been addressed.

The changes to the dependency rules during the column type alteration are as follows:
- The name of the new column is not set until we are ready to make it public. Previously, the column name was assigned too early in the pre-commit phase, which necessitated additional shadow transient column name logic. This new approach simplifies the process and eliminates the need for that extra rename logic.
- The ColumnName and Column elements for both the old and new columns are swapped within the same stage.

I have also maintained the existing rules for cases where the column type is not being altered by utilizing the IsAlterColumnTypeOp and IsNotAlterColumnTypeOp predicates.

I updated existing tests to query the column during each stage of the alter. This allowed me to reproduce the issue.

Epic: CRDB-25314
Closes #133996
Release note: none

Co-authored-by: Matt Spilchen <[email protected]>

pkg/ccl/schemachangerccl/testdata/decomp/multiregion

@@ -190,37 +190,31 @@ ElementState:
     tableId: 110
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 1
     name: b
     tableId: 110
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 2
     name: rowid
     tableId: 110
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967292e+09
     name: crdb_internal_origin_timestamp
     tableId: 110
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967293e+09
     name: crdb_internal_origin_id
     tableId: 110
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967294e+09
     name: tableoid
     tableId: 110
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967295e+09
     name: crdb_internal_mvcc_timestamp
     tableId: 110
@@ -591,37 +585,31 @@ ElementState:
     tableId: 109
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 1
     name: a
     tableId: 109
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 2
     name: rowid
     tableId: 109
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967292e+09
     name: crdb_internal_origin_timestamp
     tableId: 109
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967293e+09
     name: crdb_internal_origin_id
     tableId: 109
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967294e+09
     name: tableoid
     tableId: 109
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967295e+09
     name: crdb_internal_mvcc_timestamp
     tableId: 109
@@ -1021,43 +1009,36 @@ ElementState:
     tableId: 108
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 1
     name: k
     tableId: 108
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 2
     name: v
     tableId: 108
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 3
     name: crdb_region
     tableId: 108
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967292e+09
     name: crdb_internal_origin_timestamp
     tableId: 108
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967293e+09
     name: crdb_internal_origin_id
     tableId: 108
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967294e+09
     name: tableoid
     tableId: 108
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967295e+09
     name: crdb_internal_mvcc_timestamp
     tableId: 108

pkg/ccl/schemachangerccl/testdata/decomp/partitioning

@@ -111,43 +111,36 @@ ElementState:
     tableId: 104
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 1
     name: pk
     tableId: 104
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 2
     name: a
     tableId: 104
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 3
     name: j
     tableId: 104
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967292e+09
     name: crdb_internal_origin_timestamp
     tableId: 104
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967293e+09
     name: crdb_internal_origin_id
     tableId: 104
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967294e+09
     name: tableoid
     tableId: 104
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967295e+09
     name: crdb_internal_mvcc_timestamp
     tableId: 104
@@ -631,37 +624,31 @@ ElementState:
     tableId: 105
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 1
     name: a
     tableId: 105
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 2
     name: b
     tableId: 105
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967292e+09
     name: crdb_internal_origin_timestamp
     tableId: 105
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967293e+09
     name: crdb_internal_origin_id
     tableId: 105
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967294e+09
     name: tableoid
     tableId: 105
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967295e+09
     name: crdb_internal_mvcc_timestamp
     tableId: 105

pkg/sql/schemachanger/dml_injection_test.go

@@ -186,6 +186,7 @@ func TestAlterTableDMLInjection(t *testing.T) {
 				"ALTER TABLE tbl ADD COLUMN new_col BIGINT NOT NULL DEFAULT 100",
 			},
 			schemaChange: "ALTER TABLE tbl ALTER COLUMN new_col SET DATA TYPE TEXT",
+			query:        "SELECT new_col FROM tbl LIMIT 1",
 		},
 		{
 			desc:         "add column default udf",

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

@@ -13,7 +13,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
@@ -302,8 +301,10 @@ func handleGeneralColumnConversion(
 
 	oldColComment, newColComment := getColumnCommentForColumnReplacement(b, tbl.TableID, col.ColumnID, newColID)
 
-	// First set the target status of the old column to drop. We will replace this
-	// column with a new column. This column stays visible until the second backfill.
+	// First, set the target status of the old column to drop. This column will be
+	// replaced by a new one but remains visible until the new column is ready to be
+	// made public. A dependency rule ensures that the old and new columns are swapped
+	// within the same stage.
 	b.Drop(col)
 	b.Drop(colName)
 	b.Drop(oldColType)
@@ -337,7 +338,7 @@ func handleGeneralColumnConversion(
 	// Add the spec for the new column. It will be identical to the column it is replacing,
 	// except the type will differ, and it will have a transient computed expression.
 	// This expression will reference the original column to facilitate the backfill.
-	// This column becomes visible after the first backfill.
+	// This column becomes visible in the same stage the old column becomes invisible.
 	spec := addColumnSpec{
 		tbl: tbl,
 		col: &scpb.Column{
@@ -370,24 +371,6 @@ func handleGeneralColumnConversion(
 		fam: nil,
 	}
 	addColumn(b, spec, t)
-
-	// The above operation will cause a backfill to occur twice. Once with both columns,
-	// then another time with the old column removed. Since both columns will exist at
-	// the same time for a short period of time, we need to rename the old column so that
-	// we can access either one. We add this name as a transient so that it is cleaned up
-	// prior to the old column being totally removed.
-	nameExists := func(name string) bool {
-		return getColumnIDFromColumnName(b, tbl.TableID, tree.Name(name), false /* required */) != 0
-	}
-	oldColumnRename := tabledesc.GenerateUniqueName(fmt.Sprintf("%s_shadow", colName.Name), nameExists)
-	b.AddTransient(&scpb.ColumnName{
-		TableID:  tbl.TableID,
-		ColumnID: col.ColumnID,
-		Name:     oldColumnRename,
-		// If we don't complete the operation, the column won't be dropped, so we
-		// need to remember the original name to preserve it.
-		AbsentName: colName.Name,
-	})
 }
 
 func updateColumnType(b BuildCtx, oldColType, newColType *scpb.ColumnType) {

pkg/sql/schemachanger/scbuild/testdata/alter_table_alter_column_type

@@ -107,5 +107,3 @@ ALTER TABLE t ALTER COLUMN c2 SET DATA TYPE BIGINT USING c2::BIGINT
   {columnId: 4, indexId: 4, kind: STORED, ordinalInKind: 1, tableId: 104}
 - [[IndexColumn:{DescID: 104, ColumnID: 4, IndexID: 5}, TRANSIENT_ABSENT], ABSENT]
   {columnId: 4, indexId: 5, kind: STORED, ordinalInKind: 1, tableId: 104}
-- [[ColumnName:{DescID: 104, Name: c2_shadow, ColumnID: 2}, TRANSIENT_ABSENT], ABSENT]
-  {absentName: c2, columnId: 2, name: c2_shadow, tableId: 104}

pkg/sql/schemachanger/scdecomp/testdata/other

@@ -285,31 +285,26 @@ ElementState:
     tableId: 112
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 1
     name: id
     tableId: 112
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967292e+09
     name: crdb_internal_origin_timestamp
     tableId: 112
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967293e+09
     name: crdb_internal_origin_id
     tableId: 112
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967294e+09
     name: tableoid
     tableId: 112
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967295e+09
     name: crdb_internal_mvcc_timestamp
     tableId: 112
@@ -570,37 +565,31 @@ ElementState:
     tableId: 113
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 1
     name: k
     tableId: 113
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 2
     name: id
     tableId: 113
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967292e+09
     name: crdb_internal_origin_timestamp
     tableId: 113
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967293e+09
     name: crdb_internal_origin_id
     tableId: 113
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967294e+09
     name: tableoid
     tableId: 113
   Status: PUBLIC
 - ColumnName:
-    absentName: ""
     columnId: 4.294967295e+09
     name: crdb_internal_mvcc_timestamp
     tableId: 113