From 2a118aadb7cc48b938c79a823fdbe4eda81c37c7 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 12 Dec 2022 22:27:31 -0500 Subject: [PATCH] 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. --- .../validate_system_schema_after_version_upgrade.go | 11 +++++------ .../upgrades/sampled_stmt_diagnostics_requests.go | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/roachtest/tests/validate_system_schema_after_version_upgrade.go b/pkg/cmd/roachtest/tests/validate_system_schema_after_version_upgrade.go index 89b73e0cca12..716593ea90e9 100644 --- a/pkg/cmd/roachtest/tests/validate_system_schema_after_version_upgrade.go +++ b/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 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) } } @@ -116,7 +115,7 @@ func registerValidateSystemSchemaAfterVersionUpgrade(r registry.Registry) { obtainSystemSchemaStep(1, &actual), // Compare the results. - validateEquivalenceStep(expected, actual), + validateEquivalenceStep(&expected, &actual), ) u.run(ctx, t) }, diff --git a/pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go b/pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go index 2abfc44ece1c..21b4007d675b 100644 --- a/pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go +++ b/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` )