sql: adding version gates for changes related to JSON forward indexes #101932

Shivs11 · 2023-04-20T17:58:31Z

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.

cockroach-teamcity · 2023-04-20T17:58:40Z

This change is

rafiss · 2023-04-24T12:56:24Z

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


we shouldn't just rewrite these to false. instead, change the assertion so it's SIMILAR TO 1000023.2-%

Shivs11

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @mgartner, and @rafiss)

pkg/sql/logictest/testdata/logic_test/mixed_version_can_login line 26 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we shouldn't just rewrite these to false. instead, change the assertion so it's SIMILAR TO 1000023.2-%

I don't think this is required anymore since the new version to be added still has the minor number to be set to 1. Thus, running this test with 1000023.1 worked unless this itself is wrong.

dt · 2023-04-25T18:03:00Z

pkg/upgrade/upgrades/upgrades.go

+		"enable JSON forward indexes and ORDER BY on JSON columns",
+		toCV(clusterversion.V23_2_JSONForwardIndexes),
+		upgrade.NoPrecondition,
+		NoTenantUpgradeFunc,


why do we need an upgrade at all?

Shivs11

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @mgartner, and @rafiss)

pkg/upgrade/upgrades/upgrades.go line 317 at r2 (raw file):

Previously, dt (David Taylor) wrote…

why do we need an upgrade at all?

Not sure if I understand your question to the fullest, but do you mean why we need an upgrade to this version? I believe this is required since each node in a given cluster has to be at this specific cluster version before it can start ordering/creating forward indexes on JSON columns. Thus, I wanted to add some checks in the code based on the cluster version number.

dt

It isn't clear to me that this needs a new version minted versus just having the usage gates be on IsActive(V23_1) ?

Either way, even if it does need to mint a new version, then I don't think you need the no-op upgrade.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @mgartner, and @rafiss)

rharding6373

@dt We haven't backported Shiv's JSON forward index support to 23.1 and only plan to have it available on 23.2. Please correct me if I'm wrong, but my understanding is that gating on IsActive(V23_1) would allow usage on 23.1, which is not what we want. I could see gating on IsActive(V23_2), except it seems we're at a point in this release cycle where that version hasn't been created yet.

Gating this feature, which doesn't require any upgrade steps, correctly has been confusing, so we're happy to get input on the best way to do it.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @mgartner, and @rafiss)

rharding6373

TIL Thank you!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @mgartner, and @rafiss)

Shivs11

Thank for the great pointers everyone! The changes have been made and it is RFAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @mgartner, and @rafiss)

dt · 2023-04-27T17:54:27Z

pkg/upgrade/upgrades/json_forward_indexes_test.go

+	// 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())


can this just be V23_1?

rharding6373

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @mgartner, @rafiss, and @Shivs11)

pkg/upgrade/upgrades/json_forward_indexes_test.go line 27 at r4 (raw file):

)

func TestJSONForwardingIndexes(t *testing.T) {

I was thinking about how what is really desirable to test is mixed versions -- which the version upgrade tests do, and happened to see Renato's fix for the version upgrade test that was failing with json forward indexes: https://github.com/cockroachdb/cockroach/pull/101638/files

There's a small bug there, which is that we should check for V23_2, not V23_1, for json forwarding indexes. If you have time do you mind adding a fix in this PR or a new one?

pkg/upgrade/upgrades/json_forward_indexes_test.go line 83 at r4 (raw file):

	)`)
	require.Error(t, err)

Can we also upgrade to 23.2 here and check that the previous commands succeed?

rharding6373

I'm picking up Shiv's PR to bring it over the finish line. I added the version gate for JSON forward indexes to the declarative schema changer (which supports CREATE INDEX as of 23.1) and a minor fix to operation_generator since JSON forward indexes are enabled in a different version than ARRAY. PTAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @mgartner, @rafiss, @smg260, and @srosenberg)

pkg/upgrade/upgrades/json_forward_indexes_test.go line 27 at r4 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

I was thinking about how what is really desirable to test is mixed versions -- which the version upgrade tests do, and happened to see Renato's fix for the version upgrade test that was failing with json forward indexes: https://github.com/cockroachdb/cockroach/pull/101638/files

There's a small bug there, which is that we should check for V23_2, not V23_1, for json forwarding indexes. If you have time do you mind adding a fix in this PR or a new one?

Done.

pkg/upgrade/upgrades/json_forward_indexes_test.go line 58 at r4 (raw file):

Previously, dt (David Taylor) wrote…

can this just be V23_1?

Thanks for the suggestion. We found that it couldn't, and it took us a long time to realize it was because create index uses the declarative schema changer in 23.1 and therefore a different path to create the index that we hadn't protected with the version gate. We now have tests for V22_2 and V23_1 to cover both possible paths.

pkg/upgrade/upgrades/json_forward_indexes_test.go line 83 at r4 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Can we also upgrade to 23.2 here and check that the previous commands succeed?

Done.

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 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.

rharding6373

Turns out someone already fixed operation_generator since I last reviewed this :-) RFAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @mgartner, @rafiss, @smg260, and @srosenberg)

fqazi

Reviewed 3 of 19 files at r1, 1 of 2 files at r6, 6 of 6 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, and @rafiss)

rharding6373 · 2023-05-19T17:28:59Z

Last call for comments! If there are no objections I will try to merge this EOD.

rharding6373 · 2023-05-22T21:46:29Z

bors r=fqazi

craig · 2023-05-22T22:35:40Z

Build succeeded:

Bazel Essential CI (Cockroach)

Shivs11 force-pushed the adding_cluster_version_number branch 3 times, most recently from b52df42 to e2f02db Compare April 20, 2023 18:03

Shivs11 requested review from mgartner, fqazi and a team April 20, 2023 18:05

Shivs11 marked this pull request as ready for review April 20, 2023 18:05

Shivs11 requested a review from a team April 20, 2023 18:05

Shivs11 marked this pull request as draft April 20, 2023 18:57

Shivs11 force-pushed the adding_cluster_version_number branch 4 times, most recently from 3671e0b to 5e32b80 Compare April 21, 2023 17:45

rafiss reviewed Apr 24, 2023

View reviewed changes

Shivs11 force-pushed the adding_cluster_version_number branch 2 times, most recently from b3790b9 to a9401c3 Compare April 24, 2023 16:24

Shivs11 commented Apr 24, 2023

View reviewed changes

Shivs11 force-pushed the adding_cluster_version_number branch 2 times, most recently from 1e16696 to 107903c Compare April 24, 2023 16:58

Shivs11 marked this pull request as ready for review April 24, 2023 16:58

Shivs11 force-pushed the adding_cluster_version_number branch 3 times, most recently from f342320 to e287ba9 Compare April 24, 2023 19:36

Shivs11 requested a review from rafiss April 24, 2023 20:14

dt reviewed Apr 25, 2023

View reviewed changes

Shivs11 commented Apr 25, 2023

View reviewed changes

Shivs11 requested a review from dt April 26, 2023 14:40

dt reviewed Apr 26, 2023

View reviewed changes

rharding6373 reviewed Apr 26, 2023

View reviewed changes

Shivs11 force-pushed the adding_cluster_version_number branch 2 times, most recently from 5ee49ff to 676c402 Compare April 26, 2023 20:25

Shivs11 commented Apr 26, 2023

View reviewed changes

Shivs11 requested review from dt and rharding6373 April 27, 2023 17:47

dt reviewed Apr 27, 2023

View reviewed changes

rharding6373 reviewed Apr 27, 2023

View reviewed changes

rharding6373 force-pushed the adding_cluster_version_number branch from 676c402 to a7ecb81 Compare May 15, 2023 22:00

rharding6373 requested a review from a team as a code owner May 15, 2023 22:00

rharding6373 force-pushed the adding_cluster_version_number branch 2 times, most recently from 1f534a6 to 30fc318 Compare May 15, 2023 22:28

rharding6373 requested a review from a team as a code owner May 15, 2023 22:28

rharding6373 requested review from srosenberg and smg260 and removed request for a team May 15, 2023 22:28

rharding6373 reviewed May 15, 2023

View reviewed changes

rharding6373 force-pushed the adding_cluster_version_number branch from 30fc318 to 33a903e Compare May 15, 2023 22:37

rharding6373 reviewed May 15, 2023

View reviewed changes

rharding6373 requested review from dt and removed request for srosenberg and smg260 May 15, 2023 22:40

fqazi approved these changes May 16, 2023

View reviewed changes

fqazi mentioned this pull request May 17, 2023

roachtest: acceptance/mixed version flaked on master #103452

Closed

craig bot merged commit a1b8517 into cockroachdb:master May 22, 2023

cockroach-teamcity mentioned this pull request May 23, 2023

PR #101932 - sql: adding version gates for changes related to JSON forward indexes cockroachdb/docs#17076

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: adding version gates for changes related to JSON forward indexes #101932

sql: adding version gates for changes related to JSON forward indexes #101932

Shivs11 commented Apr 20, 2023 •

edited by rharding6373

Loading

cockroach-teamcity commented Apr 20, 2023

rafiss Apr 24, 2023 •

edited

Loading

Shivs11 left a comment

dt Apr 25, 2023

Shivs11 left a comment

dt left a comment

rharding6373 left a comment

rharding6373 left a comment

Shivs11 left a comment

dt Apr 27, 2023

rharding6373 left a comment

rharding6373 left a comment

rharding6373 left a comment

fqazi left a comment

rharding6373 commented May 19, 2023

rharding6373 commented May 22, 2023

craig bot commented May 22, 2023

sql: adding version gates for changes related to JSON forward indexes #101932

sql: adding version gates for changes related to JSON forward indexes #101932

Conversation

Shivs11 commented Apr 20, 2023 • edited by rharding6373 Loading

cockroach-teamcity commented Apr 20, 2023

rafiss Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Shivs11 left a comment

Choose a reason for hiding this comment

dt Apr 25, 2023

Choose a reason for hiding this comment

Shivs11 left a comment

Choose a reason for hiding this comment

dt left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

Shivs11 left a comment

Choose a reason for hiding this comment

dt Apr 27, 2023

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

rharding6373 commented May 19, 2023

rharding6373 commented May 22, 2023

craig bot commented May 22, 2023

Shivs11 commented Apr 20, 2023 •

edited by rharding6373

Loading

rafiss Apr 24, 2023 •

edited

Loading