From 33a903e3eb874ef3d2fb3d09e3ea6d2ff086aa6c Mon Sep 17 00:00:00 2001 From: Shivam Saraf Date: Thu, 20 Apr 2023 13:47:00 -0400 Subject: [PATCH] sql: adding version gates for changes related to JSON forward indexes Previously, #99275 and #97928 were responsible for laying the foundations of a JSON lexicographical order as well as enabling JSON forward indexes in CRDB. However, these commits did not account for version gates. In a given cluster, all of its nodes are required to be at a certain version number for creating JSON forward indexes as well as for ordering JSON columns. Thus, this PR aims to enable version gates for ensuring all nodes in a given cluster meet this requirement. Epic: CRDB-24501 Fixes: #35706 Release note (sql change): This PR adds version gates which requires all nodes in a given cluster to have a minimum binary version number which in turn is required for creating forward indexes on JSON columns and for ordering JSON columns. --- pkg/sql/create_index.go | 34 +++- pkg/sql/create_table.go | 15 +- .../testdata/logic_test/expression_index | 2 + pkg/sql/logictest/testdata/logic_test/json | 1 + .../logictest/testdata/logic_test/json_index | 2 + .../local-mixed-22.2-23.1/generated_test.go | 14 -- .../internal/scbuildstmt/create_index.go | 4 +- pkg/upgrade/upgrades/BUILD.bazel | 1 + .../upgrades/json_forward_indexes_test.go | 147 ++++++++++++++++++ 9 files changed, 202 insertions(+), 18 deletions(-) create mode 100644 pkg/upgrade/upgrades/json_forward_indexes_test.go diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go index 2cde1b4a8915..0e78c89cf977 100644 --- a/pkg/sql/create_index.go +++ b/pkg/sql/create_index.go @@ -195,7 +195,7 @@ func makeIndexDescriptor( return nil, pgerror.Newf(pgcode.DuplicateRelation, "index with name %q already exists", n.Name) } - if err := checkIndexColumns(tableDesc, columns, n.Storing, n.Inverted); err != nil { + if err := checkIndexColumns(tableDesc, columns, n.Storing, n.Inverted, params.ExecCfg().Settings.Version.ActiveVersion(params.ctx)); err != nil { return nil, err } @@ -319,7 +319,11 @@ func makeIndexDescriptor( } func checkIndexColumns( - desc catalog.TableDescriptor, columns tree.IndexElemList, storing tree.NameList, inverted bool, + desc catalog.TableDescriptor, + columns tree.IndexElemList, + storing tree.NameList, + inverted bool, + version clusterversion.ClusterVersion, ) error { for i, colDef := range columns { col, err := catalog.MustFindColumnByTreeName(desc, colDef.Column) @@ -336,6 +340,20 @@ func checkIndexColumns( return pgerror.New(pgcode.DatatypeMismatch, "operator classes are only allowed for the last column of an inverted index") } + + // Checking if JSON Columns can be forward indexed for a given cluster version. + if col.GetType().Family() == types.JsonFamily && !inverted && !version.IsActive(clusterversion.V23_2) { + return errors.WithHint( + pgerror.Newf( + pgcode.InvalidTableDefinition, + "index element %s of type %s is not indexable in a non-inverted index", + col.GetName(), + col.GetType().Name(), + ), + "you may want to create an inverted index instead. See the documentation for inverted indexes: "+docs.URL("inverted-indexes.html"), + ) + } + } for i, colName := range storing { col, err := catalog.MustFindColumnByTreeName(desc, colName) @@ -544,6 +562,18 @@ func replaceExpressionElemsWithVirtualCols( ) } + if typ.Family() == types.JsonFamily && !version.IsActive(clusterversion.V23_2) { + return errors.WithHint( + pgerror.Newf( + pgcode.InvalidTableDefinition, + "index element %s of type %s is not indexable in a non-inverted index", + elem.Expr.String(), + typ.Name(), + ), + "you may want to create an inverted index instead. See the documentation for inverted indexes: "+docs.URL("inverted-indexes.html"), + ) + } + if !isInverted && !colinfo.ColumnTypeIsIndexable(typ) { if colinfo.ColumnTypeIsInvertedIndexable(typ) { return errors.WithHint( diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 344c77c0ed5b..97c898b4ba35 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -1602,6 +1602,19 @@ func NewTableDesc( incTelemetryForNewColumn(d, col) } + // Version gates for enabling primary keys for JSONB columns + if col.Type.Family() == types.JsonFamily && d.PrimaryKey.IsPrimaryKey && !version.IsActive(clusterversion.V23_2) { + return nil, errors.WithHint( + pgerror.Newf( + pgcode.InvalidTableDefinition, + "index element %s of type %s is not indexable in a non-inverted index", + col.Name, + col.Type.Name(), + ), + "you may want to create an inverted index instead. See the documentation for inverted indexes: "+docs.URL("inverted-indexes.html"), + ) + } + desc.AddColumn(col) if idx != nil { @@ -1786,7 +1799,7 @@ func NewTableDesc( ); err != nil { return nil, err } - if err := checkIndexColumns(&desc, d.Columns, d.Storing, d.Inverted); err != nil { + if err := checkIndexColumns(&desc, d.Columns, d.Storing, d.Inverted, version); err != nil { return nil, err } if !version.IsActive(clusterversion.V23_2_PartiallyVisibleIndexes) && diff --git a/pkg/sql/logictest/testdata/logic_test/expression_index b/pkg/sql/logictest/testdata/logic_test/expression_index index 2c0c28491558..06881654032d 100644 --- a/pkg/sql/logictest/testdata/logic_test/expression_index +++ b/pkg/sql/logictest/testdata/logic_test/expression_index @@ -1,3 +1,5 @@ +# LogicTest: default-configs !local-mixed-22.2-23.1 + statement ok CREATE TABLE t ( k INT PRIMARY KEY, diff --git a/pkg/sql/logictest/testdata/logic_test/json b/pkg/sql/logictest/testdata/logic_test/json index 9aede9c08c56..7d8fd3f71e58 100644 --- a/pkg/sql/logictest/testdata/logic_test/json +++ b/pkg/sql/logictest/testdata/logic_test/json @@ -182,6 +182,7 @@ SELECT bar FROM foo WHERE bar->'a' = '"b"'::JSON statement ok SELECT bar FROM foo ORDER BY bar +skipif config local-mixed-22.2-23.1 statement ok CREATE TABLE pk (k JSON PRIMARY KEY) diff --git a/pkg/sql/logictest/testdata/logic_test/json_index b/pkg/sql/logictest/testdata/logic_test/json_index index 3f679a330cab..e27bd0e38ff0 100644 --- a/pkg/sql/logictest/testdata/logic_test/json_index +++ b/pkg/sql/logictest/testdata/logic_test/json_index @@ -1,3 +1,5 @@ +# LogicTest: default-configs !local-mixed-22.2-23.1 + # Add JSON columns as primary index. statement ok CREATE TABLE t (x JSONB PRIMARY KEY) 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 b39e3c51742f..82cefcd5c932 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 @@ -744,13 +744,6 @@ func TestLogic_export( runLogicTest(t, "export") } -func TestLogic_expression_index( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "expression_index") -} - func TestLogic_external_connection_privileges( t *testing.T, ) { @@ -1059,13 +1052,6 @@ func TestLogic_json_builtins( runLogicTest(t, "json_builtins") } -func TestLogic_json_index( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "json_index") -} - func TestLogic_kv_builtin_functions( t *testing.T, ) { diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go index aaf4a8555d80..3ea71a294086 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go @@ -376,6 +376,7 @@ func processColNodeType( }) } // Only certain column types are supported for inverted indexes. + version := b.EvalCtx().Settings.Version.ActiveVersion(b) if n.Inverted && lastColIdx && !colinfo.ColumnTypeIsInvertedIndexable(columnType.Type) { colNameForErr := colName @@ -385,7 +386,8 @@ func processColNodeType( panic(tabledesc.NewInvalidInvertedColumnError(colNameForErr, columnType.Type.String())) } else if (!n.Inverted || !lastColIdx) && - !colinfo.ColumnTypeIsIndexable(columnType.Type) { + (!colinfo.ColumnTypeIsIndexable(columnType.Type) || + (columnType.Type.Family() == types.JsonFamily && !version.IsActive(clusterversion.V23_2))) { // Otherwise, check if the column type is indexable. panic(unimplemented.NewWithIssueDetailf(35730, columnType.Type.DebugString(), diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 403fd2d1bc56..560cf1be0e51 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -108,6 +108,7 @@ go_test( "ensure_sql_schema_telemetry_schedule_test.go", "external_connections_table_user_id_migration_test.go", "helpers_test.go", + "json_forward_indexes_test.go", "key_visualizer_migration_test.go", "main_test.go", "role_members_ids_migration_test.go", diff --git a/pkg/upgrade/upgrades/json_forward_indexes_test.go b/pkg/upgrade/upgrades/json_forward_indexes_test.go new file mode 100644 index 000000000000..1258b1f26491 --- /dev/null +++ b/pkg/upgrade/upgrades/json_forward_indexes_test.go @@ -0,0 +1,147 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package upgrades_test + +import ( + "context" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/server" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" + "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/stretchr/testify/require" +) + +func TestJSONForwardingIndexes(t *testing.T) { + var err error + skip.UnderStressRace(t) + defer leaktest.AfterTest(t)() + ctx := context.Background() + + settings := cluster.MakeTestingClusterSettingsWithVersions( + clusterversion.TestingBinaryVersion, + clusterversion.TestingBinaryMinSupportedVersion, + false, + ) + + tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{ + ServerArgs: base.TestServerArgs{ + Settings: settings, + Knobs: base.TestingKnobs{ + Server: &server.TestingKnobs{ + DisableAutomaticVersionUpgrade: make(chan struct{}), + BinaryVersionOverride: clusterversion.TestingBinaryMinSupportedVersion, + }, + }, + }, + }) + defer tc.Stopper().Stop(ctx) + + db := tc.ServerConn(0) + defer db.Close() + + // Set the cluster version to 22.2 to test that with the legacy schema changer + // we cannot create forward indexes on JSON columns. + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.V22_2).String()) + require.NoError(t, err) + + _, err = db.Exec(`CREATE DATABASE test`) + require.NoError(t, err) + + _, err = db.Exec(`CREATE TABLE test.hello ( + key INT PRIMARY KEY, + j JSONB +)`) + require.NoError(t, err) + + _, err = db.Exec(`INSERT INTO test.hello VALUES (1, '[{"a":"b"}]'::JSONB)`) + require.NoError(t, err) + + // Creating an index on the JSON column should result in an error. + _, err = db.Exec(`CREATE INDEX testidx_col on test.hello (j)`) + require.Error(t, err) + + // Creating an JSON expression index should result in an error. + _, err = db.Exec(`CREATE INDEX testidx_expr on test.hello ((j->'a'))`) + require.Error(t, err) + + // Creating a table with an index on the JSON column should result in an error. + _, err = db.Exec(`CREATE TABLE test.tbl_json_secondary( + j JSONB, + INDEX tbl_json_secondary_idx (j) +)`) + require.Error(t, err) + + // Creating a primary key on a JSON column should result in an error. + _, err = db.Exec(`CREATE TABLE test.tbl_json_primary ( + key JSONB PRIMARY KEY + )`) + require.Error(t, err) + + // Set the cluster version to 23.1 to test that with the declarative schema + // changer we cannot create forward indexes on JSON columns. + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.V23_1).String()) + require.NoError(t, err) + + // Creating an index on the JSON column should result in an error. + _, err = db.Exec(`CREATE INDEX on test.hello (j)`) + require.Error(t, err) + + // Creating an JSON expression index should result in an error. + _, err = db.Exec(`CREATE INDEX on test.hello ((j->'a'))`) + require.Error(t, err) + + // Creating a table with an index on the JSON column should result in an error. + _, err = db.Exec(`CREATE TABLE test.tbl_json_secondary( + j JSONB, + INDEX tbl_json_secondary_idx (j) +)`) + require.Error(t, err) + + // Creating a primary key on a JSON column should result in an error. + _, err = db.Exec(`CREATE TABLE test.tbl_json_primary ( + key JSONB PRIMARY KEY + )`) + require.Error(t, err) + + // Setting a cluster version that supports forward indexes on JSON + // columns and expecting success when creating forward indexes. + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.V23_2).String()) + require.NoError(t, err) + + // Creating an index on the JSON column should not result in an error. + _, err = db.Exec(`CREATE INDEX on test.hello (j)`) + require.NoError(t, err) + + // Creating an JSON expression index should not result in an error. + _, err = db.Exec(`CREATE INDEX on test.hello ((j->'a'))`) + require.NoError(t, err) + + // Creating a table with an index on the JSON column should result in an error. + _, err = db.Exec(`CREATE TABLE test.tbl_json_secondary( + j JSONB, + INDEX tbl_json_secondary_idx (j) +)`) + require.NoError(t, err) + + // Creating a primary key on a JSON column should result in an error. + _, err = db.Exec(`CREATE TABLE test.tbl_json_primary ( + key JSONB PRIMARY KEY + )`) + require.NoError(t, err) +}