upgrades: fix upgrade to add statement_diagnostics_requests.completed…

…_idx

The migration used a different column ordering than the descriptor in the
bootstrap schema. The value in the bootstrap schema is the value used to
determine whether the migration succeeded successfully. In general, you
can hit this bug if you upgrade from 22.1->22.2 and then you create the
index with the migration but crash before the index is fully created. In
that case, the code will think that it's the wrong index. This should be
rare, but would be problematic. Now we've made them match.

This change also fixes the roachtest which checks that the system
schema looks correct to check on what happens when you upgrade from a previous
snapshot. The problem with the test is that it read the strings before they
were assigned.

Fixes #93133

Release note (bug fix): Fixed a rare bug which could cause upgrades from 22.1
to 22.2 to fail if the job coordinator node crashes in the middle of a specific
upgrade migration.

pkg/cmd/roachtest/tests/validate_system_schema_after_version_upgrade.go

@@ -65,8 +65,7 @@ func registerValidateSystemSchemaAfterVersionUpgrade(r registry.Registry) {
 			}
 
 			// expected and actual output of `SHOW CREATE ALL TABLES;`.
-			var expected string
-			var actual string
+			var expected, actual, actualFromCheckpoint string
 
 			// Query node `SHOW CREATE ALL TABLES` and store return in output.
 			obtainSystemSchemaStep := func(node int, output *string) versionStep {
@@ -83,13 +82,13 @@ func registerValidateSystemSchemaAfterVersionUpgrade(r registry.Registry) {
 			}
 
 			// Compare whether two strings are equal -- used to compare expected and actual.
-			validateEquivalenceStep := func(str1, str2 string) versionStep {
+			validateEquivalenceStep := func(str1, str2 *string) versionStep {
 				return func(ctx context.Context, t test.Test, u *versionUpgradeTest) {
-					if str1 != str2 {
+					if *str1 != *str2 {
 						t.Fatal("After upgrading, `USE system; SHOW CREATE ALL TABLES;` " +
 							"does not match expected output after version upgrade.\n")
 					}
-					t.L().Printf("validating succeeded")
+					t.L().Printf("validating succeeded:\n%v", *str1)
 				}
 			}
 
@@ -103,6 +102,24 @@ func registerValidateSystemSchemaAfterVersionUpgrade(r registry.Registry) {
 				// Wipe the node.
 				wipeClusterStep(c.Node(1)),
 
+				// Obtain the actual from a node which got upgraded to the precessor.
+				uploadAndStartFromCheckpointFixture(c.Node(1), predecessorVersion),
+
+				// Wait for the upgrade to the predecessorVersion.
+				waitForUpgradeStep(c.Node(1)),
+
+				// Upgrade the node version.
+				binaryUpgradeStep(c.Node(1), mainVersion),
+
+				// Wait for the cluster version to also bump up to make sure the migration logic is run.
+				waitForUpgradeStep(c.Node(1)),
+
+				// Obtain the actual output on the upgraded version.
+				obtainSystemSchemaStep(1, &actualFromCheckpoint),
+
+				// Wipe the node again.
+				wipeClusterStep(c.Node(1)),
+
 				// Restart the node with a previous binary version.
 				uploadAndStart(c.Node(1), predecessorVersion),
 
@@ -116,7 +133,7 @@ func registerValidateSystemSchemaAfterVersionUpgrade(r registry.Registry) {
 				obtainSystemSchemaStep(1, &actual),
 
 				// Compare the results.
-				validateEquivalenceStep(expected, actual),
+				validateEquivalenceStep(&expected, &actual),
 			)
 			u.run(ctx, t)
 		},

pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go

@@ -31,7 +31,7 @@ ALTER TABLE system.statement_diagnostics_requests
 
 	createCompletedIdxV3 = `
 CREATE INDEX completed_idx ON system.statement_diagnostics_requests (completed, ID)
-  STORING (statement_fingerprint, sampling_probability, min_execution_latency, expires_at)`
+  STORING (statement_fingerprint, min_execution_latency, expires_at, sampling_probability)`
 
 	dropCompletedIdxV2 = `DROP INDEX IF EXISTS system.statement_diagnostics_requests@completed_idx_v2`
 )