From db31fb4208e4c48be4c9eee08c9f7c2dbb827579 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Mon, 2 Oct 2023 02:07:01 -0600 Subject: [PATCH] sql: check unsupported types during schema changes and type/function creation It is necessary to check the cluster version when building an expression that references a newly-added type, in order to ensure that all nodes support that type. Previously, these checks were omitted for casts in the vectorized engine, expressions for partial index predicates and check constraints, function parameters and return types, and user-defined composite types. This patch adds version-checking for function/procedure parameters and return types, as well as for user-defined types. It also augments the type-checking logic for casts and type annotations with a version check; this handles the remaining cases. The execution-time checks for cast expressions are left untouched, just in case the new type-checking logic misses important cases. For now, these changes only apply to the `PG_LSN` type, which will be new in 23.2. A future commit will add support for `REFCURSOR`, and will need to use the same checks. Informs #111560 Release note: None --- pkg/sql/backfill/backfill.go | 2 + pkg/sql/create_type.go | 4 + .../testdata/logic_test/pg_lsn_mixed | 128 +++++++++++++++++- .../BUILD.bazel | 2 +- .../generated_test.go | 7 + .../local-mixed-22.2-23.1/generated_test.go | 7 - pkg/sql/opt/optbuilder/BUILD.bazel | 1 + pkg/sql/opt/optbuilder/create_function.go | 23 +++- pkg/sql/planner.go | 2 + .../scbuild/tree_context_builder.go | 1 + pkg/sql/sem/tree/BUILD.bazel | 1 + pkg/sql/sem/tree/type_check.go | 30 ++++ pkg/sql/types/types.go | 1 - 13 files changed, 192 insertions(+), 17 deletions(-) diff --git a/pkg/sql/backfill/backfill.go b/pkg/sql/backfill/backfill.go index 256e7a18a23d..aa541b86e46b 100644 --- a/pkg/sql/backfill/backfill.go +++ b/pkg/sql/backfill/backfill.go @@ -236,6 +236,7 @@ func (cb *ColumnBackfiller) InitForDistributedUse( // Set up a SemaContext to type check the default and computed expressions. semaCtx := tree.MakeSemaContext() semaCtx.TypeResolver = &resolver + semaCtx.Version = evalCtx.Settings.Version var err error defaultExprs, err = schemaexpr.MakeDefaultExprs( ctx, cb.added, &transform.ExprTransformContext{}, evalCtx, &semaCtx, @@ -670,6 +671,7 @@ func (ib *IndexBackfiller) InitForDistributedUse( // Set up a SemaContext to type check the default and computed expressions. semaCtx := tree.MakeSemaContext() semaCtx.TypeResolver = &resolver + semaCtx.Version = evalCtx.Settings.Version // Convert any partial index predicate strings into expressions. predicates, colExprs, referencedColumns, err = constructExprs( ctx, desc, ib.added, ib.cols, ib.addedCols, ib.computedCols, evalCtx, &semaCtx, diff --git a/pkg/sql/create_type.go b/pkg/sql/create_type.go index 83bd3910171b..741ba9778864 100644 --- a/pkg/sql/create_type.go +++ b/pkg/sql/create_type.go @@ -423,6 +423,10 @@ func CreateCompositeTypeDesc( if err != nil { return nil, err } + err = tree.CheckUnsupportedType(params.ctx, params.extendedEvalCtx.Settings.Version, typ) + if err != nil { + return nil, err + } if typ.UserDefined() { return nil, unimplemented.NewWithIssue(91779, "composite types that reference user-defined types not yet supported") diff --git a/pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed b/pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed index 41e80a673aa8..ff35d409e0da 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed +++ b/pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed @@ -1,8 +1,124 @@ -# LogicTest: local-mixed-22.2-23.1 -# TODO(otan): add tests for mixed 23.1-23.2. +# LogicTest: cockroach-go-testserver-upgrade-to-master -statement error must be finalized to use pg_lsn -SELECT '1010F/AAAA'::text::pg_lsn +statement ok +CREATE TABLE xy (x INT, y INT); -statement error pg_lsn not supported until version 23.2 -CREATE TABLE pg_lsn_table(id pg_lsn, val pg_lsn) +# ---------------------------------------------------------------------- +# Test PG_LSN references with all nodes running old binaries. +# ---------------------------------------------------------------------- + +# Cast to PG_LSN. +statement error pgcode 0A000 unimplemented: this syntax +SELECT 'foo'::PG_LSN; + +# Cast to PG_LSN using the vectorized engine. +statement error pgcode 0A000 unimplemented: this syntax +SELECT 'foo'::PG_LSN FROM generate_series(1, 100) LIMIT 1; + +# Table that references PG_LSN. +statement error pgcode 0A000 unimplemented: this syntax +CREATE TABLE t (x PG_LSN); + +# Add a PG_LSN column. +statement error pgcode 0A000 unimplemented: this syntax +ALTER TABLE xy ADD COLUMN curs PG_LSN; + +# Alter a column type to PG_LSN. +statement ok +SET enable_experimental_alter_column_type_general=true; + +statement error pgcode 0A000 unimplemented: this syntax +ALTER TABLE xy ALTER COLUMN y TYPE PG_LSN; + +# Create a partial index that uses the PG_LSN type. +statement error pgcode 0A000 unimplemented: this syntax +CREATE INDEX part ON xy (x) WHERE y::PG_LSN < 'foo'; + +# Add a check constraint that uses the PG_LSN type. +statement error pgcode 0A000 unimplemented: this syntax +ALTER TABLE xy ADD CONSTRAINT bar CHECK (y::PG_LSN < 'baz'); + +# UDT that references PG_LSN. +statement error pgcode 0A000 unimplemented: this syntax +CREATE TYPE typ AS (x INT, y PG_LSN); + +# Function that returns PG_LSN. +statement error pgcode 0A000 unimplemented: this syntax +CREATE OR REPLACE FUNCTION f() RETURNS PG_LSN AS $$ + SELECT 'foo'; +$$ LANGUAGE SQL; + +# Function that takes PG_LSN argument. +statement error pgcode 0A000 unimplemented: this syntax +CREATE OR REPLACE FUNCTION f(curs PG_LSN) RETURNS STRING AS $$ + SELECT curs; +$$ LANGUAGE SQL; + +# Function that references PG_LSN internally. +statement error pgcode 0A000 unimplemented: this syntax +CREATE OR REPLACE FUNCTION f() RETURNS INT AS $$ + SELECT 'foo'::PG_LSN; + SELECT 0; +$$ LANGUAGE SQL; + +# ---------------------------------------------------------------------- +# Verify that PG_LSN is not allowed after upgrading the gateway. +# ---------------------------------------------------------------------- + +upgrade 0 + +user root nodeidx=0 + +# Cast to PG_LSN. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +SELECT 'foo'::PG_LSN; + +# Cast to PG_LSN using the vectorized engine. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +SELECT 'foo'::PG_LSN FROM generate_series(1, 100) LIMIT 1; + +# Table that references PG_LSN. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +CREATE TABLE t (x PG_LSN); + +# Add a PG_LSN column. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +ALTER TABLE xy ADD COLUMN curs PG_LSN; + +# Alter a column type to PG_LSN. +statement ok +SET enable_experimental_alter_column_type_general=true; + +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +ALTER TABLE xy ALTER COLUMN y TYPE PG_LSN; + +# Create a partial index that uses the PG_LSN type. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +CREATE INDEX part ON xy (x) WHERE y::PG_LSN < 'foo'; + +# Add a check constraint that uses the PG_LSN type. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +ALTER TABLE xy ADD CONSTRAINT bar CHECK (y::PG_LSN < 'baz'); + +# UDT that references PG_LSN. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +CREATE TYPE typ AS (x INT, y PG_LSN); + +# Function that returns PG_LSN. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +CREATE OR REPLACE FUNCTION f() RETURNS PG_LSN AS $$ + SELECT 'foo'; +$$ LANGUAGE SQL; + +# Function that takes PG_LSN argument. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +CREATE OR REPLACE FUNCTION f(curs PG_LSN) RETURNS INT AS $$ + SELECT 0; +$$ LANGUAGE SQL; + +# Function that references PG_LSN internally. +statement error pgcode 0A000 pq: pg_lsn not supported until version 23.2 +CREATE OR REPLACE FUNCTION f() RETURNS INT AS $$ + SELECT 'foo'::PG_LSN; + SELECT 0; +$$ LANGUAGE SQL; diff --git a/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/BUILD.bazel b/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/BUILD.bazel index 9e1a32cc3035..cee2384240f3 100644 --- a/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/BUILD.bazel +++ b/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/BUILD.bazel @@ -15,7 +15,7 @@ go_test( "//pkg/sql/logictest:testdata", # keep ], exec_properties = {"Pool": "large"}, - shard_count = 11, + shard_count = 12, tags = [ "cpu:2", ], diff --git a/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/generated_test.go b/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/generated_test.go index 8e13fd4eca21..2f79468f9058 100644 --- a/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/generated_test.go +++ b/pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/generated_test.go @@ -154,3 +154,10 @@ func TestLogic_mixed_version_upgrade_repair_descriptors( defer leaktest.AfterTest(t)() runLogicTest(t, "mixed_version_upgrade_repair_descriptors") } + +func TestLogic_pg_lsn_mixed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "pg_lsn_mixed") +} diff --git a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go index 39aa14d9c6af..cc2c282071d1 100644 --- a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go +++ b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go @@ -1317,13 +1317,6 @@ func TestLogic_pg_extension( runLogicTest(t, "pg_extension") } -func TestLogic_pg_lsn_mixed( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "pg_lsn_mixed") -} - func TestLogic_pgcrypto_builtins( t *testing.T, ) { diff --git a/pkg/sql/opt/optbuilder/BUILD.bazel b/pkg/sql/opt/optbuilder/BUILD.bazel index 9acbe42ef2e3..d84931a653be 100644 --- a/pkg/sql/opt/optbuilder/BUILD.bazel +++ b/pkg/sql/opt/optbuilder/BUILD.bazel @@ -50,6 +50,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder", visibility = ["//visibility:public"], deps = [ + "//pkg/clusterversion", "//pkg/kv/kvserver/concurrency/isolation", "//pkg/server/telemetry", "//pkg/settings", diff --git a/pkg/sql/opt/optbuilder/create_function.go b/pkg/sql/opt/optbuilder/create_function.go index de80d5cc743f..b76cd8439c20 100644 --- a/pkg/sql/opt/optbuilder/create_function.go +++ b/pkg/sql/opt/optbuilder/create_function.go @@ -11,8 +11,10 @@ package optbuilder import ( + "context" "fmt" + "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" @@ -139,6 +141,7 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateRoutine, inScope *scope) (o // named parameters to the scope so that references to them in the body can // be resolved. bodyScope := b.allocScope() + version := b.evalCtx.Settings.Version var paramTypes tree.ParamTypes for i := range cf.Params { param := &cf.Params[i] @@ -146,6 +149,8 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateRoutine, inScope *scope) (o if err != nil { panic(err) } + // The parameter type must be supported by the current cluster version. + checkUnsupportedType(b.ctx, version, typ) if types.IsRecordType(typ) { if language == tree.RoutineLangSQL { panic(pgerror.Newf(pgcode.InvalidFunctionDefinition, @@ -258,7 +263,7 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateRoutine, inScope *scope) (o // TODO(mgartner): stmtScope.cols does not describe the result // columns of the statement. We should use physical.Presentation // instead. - err = validateReturnType(funcReturnType, stmtScope.cols) + err = validateReturnType(b.ctx, version, funcReturnType, stmtScope.cols) if err != nil { panic(err) } @@ -301,7 +306,15 @@ func formatFuncBodyStmt(fmtCtx *tree.FmtCtx, ast tree.NodeFormatter, newLine boo fmtCtx.WriteString(";") } -func validateReturnType(expected *types.T, cols []scopeColumn) error { +func validateReturnType( + ctx context.Context, version clusterversion.Handle, expected *types.T, cols []scopeColumn, +) error { + // The return type must be supported by the current cluster version. + checkUnsupportedType(ctx, version, expected) + for i := range cols { + checkUnsupportedType(ctx, version, cols[i].typ) + } + // If return type is void, any column types are valid. if expected.Equivalent(types.Void) { return nil @@ -406,3 +419,9 @@ func checkStmtVolatility( } } } + +func checkUnsupportedType(ctx context.Context, version clusterversion.Handle, typ *types.T) { + if err := tree.CheckUnsupportedType(ctx, version, typ); err != nil { + panic(err) + } +} diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go index 6bd0b345192c..55936de09328 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -404,6 +404,7 @@ func newInternalPlanner( p.semaCtx.NameResolver = p p.semaCtx.DateStyle = sd.GetDateStyle() p.semaCtx.IntervalStyle = sd.GetIntervalStyle() + p.semaCtx.Version = execCfg.Settings.Version plannerMon := mon.NewMonitor(redact.Sprintf("internal-planner.%s.%s", user, opName), mon.MemoryResource, @@ -903,6 +904,7 @@ func (p *planner) resetPlanner( p.semaCtx.NameResolver = p p.semaCtx.DateStyle = sd.GetDateStyle() p.semaCtx.IntervalStyle = sd.GetIntervalStyle() + p.semaCtx.Version = p.execCfg.Settings.Version p.autoCommit = false diff --git a/pkg/sql/schemachanger/scbuild/tree_context_builder.go b/pkg/sql/schemachanger/scbuild/tree_context_builder.go index e5ee28360a0c..e9e2b7f5578e 100644 --- a/pkg/sql/schemachanger/scbuild/tree_context_builder.go +++ b/pkg/sql/schemachanger/scbuild/tree_context_builder.go @@ -36,6 +36,7 @@ func newSemaCtx(d Dependencies) *tree.SemaContext { semaCtx.NameResolver = d.CatalogReader() semaCtx.DateStyle = d.SessionData().GetDateStyle() semaCtx.IntervalStyle = d.SessionData().GetIntervalStyle() + semaCtx.Version = d.ClusterSettings().Version return &semaCtx } diff --git a/pkg/sql/sem/tree/BUILD.bazel b/pkg/sql/sem/tree/BUILD.bazel index 1751a68f67d4..3e77aaa647fa 100644 --- a/pkg/sql/sem/tree/BUILD.bazel +++ b/pkg/sql/sem/tree/BUILD.bazel @@ -122,6 +122,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/sql/sem/tree", visibility = ["//visibility:public"], deps = [ + "//pkg/clusterversion", "//pkg/col/coldata", "//pkg/col/typeconv", # keep "//pkg/geo", diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 0a01e31c40f2..8a6b70c5aab2 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -15,6 +15,7 @@ import ( "fmt" "strings" + "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/cast" @@ -75,6 +76,10 @@ type SemaContext struct { DateStyle pgdate.DateStyle // IntervalStyle refers to the IntervalStyle to parse as. IntervalStyle duration.IntervalStyle + + // Version accesses the current cluster version. It is used to verify that + // types are supported by the current cluster version. It may be unset. + Version clusterversion.Handle } // SemaProperties is a holder for required and derived properties @@ -599,6 +604,9 @@ func (expr *CastExpr) TypeCheck( if err != nil { return nil, err } + if err = CheckUnsupportedType(ctx, semaCtx.Version, exprType); err != nil { + return nil, err + } expr.Type = exprType canElideCast := true switch { @@ -739,6 +747,9 @@ func (expr *AnnotateTypeExpr) TypeCheck( if err != nil { return nil, err } + if err = CheckUnsupportedType(ctx, semaCtx.Version, annotateType); err != nil { + return nil, err + } expr.Type = annotateType subExpr, err := typeCheckAndRequire( ctx, @@ -3411,3 +3422,22 @@ func getMostSignificantOverload( } return ret, nil } + +// CheckUnsupportedType returns an error if the given type is not supported by +// the current cluster version. If the given version handle is nil, +// CheckUnsupportedType returns nil. +func CheckUnsupportedType(ctx context.Context, version clusterversion.Handle, typ *types.T) error { + if version == nil { + return nil + } + var errorTypeString string + if typ.Family() == types.PGLSNFamily { + errorTypeString = "pg_lsn" + } + if errorTypeString != "" && !version.IsActive(ctx, clusterversion.V23_2) { + return pgerror.Newf(pgcode.FeatureNotSupported, + "%s not supported until version 23.2", errorTypeString, + ) + } + return nil +} diff --git a/pkg/sql/types/types.go b/pkg/sql/types/types.go index 4b00a4be65a4..54bcb4f5e934 100644 --- a/pkg/sql/types/types.go +++ b/pkg/sql/types/types.go @@ -2941,7 +2941,6 @@ var postgresPredefinedTypeIssues = map[string]int{ "macaddr8": 45813, "money": 41578, "path": 21286, - "pg_lsn": -1, "txid_snapshot": -1, "xml": 43355, }