Skip to content

Commit 77a89ad

Browse files
committed
scbuild: use ValidateComputedColumnExpressionWithLookup function
This validates the computed column by referencing the builder state rather than the table descriptor. The table descriptor would not reflect the schema changes that are being staged by the declarative schema changer. Release note: None
1 parent 88ee935 commit 77a89ad

File tree

4 files changed

+67
-13
lines changed

4 files changed

+67
-13
lines changed

pkg/sql/schemachanger/scbuild/builder_state.go

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"context"
1010
"sort"
1111

12+
"github.com/cockroachdb/cockroach/pkg/clusterversion"
1213
"github.com/cockroachdb/cockroach/pkg/config/zonepb"
1314
"github.com/cockroachdb/cockroach/pkg/keys"
1415
"github.com/cockroachdb/cockroach/pkg/security/username"
@@ -838,29 +839,60 @@ func (b *builderState) WrapExpression(tableID catid.DescID, expr tree.Expr) *scp
838839

839840
// ComputedColumnExpression implements the scbuildstmt.TableHelpers interface.
840841
func (b *builderState) ComputedColumnExpression(
841-
tbl *scpb.Table, d *tree.ColumnTableDef, exprContext tree.SchemaExprContext,
842+
tbl *scpb.Table,
843+
d *tree.ColumnTableDef,
844+
exprContext tree.SchemaExprContext,
845+
getAllNonDropColumnsFn func() colinfo.ResultColumns,
846+
columnLookupByNameFn schemaexpr.ColumnLookupFn,
842847
) (tree.Expr, *types.T) {
843848
_, _, ns := scpb.FindNamespace(b.QueryByID(tbl.TableID))
844849
tn := tree.MakeTableNameFromPrefix(b.NamePrefix(tbl), tree.Name(ns.Name))
845850
b.ensureDescriptor(tbl.TableID)
846-
// TODO(postamar): this doesn't work when referencing newly added columns.
847-
expr, typ, err := schemaexpr.ValidateComputedColumnExpression(
851+
852+
// In versions before 26.1, computed columns referencing newly added columns
853+
// are not supported in the declarative schema changer. Fall back to the
854+
// legacy schema changer for those cases.
855+
if !b.clusterSettings.Version.IsActive(b.ctx, clusterversion.V26_1) {
856+
// Use the old validation logic that doesn't support newly added columns.
857+
expr, typ, err := schemaexpr.ValidateComputedColumnExpression(
858+
b.ctx,
859+
b.descCache[tbl.TableID].desc.(catalog.TableDescriptor),
860+
d,
861+
&tn,
862+
exprContext,
863+
b.semaCtx,
864+
b.clusterSettings.Version.ActiveVersion(b.ctx),
865+
)
866+
if err != nil {
867+
// This may be referencing newly added columns, so return a
868+
// NotImplementedError to force fallback to the legacy schema changer.
869+
if pgerror.GetPGCode(err) == pgcode.UndefinedColumn {
870+
panic(errors.Wrapf(errors.WithSecondaryError(scerrors.NotImplementedError(d), err),
871+
"computed column validation error"))
872+
}
873+
panic(err)
874+
}
875+
parsedExpr, err := parser.ParseExpr(expr)
876+
if err != nil {
877+
panic(err)
878+
}
879+
return parsedExpr, typ
880+
}
881+
882+
// In 26.1+, we can validate computed columns that reference newly added
883+
// columns by using the lookup functions to query the builder state.
884+
expr, typ, err := schemaexpr.ValidateComputedColumnExpressionWithLookup(
848885
b.ctx,
849886
b.descCache[tbl.TableID].desc.(catalog.TableDescriptor),
850887
d,
851888
&tn,
852889
exprContext,
853890
b.semaCtx,
854891
b.clusterSettings.Version.ActiveVersion(b.ctx),
892+
getAllNonDropColumnsFn,
893+
columnLookupByNameFn,
855894
)
856895
if err != nil {
857-
// This may be referencing newly added columns, so cheat and return
858-
// a not implemented error.
859-
if pgerror.GetPGCode(err) == pgcode.UndefinedColumn {
860-
861-
panic(errors.Wrapf(errors.WithSecondaryError(scerrors.NotImplementedError(d), err),
862-
"computed column validation error"))
863-
}
864896
panic(err)
865897
}
866898
parsedExpr, err := parser.ParseExpr(expr)

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,15 @@ func alterTableAddColumn(
167167
))
168168
}
169169
if desc.IsComputed() {
170-
validExpr, _ := b.ComputedColumnExpression(tbl, d, tree.ComputedColumnExprContext(d.IsVirtual()))
170+
validExpr, _ := b.ComputedColumnExpression(
171+
tbl, d, tree.ComputedColumnExprContext(d.IsVirtual()),
172+
func() colinfo.ResultColumns {
173+
return getNonDropResultColumns(b, tbl.TableID)
174+
},
175+
func(columnName tree.Name) (exists, accessible, computed bool, id catid.ColumnID, typ *types.T) {
176+
return columnLookupFn(b, tbl.TableID, columnName)
177+
},
178+
)
171179
expr := b.WrapExpression(tbl.TableID, validExpr)
172180
if spec.colType.ElementCreationMetadata.In_24_3OrLater {
173181
spec.compute = &scpb.ColumnComputeExpression{

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -978,7 +978,15 @@ func maybeCreateVirtualColumnForIndex(
978978
d.Nullable.Nullability = tree.Null
979979
// Infer column type from expression.
980980
{
981-
_, columnType := b.ComputedColumnExpression(tbl, d, tree.ExpressionIndexElementExpr)
981+
_, columnType := b.ComputedColumnExpression(
982+
tbl, d, tree.ExpressionIndexElementExpr,
983+
func() colinfo.ResultColumns {
984+
return getNonDropResultColumns(b, tbl.TableID)
985+
},
986+
func(columnName tree.Name) (exists, accessible, computed bool, id catid.ColumnID, typ *types.T) {
987+
return columnLookupFn(b, tbl.TableID, columnName)
988+
},
989+
)
982990
d.Type = columnType
983991
validateColumnIndexableType(columnType)
984992
}

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,10 @@ import (
1515
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
1616
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
1717
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
18+
"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
1819
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
1920
"github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion"
21+
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
2022
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
2123
"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scdecomp"
2224
"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
@@ -313,7 +315,11 @@ type TableHelpers interface {
313315
// ComputedColumnExpression returns a validated computed column expression
314316
// and its type.
315317
// TODO(postamar): make this more low-level instead of consuming an AST
316-
ComputedColumnExpression(tbl *scpb.Table, d *tree.ColumnTableDef, exprContext tree.SchemaExprContext) (tree.Expr, *types.T)
318+
ComputedColumnExpression(
319+
tbl *scpb.Table, d *tree.ColumnTableDef, exprContext tree.SchemaExprContext,
320+
getAllNonDropColumnsFn func() colinfo.ResultColumns,
321+
columnLookupByNameFn schemaexpr.ColumnLookupFn,
322+
) (tree.Expr, *types.T)
317323

318324
// PartialIndexPredicateExpression returns a validated partial predicate
319325
// wrapped expression

0 commit comments

Comments
 (0)