From 5e32b80c0134937604fc2b3493d12e31e8f1ac7e 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 +- .../testdata/logic_test/crdb_internal_tenant | 4 +- pkg/cli/testdata/declarative-rules/deprules | 2 +- pkg/cli/testdata/declarative-rules/oprules | 2 +- pkg/clusterversion/cockroach_versions.go | 8 ++ pkg/sql/create_index.go | 35 +++++- pkg/sql/create_table.go | 17 ++- .../testdata/logic_test/crdb_internal | 4 +- .../testdata/logic_test/expression_index | 2 + pkg/sql/logictest/testdata/logic_test/json | 1 + .../logictest/testdata/logic_test/json_index | 2 + .../logic_test/mixed_version_can_login | 8 +- .../logic_test/mixed_version_range_tombstones | 2 +- .../mixed_version_role_members_user_ids | 8 +- .../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 + 19 files changed, 193 insertions(+), 34 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..45dab6924b0c 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.2-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..18a52f5bd0ff 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.2-4set the active cluster version in the format '<major>.<minor>'Serverless/Dedicated/Self-Hosted diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant index 4a724a9e4ad8..56f3de8f4f84 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant @@ -391,7 +391,7 @@ select crdb_internal.get_vmodule() query T select regexp_replace(regexp_replace(crdb_internal.node_executable_version()::string, '(-\d+)?$', ''), '10000', ''); ---- -23.1 +23.2 query ITTT colnames select node_id, component, field, regexp_replace(regexp_replace(value, '^\d+$', ''), e':\\d+', ':') as value from crdb_internal.node_runtime_info @@ -484,7 +484,7 @@ select * from crdb_internal.gossip_alerts query T select regexp_replace(regexp_replace(crdb_internal.node_executable_version()::string, '(-\d+)?$', ''), '10000', ''); ---- -23.1 +23.2 user root diff --git a/pkg/cli/testdata/declarative-rules/deprules b/pkg/cli/testdata/declarative-rules/deprules index 1b9be021bb1d..73d2552ba94e 100644 --- a/pkg/cli/testdata/declarative-rules/deprules +++ b/pkg/cli/testdata/declarative-rules/deprules @@ -1,6 +1,6 @@ dep ---- -debug declarative-print-rules 1000023.1-2 dep +debug declarative-print-rules 1000023.2-4 dep deprules ---- - name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: PUBLIC->VALIDATED' diff --git a/pkg/cli/testdata/declarative-rules/oprules b/pkg/cli/testdata/declarative-rules/oprules index 3e188f0d5916..8f1437b2d6b2 100644 --- a/pkg/cli/testdata/declarative-rules/oprules +++ b/pkg/cli/testdata/declarative-rules/oprules @@ -1,6 +1,6 @@ op ---- -debug declarative-print-rules 1000023.1-2 op +debug declarative-print-rules 1000023.2-4 op rules ---- [] 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/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal index 6706dd4dbb77..f66ab26e8dda 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal @@ -569,7 +569,7 @@ select crdb_internal.get_vmodule() query T select regexp_replace(crdb_internal.node_executable_version()::string, '(-\d+)?$', ''); ---- -1000023.1 +1000023.2 query ITTT colnames select node_id, component, field, regexp_replace(regexp_replace(value, '^\d+$', ''), e':\\d+', ':') as value from crdb_internal.node_runtime_info @@ -723,7 +723,7 @@ SELECT * FROM crdb_internal.check_consistency(true, b'\x02', b'\x04') query T select regexp_replace(crdb_internal.node_executable_version()::string, '(-\d+)?$', ''); ---- -1000023.1 +1000023.2 user root 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/testdata/logic_test/mixed_version_can_login b/pkg/sql/logictest/testdata/logic_test/mixed_version_can_login index c55c2ba5f016..391d4a7a2509 100644 --- a/pkg/sql/logictest/testdata/logic_test/mixed_version_can_login +++ b/pkg/sql/logictest/testdata/logic_test/mixed_version_can_login @@ -23,7 +23,7 @@ upgrade 1 query B SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false query T nodeidx=2 SELECT crdb_internal.node_executable_version() @@ -36,7 +36,7 @@ user testuser nodeidx=0 query B SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false # Verify that a root user can login on the upgraded node. user root nodeidx=1 @@ -44,7 +44,7 @@ user root nodeidx=1 query B SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false # Verify that a non-root user can login on the non-upgraded node. user testuser nodeidx=2 @@ -67,7 +67,7 @@ upgrade 2 query B SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false statement ok CREATE TABLE bootstrapped_tenant_data AS diff --git a/pkg/sql/logictest/testdata/logic_test/mixed_version_range_tombstones b/pkg/sql/logictest/testdata/logic_test/mixed_version_range_tombstones index 9c52fa9ca0e1..b9d0a3e6053e 100644 --- a/pkg/sql/logictest/testdata/logic_test/mixed_version_range_tombstones +++ b/pkg/sql/logictest/testdata/logic_test/mixed_version_range_tombstones @@ -24,7 +24,7 @@ upgrade 0 query B nodeidx=0 SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false # Range tombstones remain disabled. diff --git a/pkg/sql/logictest/testdata/logic_test/mixed_version_role_members_user_ids b/pkg/sql/logictest/testdata/logic_test/mixed_version_role_members_user_ids index 874f6f535ce2..ba4a983b8d3b 100644 --- a/pkg/sql/logictest/testdata/logic_test/mixed_version_role_members_user_ids +++ b/pkg/sql/logictest/testdata/logic_test/mixed_version_role_members_user_ids @@ -49,7 +49,7 @@ upgrade 1 query B nodeidx=1 SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false user root nodeidx=1 @@ -85,14 +85,14 @@ upgrade 2 query B nodeidx=0 SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false query B nodeidx=1 SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false query B nodeidx=2 SELECT crdb_internal.node_executable_version() SIMILAR TO '1000023.1-%' ---- -true +false 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() {