From 42ebbb1195df62e975037959eff3242d0800eaa5 Mon Sep 17 00:00:00 2001 From: richardjcai Date: Fri, 14 May 2021 16:56:27 -0400 Subject: [PATCH 1/4] build: add pipefail to teamcity-diagram-generation.sh Release note: None --- build/teamcity-diagram-generation.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/build/teamcity-diagram-generation.sh b/build/teamcity-diagram-generation.sh index c57213fcd9a8..ae551bf1dadc 100644 --- a/build/teamcity-diagram-generation.sh +++ b/build/teamcity-diagram-generation.sh @@ -1,5 +1,7 @@ #!/usr/bin/env bash +set -euo pipefail + source "$(dirname "${0}")/teamcity-support.sh" tc_start_block "Get Railroad Jar" From 3309fd0472b5822f6bb5ce9b50190539eb504636 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Mon, 17 May 2021 11:24:56 -0400 Subject: [PATCH 2/4] sql/opt: support implicit SELECT FOR UPDATE locking with nested renders This commit adds support for implicit SELECT FOR UPDATE to a collection of UPDATE and UPSERT queries that previously did not use row-level locking because they included 2 or more nested ProjExprs. The pattern matching that enabled implicit SELECT FOR UPDATE was not handling this case correctly. The most common case where this had an effect was with ``` INSERT INTO ... ON CONFLICT ... DO UPDATE SET ``` statements without a predicate. This class of statement always seems to include nested render expressions, and so it wasn't acquiring row-level locks during its initial row scan. Release note (sql change): `INSERT INTO ... ON CONFLICT ... DO UPDATE SET` statements without predicates now acquire locks using the FOR UPDATE locking mode during their initial row scan, which improves performance for contended workloads. This behavior is configurable using the enable_implicit_select_for_update session variable and the sql.defaults.implicit_select_for_update.enabled cluster setting. --- pkg/sql/opt/exec/execbuilder/mutation.go | 28 ++++---- .../execbuilder/testdata/check_constraints | 1 + pkg/sql/opt/exec/execbuilder/testdata/unique | 4 ++ pkg/sql/opt/exec/execbuilder/testdata/upsert | 70 +++++++++++++++++++ .../exec/execbuilder/testdata/virtual_columns | 11 +++ 5 files changed, 100 insertions(+), 14 deletions(-) diff --git a/pkg/sql/opt/exec/execbuilder/mutation.go b/pkg/sql/opt/exec/execbuilder/mutation.go index 8510c6b6bd01..ee92181ff7f0 100644 --- a/pkg/sql/opt/exec/execbuilder/mutation.go +++ b/pkg/sql/opt/exec/execbuilder/mutation.go @@ -1122,12 +1122,10 @@ func (b *Builder) shouldApplyImplicitLockingToUpdateInput(upd *memo.UpdateExpr) // Try to match the Update's input expression against the pattern: // - // [Project] [IndexJoin] Scan + // [Project]* [IndexJoin] Scan // input := upd.Input - if proj, ok := input.(*memo.ProjectExpr); ok { - input = proj.Input - } + input = unwrapProjectExprs(input) if idxJoin, ok := input.(*memo.IndexJoinExpr); ok { input = idxJoin.Input } @@ -1138,9 +1136,6 @@ func (b *Builder) shouldApplyImplicitLockingToUpdateInput(upd *memo.UpdateExpr) // tryApplyImplicitLockingToUpsertInput determines whether or not the builder // should apply a FOR UPDATE row-level locking mode to the initial row scan of // an UPSERT statement. -// -// TODO(nvanbenschoten): implement this method to match on appropriate Upsert -// expression trees and apply a row-level locking mode. func (b *Builder) shouldApplyImplicitLockingToUpsertInput(ups *memo.UpsertExpr) bool { if !b.evalCtx.SessionData.ImplicitSelectForUpdate { return false @@ -1148,12 +1143,10 @@ func (b *Builder) shouldApplyImplicitLockingToUpsertInput(ups *memo.UpsertExpr) // Try to match the Upsert's input expression against the pattern: // - // [Project] (LeftJoin Scan | LookupJoin) [Project] Values + // [Project]* (LeftJoin Scan | LookupJoin) [Project]* Values // input := ups.Input - if proj, ok := input.(*memo.ProjectExpr); ok { - input = proj.Input - } + input = unwrapProjectExprs(input) switch join := input.(type) { case *memo.LeftJoinExpr: if _, ok := join.Right.(*memo.ScanExpr); !ok { @@ -1167,9 +1160,7 @@ func (b *Builder) shouldApplyImplicitLockingToUpsertInput(ups *memo.UpsertExpr) default: return false } - if proj, ok := input.(*memo.ProjectExpr); ok { - input = proj.Input - } + input = unwrapProjectExprs(input) _, ok := input.(*memo.ValuesExpr) return ok } @@ -1183,3 +1174,12 @@ func (b *Builder) shouldApplyImplicitLockingToUpsertInput(ups *memo.UpsertExpr) func (b *Builder) shouldApplyImplicitLockingToDeleteInput(del *memo.DeleteExpr) bool { return false } + +// unwrapProjectExprs unwraps zero or more nested ProjectExprs. It returns the +// first non-ProjectExpr in the chain, or the input if it is not a ProjectExpr. +func unwrapProjectExprs(input memo.RelExpr) memo.RelExpr { + if proj, ok := input.(*memo.ProjectExpr); ok { + return unwrapProjectExprs(proj.Input) + } + return input +} diff --git a/pkg/sql/opt/exec/execbuilder/testdata/check_constraints b/pkg/sql/opt/exec/execbuilder/testdata/check_constraints index 206c0175bc5c..b2b810442084 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/check_constraints +++ b/pkg/sql/opt/exec/execbuilder/testdata/check_constraints @@ -51,6 +51,7 @@ vectorized: true estimated row count: 1 (missing stats) table: t9@primary spans: /5/0-/5/1/2 + locking strength: for update query T EXPLAIN (VERBOSE) UPDATE t9 SET a = 2 WHERE a = 5 diff --git a/pkg/sql/opt/exec/execbuilder/testdata/unique b/pkg/sql/opt/exec/execbuilder/testdata/unique index 37c2ca3d5a37..0df614801e0c 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/unique +++ b/pkg/sql/opt/exec/execbuilder/testdata/unique @@ -2188,6 +2188,7 @@ vectorized: true │ estimated row count: 10 (missing stats) │ table: uniq_enum@primary │ spans: /"\xc0"-/"\xc0"/PrefixEnd +│ locking strength: for update │ ├── • constraint-check │ │ @@ -3393,6 +3394,7 @@ vectorized: true │ │ table: uniq_enum@primary │ │ equality: (column1, column3) = (r,i) │ │ equality cols are key +│ │ locking strength: for update │ │ │ └── • values │ columns: (column1, column2, column3, column4) @@ -3543,6 +3545,7 @@ vectorized: true │ │ table: uniq_enum@uniq_enum_r_s_j_key │ │ equality cols are key │ │ lookup condition: ((column2 = s) AND (column4 = j)) AND (r IN ('us-east', 'us-west', 'eu-west')) +│ │ locking strength: for update │ │ │ └── • values │ columns: (column1, column2, column3, column4) @@ -4071,6 +4074,7 @@ vectorized: true │ │ table: uniq_partial_enum@primary │ │ equality: (column1, column2) = (r,a) │ │ equality cols are key +│ │ locking strength: for update │ │ │ └── • values │ columns: (column1, column2, column3, column4) diff --git a/pkg/sql/opt/exec/execbuilder/testdata/upsert b/pkg/sql/opt/exec/execbuilder/testdata/upsert index 0c96842f2c73..588805c18b8d 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/upsert +++ b/pkg/sql/opt/exec/execbuilder/testdata/upsert @@ -271,6 +271,76 @@ vectorized: true row 2, expr 0: 3 row 3, expr 0: 4 +query T +EXPLAIN (VERBOSE) +INSERT INTO indexed +VALUES (1, 2, 3) +ON CONFLICT (a) +DO UPDATE SET b = 2, c = 3 +---- +distribution: local +vectorized: true +· +• upsert +│ columns: () +│ estimated row count: 0 (missing stats) +│ into: indexed(a, b, c, d) +│ auto commit +│ arbiter indexes: primary +│ +└── • project + │ columns: (column1, column2, column3, d_comp, a, b, c, d, upsert_b, upsert_c, upsert_d, a, check1) + │ + └── • render + │ columns: (check1, column1, column2, column3, d_comp, a, b, c, d, upsert_b, upsert_c, upsert_d) + │ estimated row count: 1 (missing stats) + │ render check1: upsert_c > 0 + │ render column1: column1 + │ render column2: column2 + │ render column3: column3 + │ render d_comp: d_comp + │ render a: a + │ render b: b + │ render c: c + │ render d: d + │ render upsert_b: upsert_b + │ render upsert_c: upsert_c + │ render upsert_d: upsert_d + │ + └── • render + │ columns: (upsert_b, upsert_c, upsert_d, column1, column2, column3, d_comp, a, b, c, d) + │ estimated row count: 1 (missing stats) + │ render upsert_b: CASE WHEN a IS NULL THEN column2 ELSE 2 END + │ render upsert_c: CASE WHEN a IS NULL THEN column3 ELSE 3 END + │ render upsert_d: CASE WHEN a IS NULL THEN d_comp ELSE a + 3 END + │ render column1: column1 + │ render column2: column2 + │ render column3: column3 + │ render d_comp: d_comp + │ render a: a + │ render b: b + │ render c: c + │ render d: d + │ + └── • cross join (left outer) + │ columns: (column1, column2, column3, d_comp, a, b, c, d) + │ estimated row count: 1 (missing stats) + │ + ├── • values + │ columns: (column1, column2, column3, d_comp) + │ size: 4 columns, 1 row + │ row 0, expr 0: 1 + │ row 0, expr 1: 2 + │ row 0, expr 2: 3 + │ row 0, expr 3: 4 + │ + └── • scan + columns: (a, b, c, d) + estimated row count: 1 (missing stats) + table: indexed@primary + spans: /1/0 + locking strength: for update + # Drop index and verify that existing values no longer need to be fetched. statement ok DROP INDEX indexed@secondary CASCADE diff --git a/pkg/sql/opt/exec/execbuilder/testdata/virtual_columns b/pkg/sql/opt/exec/execbuilder/testdata/virtual_columns index e2cc1e9c6059..2fa773a3e8e9 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/virtual_columns +++ b/pkg/sql/opt/exec/execbuilder/testdata/virtual_columns @@ -423,6 +423,7 @@ vectorized: true estimated row count: 1,000 (missing stats) table: t@primary spans: FULL SCAN + locking strength: for update query T EXPLAIN (VERBOSE) UPDATE t SET b=b+1 WHERE v=45 RETURNING a,b,v @@ -504,6 +505,7 @@ vectorized: true estimated row count: 1,000 (missing stats) table: t_idx@primary spans: FULL SCAN + locking strength: for update query T EXPLAIN (VERBOSE) UPDATE t_idx SET b=b+1 RETURNING v, w @@ -546,6 +548,7 @@ vectorized: true estimated row count: 1,000 (missing stats) table: t_idx@primary spans: FULL SCAN + locking strength: for update query T EXPLAIN (VERBOSE) UPDATE t_idx SET b=b+1 WHERE v=45 RETURNING v, w @@ -594,6 +597,7 @@ vectorized: true estimated row count: 10 (missing stats) table: t_idx@t_idx_v_idx spans: /45-/46 + locking strength: for update query T EXPLAIN (VERBOSE) UPDATE t_idx SET c=6 WHERE a=2 @@ -663,6 +667,7 @@ vectorized: true estimated row count: 1,000 (missing stats) table: t_idx@primary spans: FULL SCAN + locking strength: for update query T EXPLAIN (VERBOSE) UPDATE t_idx SET b=b+1 RETURNING w @@ -705,6 +710,7 @@ vectorized: true estimated row count: 1,000 (missing stats) table: t_idx@primary spans: FULL SCAN + locking strength: for update subtest Upsert @@ -825,6 +831,7 @@ vectorized: true │ table: t@primary │ equality: (column1) = (a) │ equality cols are key + │ locking strength: for update │ └── • render │ columns: (v_comp, column1, column2) @@ -887,6 +894,7 @@ vectorized: true │ table: t@primary │ equality: (column1) = (a) │ equality cols are key + │ locking strength: for update │ └── • render │ columns: (v_comp, column1, column2) @@ -1019,6 +1027,7 @@ vectorized: true │ table: t_idx@primary │ equality: (column1) = (a) │ equality cols are key + │ locking strength: for update │ └── • render │ columns: (v_comp, w_comp, column1, column2, column3) @@ -1155,6 +1164,7 @@ vectorized: true │ table: t_idx@primary │ equality: (column1) = (a) │ equality cols are key + │ locking strength: for update │ └── • render │ columns: (v_comp, w_comp, column1, column2, column3) @@ -1233,6 +1243,7 @@ vectorized: true │ table: t_idx@primary │ equality: (column1) = (a) │ equality cols are key + │ locking strength: for update │ └── • render │ columns: (v_comp, w_comp, column1, column2, column3) From 78c511fb25db8482b6a31cadbcef9118f7c09edc Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 12 May 2021 16:15:34 -0700 Subject: [PATCH 3/4] schemaexpr: remove ComputedColumnValidator struct This commit removes `schemaexpr.ComputedColumnValidator`. The struct's methods did not store any state in the struct, so they have been converted to functions without a receiver. Release note: None --- pkg/sql/add_column.go | 8 +-- pkg/sql/alter_table.go | 8 +-- pkg/sql/catalog/schemaexpr/computed_column.go | 60 +++++++------------ pkg/sql/catalog/tabledesc/table.go | 5 ++ pkg/sql/create_table.go | 10 +--- pkg/sql/schemachanger/scbuild/table.go | 8 +-- 6 files changed, 34 insertions(+), 65 deletions(-) diff --git a/pkg/sql/add_column.go b/pkg/sql/add_column.go index b02b943b0083..18f162bb6129 100644 --- a/pkg/sql/add_column.go +++ b/pkg/sql/add_column.go @@ -157,13 +157,9 @@ func (p *planner) addColumnImpl( } if d.IsComputed() { - computedColValidator := schemaexpr.MakeComputedColumnValidator( - params.ctx, - n.tableDesc, - ¶ms.p.semaCtx, - tn, + serializedExpr, err := schemaexpr.ValidateComputedColumnExpression( + params.ctx, n.tableDesc, d, tn, params.p.SemaCtx(), ) - serializedExpr, err := computedColValidator.Validate(d) if err != nil { return err } diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 0293d31a8207..1d1fffc5c532 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -497,13 +497,7 @@ func (n *alterTableNode) startExec(params runParams) error { } // We cannot remove this column if there are computed columns that use it. - computedColValidator := schemaexpr.MakeComputedColumnValidator( - params.ctx, - n.tableDesc, - ¶ms.p.semaCtx, - tn, - ) - if err := computedColValidator.ValidateNoDependents(colToDrop); err != nil { + if err := schemaexpr.ValidateColumnHasNoDependents(n.tableDesc, colToDrop); err != nil { return err } diff --git a/pkg/sql/catalog/schemaexpr/computed_column.go b/pkg/sql/catalog/schemaexpr/computed_column.go index efdd66759535..49ee7a374f2d 100644 --- a/pkg/sql/catalog/schemaexpr/computed_column.go +++ b/pkg/sql/catalog/schemaexpr/computed_column.go @@ -26,29 +26,7 @@ import ( "github.com/cockroachdb/errors" ) -// ComputedColumnValidator validates that an expression is a valid computed -// column. See Validate for more details. -type ComputedColumnValidator struct { - ctx context.Context - desc catalog.TableDescriptor - semaCtx *tree.SemaContext - tableName *tree.TableName -} - -// MakeComputedColumnValidator returns an ComputedColumnValidator struct that -// can be used to validate computed columns. See Validate for more details. -func MakeComputedColumnValidator( - ctx context.Context, desc catalog.TableDescriptor, semaCtx *tree.SemaContext, tn *tree.TableName, -) ComputedColumnValidator { - return ComputedColumnValidator{ - ctx: ctx, - desc: desc, - semaCtx: semaCtx, - tableName: tn, - } -} - -// Validate verifies that an expression is a valid computed column expression. +// ValidateComputedColumnExpression verifies that an expression is a valid computed column expression. // It returns the serialized expression if valid, and an error otherwise. // // A computed column expression is valid if all of the following are true: @@ -57,8 +35,12 @@ func MakeComputedColumnValidator( // - It does not reference other computed columns. // // TODO(mgartner): Add unit tests for Validate. -func (v *ComputedColumnValidator) Validate( +func ValidateComputedColumnExpression( + ctx context.Context, + desc catalog.TableDescriptor, d *tree.ColumnTableDef, + tn *tree.TableName, + semaCtx *tree.SemaContext, ) (serializedExpr string, _ error) { if d.HasDefaultExpr() { return "", pgerror.New( @@ -69,7 +51,7 @@ func (v *ComputedColumnValidator) Validate( var depColIDs catalog.TableColSet // First, check that no column in the expression is a computed column. - err := iterColDescriptors(v.desc, d.Computed.Expr, func(c catalog.Column) error { + err := iterColDescriptors(desc, d.Computed.Expr, func(c catalog.Column) error { if c.IsComputed() { return pgerror.New(pgcode.InvalidTableDefinition, "computed columns cannot reference other computed columns") @@ -85,7 +67,7 @@ func (v *ComputedColumnValidator) Validate( // TODO(justin,bram): allow depending on columns like this. We disallow it // for now because cascading changes must hook into the computed column // update path. - if err := v.desc.ForeachOutboundFK(func(fk *descpb.ForeignKeyConstraint) error { + if err := desc.ForeachOutboundFK(func(fk *descpb.ForeignKeyConstraint) error { for _, id := range fk.OriginColumnIDs { if !depColIDs.Contains(id) { // We don't depend on this column. @@ -110,7 +92,7 @@ func (v *ComputedColumnValidator) Validate( } // Resolve the type of the computed column expression. - defType, err := tree.ResolveType(v.ctx, d.Type, v.semaCtx.GetTypeResolver()) + defType, err := tree.ResolveType(ctx, d.Type, semaCtx.GetTypeResolver()) if err != nil { return "", err } @@ -120,14 +102,14 @@ func (v *ComputedColumnValidator) Validate( // functions. In order to safely serialize user defined types and their // members, we need to serialize the typed expression here. expr, _, err := DequalifyAndValidateExpr( - v.ctx, - v.desc, + ctx, + desc, d.Computed.Expr, defType, "computed column", - v.semaCtx, + semaCtx, tree.VolatilityImmutable, - v.tableName, + tn, ) if err != nil { return "", err @@ -145,7 +127,7 @@ func (v *ComputedColumnValidator) Validate( return } var col catalog.Column - if col, err = v.desc.FindColumnWithID(colID); err != nil { + if col, err = desc.FindColumnWithID(colID); err != nil { err = errors.WithAssertionFailure(err) return } @@ -167,12 +149,12 @@ func (v *ComputedColumnValidator) Validate( return expr, nil } -// ValidateNoDependents verifies that the input column is not dependent on a -// computed column. The function errs if any existing computed columns or -// computed columns being added reference the given column. -// TODO(mgartner): Add unit tests for ValidateNoDependents. -func (v *ComputedColumnValidator) ValidateNoDependents(col catalog.Column) error { - for _, c := range v.desc.NonDropColumns() { +// ValidateColumnHasNoDependents verifies that the input column has no dependent +// computed columns. It returns an error if any existing or ADD mutation +// computed columns reference the given column. +// TODO(mgartner): Add unit tests. +func ValidateColumnHasNoDependents(desc catalog.TableDescriptor, col catalog.Column) error { + for _, c := range desc.NonDropColumns() { if !c.IsComputed() { continue } @@ -183,7 +165,7 @@ func (v *ComputedColumnValidator) ValidateNoDependents(col catalog.Column) error return errors.WithAssertionFailure(err) } - err = iterColDescriptors(v.desc, expr, func(colVar catalog.Column) error { + err = iterColDescriptors(desc, expr, func(colVar catalog.Column) error { if colVar.GetID() == col.GetID() { return pgerror.Newf( pgcode.InvalidColumnReference, diff --git a/pkg/sql/catalog/tabledesc/table.go b/pkg/sql/catalog/tabledesc/table.go index e36c99137286..6eee45a04b27 100644 --- a/pkg/sql/catalog/tabledesc/table.go +++ b/pkg/sql/catalog/tabledesc/table.go @@ -104,6 +104,11 @@ func MakeColumnDefDescs( } if d.IsComputed() { + // Note: We do not validate the computed column expression here because + // it may reference columns that have not yet been added to a table + // descriptor. Callers must validate the expression with + // schemaexpr.ValidateComputedColumnExpression once all possible + // reference columns are part of the table descriptor. s := tree.Serialize(d.Computed.Expr) col.ComputeExpr = &s } diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index aaf773ec22be..d886462c9ce9 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -2290,17 +2290,13 @@ func NewTableDesc( // Now that we have all the other columns set up, we can validate // any computed columns. - computedColValidator := schemaexpr.MakeComputedColumnValidator( - ctx, - &desc, - semaCtx, - &n.Table, - ) for _, def := range n.Defs { switch d := def.(type) { case *tree.ColumnTableDef: if d.IsComputed() { - serializedExpr, err := computedColValidator.Validate(d) + serializedExpr, err := schemaexpr.ValidateComputedColumnExpression( + ctx, &desc, d, &n.Table, semaCtx, + ) if err != nil { return nil, err } diff --git a/pkg/sql/schemachanger/scbuild/table.go b/pkg/sql/schemachanger/scbuild/table.go index bd29af7369c4..b79cc8d5eac6 100644 --- a/pkg/sql/schemachanger/scbuild/table.go +++ b/pkg/sql/schemachanger/scbuild/table.go @@ -132,13 +132,9 @@ func (b *buildContext) alterTableAddColumn( if d.IsComputed() { // TODO (lucy): This is not going to work when the computed column // references columns created in the same transaction. - computedColValidator := schemaexpr.MakeComputedColumnValidator( - ctx, - table, - b.SemaCtx, - tn, + serializedExpr, err := schemaexpr.ValidateComputedColumnExpression( + ctx, table, d, tn, b.SemaCtx, ) - serializedExpr, err := computedColValidator.Validate(d) if err != nil { panic(err) } From 1c40f61ff662306598ba829ebb7e40828ff9af32 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 12 May 2021 16:27:47 -0700 Subject: [PATCH 4/4] schemaexpr: remove IndexPredicateValidator struct This commit removes `schemaexpr.IndexPredicateValidator`. The struct's methods did not store any state in the struct, so they have been converted to functions without a receiver. Release note: None --- pkg/sql/catalog/schemaexpr/partial_index.go | 48 ++++++------------- .../catalog/schemaexpr/partial_index_test.go | 6 +-- pkg/sql/create_index.go | 5 +- pkg/sql/create_table.go | 9 ++-- 4 files changed, 26 insertions(+), 42 deletions(-) diff --git a/pkg/sql/catalog/schemaexpr/partial_index.go b/pkg/sql/catalog/schemaexpr/partial_index.go index aad052305dba..4aed4d2c97b7 100644 --- a/pkg/sql/catalog/schemaexpr/partial_index.go +++ b/pkg/sql/catalog/schemaexpr/partial_index.go @@ -21,35 +21,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/types" ) -// IndexPredicateValidator validates that an expression is a valid partial index -// predicate. See Validate for more details. -type IndexPredicateValidator struct { - ctx context.Context - tableName tree.TableName - desc catalog.TableDescriptor - semaCtx *tree.SemaContext -} - -// MakeIndexPredicateValidator returns an IndexPredicateValidator struct that -// can be used to validate partial index predicates. See Validate for more -// details. -func MakeIndexPredicateValidator( - ctx context.Context, - tableName tree.TableName, - desc catalog.TableDescriptor, - semaCtx *tree.SemaContext, -) IndexPredicateValidator { - return IndexPredicateValidator{ - ctx: ctx, - tableName: tableName, - desc: desc, - semaCtx: semaCtx, - } -} - -// Validate verifies that an expression is a valid partial index predicate. If -// the expression is valid, it returns the serialized expression with the -// columns dequalified. +// ValidatePartialIndexPredicate verifies that an expression is a valid partial +// index predicate. If the expression is valid, it returns the serialized +// expression with the columns dequalified. // // A predicate expression is valid if all of the following are true: // @@ -59,16 +33,22 @@ func MakeIndexPredicateValidator( // - It does not include non-immutable, aggregate, window, or set returning // functions. // -func (v *IndexPredicateValidator) Validate(e tree.Expr) (string, error) { +func ValidatePartialIndexPredicate( + ctx context.Context, + desc catalog.TableDescriptor, + e tree.Expr, + tn *tree.TableName, + semaCtx *tree.SemaContext, +) (string, error) { expr, _, err := DequalifyAndValidateExpr( - v.ctx, - v.desc, + ctx, + desc, e, types.Bool, "index predicate", - v.semaCtx, + semaCtx, tree.VolatilityImmutable, - &v.tableName, + tn, ) if err != nil { return "", err diff --git a/pkg/sql/catalog/schemaexpr/partial_index_test.go b/pkg/sql/catalog/schemaexpr/partial_index_test.go index 3b7a0a788e59..97c882ba7ee7 100644 --- a/pkg/sql/catalog/schemaexpr/partial_index_test.go +++ b/pkg/sql/catalog/schemaexpr/partial_index_test.go @@ -38,8 +38,6 @@ func TestIndexPredicateValidator_Validate(t *testing.T) { []testCol{{"c", types.String}}, ) - validator := schemaexpr.MakeIndexPredicateValidator(ctx, tn, desc, &semaCtx) - testData := []struct { expr string expectedValid bool @@ -91,7 +89,9 @@ func TestIndexPredicateValidator_Validate(t *testing.T) { t.Fatalf("%s: unexpected error: %s", d.expr, err) } - deqExpr, err := validator.Validate(expr) + deqExpr, err := schemaexpr.ValidatePartialIndexPredicate( + ctx, desc, expr, &tn, &semaCtx, + ) if !d.expectedValid { if err == nil { diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go index 9d8bf3ebec2e..f02a40708036 100644 --- a/pkg/sql/create_index.go +++ b/pkg/sql/create_index.go @@ -242,8 +242,9 @@ func MakeIndexDescriptor( } if n.Predicate != nil { - idxValidator := schemaexpr.MakeIndexPredicateValidator(params.ctx, n.Table, tableDesc, ¶ms.p.semaCtx) - expr, err := idxValidator.Validate(n.Predicate) + expr, err := schemaexpr.ValidatePartialIndexPredicate( + params.ctx, tableDesc, n.Predicate, &n.Table, params.p.SemaCtx(), + ) if err != nil { return nil, err } diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index d886462c9ce9..10a1ccdf17aa 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -1879,7 +1879,6 @@ func NewTableDesc( return newColumns, nil } - idxValidator := schemaexpr.MakeIndexPredicateValidator(ctx, n.Table, &desc, semaCtx) for _, def := range n.Defs { switch d := def.(type) { case *tree.ColumnTableDef, *tree.LikeTableDef: @@ -1964,7 +1963,9 @@ func NewTableDesc( } } if d.Predicate != nil { - expr, err := idxValidator.Validate(d.Predicate) + expr, err := schemaexpr.ValidatePartialIndexPredicate( + ctx, &desc, d.Predicate, &n.Table, semaCtx, + ) if err != nil { return nil, err } @@ -2061,7 +2062,9 @@ func NewTableDesc( } } if d.Predicate != nil { - expr, err := idxValidator.Validate(d.Predicate) + expr, err := schemaexpr.ValidatePartialIndexPredicate( + ctx, &desc, d.Predicate, &n.Table, semaCtx, + ) if err != nil { return nil, err }