From e287ba94b3932926721f4950f7a5caec5f076fc8 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 #97298 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 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. Fixes: #35706 --- .../settings/settings-for-tenants.txt | 2 +- docs/generated/settings/settings.html | 2 +- pkg/clusterversion/cockroach_versions.go | 8 ++ pkg/sql/create_index.go | 35 +++++- pkg/sql/create_table.go | 17 ++- .../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 --- pkg/upgrade/upgrades/BUILD.bazel | 1 + .../upgrades/json_forward_indexes_test.go | 107 ++++++++++++++++++ pkg/upgrade/upgrades/upgrades.go | 6 + 12 files changed, 178 insertions(+), 19 deletions(-) create mode 100644 pkg/upgrade/upgrades/json_forward_indexes_test.go diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index 5e3d61fd422b..9630a8afd32d 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -293,4 +293,4 @@ trace.opentelemetry.collector string address of an OpenTelemetry trace collecto trace.snapshot.rate duration 0s if non-zero, interval at which background trace snapshots are captured tenant-rw trace.span_registry.enabled boolean true if set, ongoing traces can be seen at https:///#/debug/tracez tenant-rw trace.zipkin.collector string the address of a Zipkin instance to receive traces, as :. If no port is specified, 9411 will be used. tenant-rw -version version 1000023.1-2 set the active cluster version in the format '.' tenant-rw +version version 1000023.1-4 set the active cluster version in the format '.' tenant-rw diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index cdbc7f5a46c0..f6c89469928e 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -246,6 +246,6 @@
trace.snapshot.rate
duration0sif non-zero, interval at which background trace snapshots are capturedServerless/Dedicated/Self-Hosted
trace.span_registry.enabled
booleantrueif set, ongoing traces can be seen at https://<ui>/#/debug/tracezServerless/Dedicated/Self-Hosted
trace.zipkin.collector
stringthe address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.Serverless/Dedicated/Self-Hosted -
version
version1000023.1-2set the active cluster version in the format '<major>.<minor>'Serverless/Dedicated/Self-Hosted +
version
version1000023.1-4set the active cluster version in the format '<major>.<minor>'Serverless/Dedicated/Self-Hosted diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index 09b66ee7830c..9c89a8f6bebf 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -540,6 +540,10 @@ const ( // the process of upgrading from previous supported releases to 23.2. V23_2Start + // V23_2_JSONForwardIndexes enables forward indexing for JSON columns, along with a + // lexicographical ordering for JSON columns, for all clusters with a version >= 23.2 + V23_2_JSONForwardIndexes + // ************************************************* // Step 1b: Add new version for 23.2 development here. // Do not add new versions to a patch release. @@ -942,6 +946,10 @@ var rawVersionsSingleton = keyedVersions{ Key: V23_2Start, Version: roachpb.Version{Major: 23, Minor: 1, Internal: 2}, }, + { + Key: V23_2_JSONForwardIndexes, + Version: roachpb.Version{Major: 23, Minor: 1, Internal: 4}, + }, // ************************************************* // Step 2b: Add new version gates for 23.2 development here. diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go index 8a57c5c51479..bce0271cc6d5 100644 --- a/pkg/sql/create_index.go +++ b/pkg/sql/create_index.go @@ -189,7 +189,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 } @@ -309,7 +309,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) @@ -326,6 +330,21 @@ 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.Less(clusterversion.ByKey(clusterversion.V23_2_JSONForwardIndexes)) { + 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) @@ -534,6 +553,18 @@ func replaceExpressionElemsWithVirtualCols( ) } + if typ.Family() == types.JsonFamily && version.Less(clusterversion.ByKey(clusterversion.V23_2_JSONForwardIndexes)) { + 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 104d243dffce..a96fdf83c24c 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -19,6 +19,7 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/build" + "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/docs" "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" @@ -1601,6 +1602,20 @@ func NewTableDesc( incTelemetryForNewColumn(d, col) } + // Version gates for enabling primary keys for JSONB columns + if col.Type.Family() == types.JsonFamily && d.PrimaryKey.IsPrimaryKey && + version.Less(clusterversion.ByKey(clusterversion.V23_2_JSONForwardIndexes)) { + 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 { @@ -1785,7 +1800,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 } idx := descpb.IndexDescriptor{ diff --git a/pkg/sql/logictest/testdata/logic_test/expression_index b/pkg/sql/logictest/testdata/logic_test/expression_index index 21206bdf7746..44d05000b3ee 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 f88a0f48db5a..09ec639394d1 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/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..2fb43cb4bc35 --- /dev/null +++ b/pkg/upgrade/upgrades/json_forward_indexes_test.go @@ -0,0 +1,107 @@ +// 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() + + // Setting an older cluster version and expecting an error when creating + // forward indexes on JSON columns. + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.V23_1WebSessionsTableHasUserIDColumn).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) + + // 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 primary key on a JSON column should result in an error. + _, err = db.Exec(`CREATE TABLE test.new ( + key JSONB PRIMARY KEY + )`) + require.Error(t, err) + + // Updating the cluster version to allow creation of forward indexes + // on JSON columns. + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.V23_2_JSONForwardIndexes).String()) + require.NoError(t, err) + + _, err = db.Exec(`CREATE INDEX on test.hello (j)`) + require.NoError(t, err) + + _, err = db.Exec(`INSERT INTO test.hello VALUES (1, '[1, 2, 3]'::JSONB)`) + require.NoError(t, err) + + _, err = db.Exec(`CREATE INDEX on test.hello ((j->'a'))`) + require.NoError(t, err) + + _, err = db.Exec(`INSERT INTO test.hello VALUES (2, '{"a": "b"}'::JSONB)`) + require.NoError(t, err) + + // Creating a primary key on a JSON column. + _, err = db.Exec(`CREATE TABLE test.new ( + key JSONB PRIMARY KEY + )`) + require.NoError(t, err) +} diff --git a/pkg/upgrade/upgrades/upgrades.go b/pkg/upgrade/upgrades/upgrades.go index 1d2faddea175..00255f026133 100644 --- a/pkg/upgrade/upgrades/upgrades.go +++ b/pkg/upgrade/upgrades/upgrades.go @@ -310,6 +310,12 @@ var upgrades = []upgradebase.Upgrade{ createActivityUpdateJobMigration, "create statement_activity and transaction_activity job", ), + upgrade.NewTenantUpgrade( + "enable JSON forward indexes and ORDER BY on JSON columns", + toCV(clusterversion.V23_2_JSONForwardIndexes), + upgrade.NoPrecondition, + NoTenantUpgradeFunc, + ), } func init() {