Merge cockroachdb#101932

101932: sql: adding version gates for changes related to JSON forward indexes r=fqazi a=Shivs11

Previously, cockroachdb#99275 and cockroachdb#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](https://cockroachlabs.atlassian.net/browse/CRDB-24501)

Fixes: cockroachdb#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.

Co-authored-by: Shivam Saraf <[email protected]>

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(

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) &&

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,

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)
 

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)

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(),

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",

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)
+}