From a570e41777a1e4dc4059e177102e1e31c063943e Mon Sep 17 00:00:00 2001 From: adityamaru Date: Tue, 4 Apr 2023 23:00:28 -0400 Subject: [PATCH] upgrades: delete userfile corruption version gate The userfile descriptor corruption version gate can be safely deleted as all clusters upgrading to 23.1+ are guaranteed to have been upgraded to 22.2 prior to that. Informs: #100552 Release note: None --- pkg/clusterversion/cockroach_versions.go | 8 -- pkg/upgrade/upgrades/BUILD.bazel | 1 - .../fix_userfile_descriptor_corruption.go | 57 --------- ...fix_userfile_descriptor_corruption_test.go | 110 ------------------ ...efore_starting_an_upgrade_external_test.go | 32 +++++ pkg/upgrade/upgrades/upgrades.go | 5 - 6 files changed, 32 insertions(+), 181 deletions(-) delete mode 100644 pkg/upgrade/upgrades/fix_userfile_descriptor_corruption_test.go diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index 64ca375f6b33..63bc3b960e99 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -276,10 +276,6 @@ const ( // TODODelete_V22_2SupportAssumeRoleAuth is the version where assume role authorization is // supported in cloud storage and KMS. TODODelete_V22_2SupportAssumeRoleAuth - // TODODelete_V22_2FixUserfileRelatedDescriptorCorruption adds a migration which uses - // heuristics to identify invalid table descriptors for userfile-related - // descriptors. - TODODelete_V22_2FixUserfileRelatedDescriptorCorruption // V22_2 is CockroachDB v22.2. It's used for all v22.2.x patch releases. V22_2 @@ -708,10 +704,6 @@ var rawVersionsSingleton = keyedVersions{ Key: TODODelete_V22_2SupportAssumeRoleAuth, Version: roachpb.Version{Major: 22, Minor: 1, Internal: 74}, }, - { - Key: TODODelete_V22_2FixUserfileRelatedDescriptorCorruption, - Version: roachpb.Version{Major: 22, Minor: 1, Internal: 76}, - }, { Key: V22_2, Version: roachpb.Version{Major: 22, Minor: 2, Internal: 0}, diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 47e85eb634ef..9e1d7cfc78e3 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -107,7 +107,6 @@ go_test( "descriptor_utils_test.go", "ensure_sql_schema_telemetry_schedule_test.go", "external_connections_table_user_id_migration_test.go", - "fix_userfile_descriptor_corruption_test.go", "helpers_test.go", "key_visualizer_migration_test.go", "main_test.go", diff --git a/pkg/upgrade/upgrades/fix_userfile_descriptor_corruption.go b/pkg/upgrade/upgrades/fix_userfile_descriptor_corruption.go index e50f7802306e..3b45bc6a79a8 100644 --- a/pkg/upgrade/upgrades/fix_userfile_descriptor_corruption.go +++ b/pkg/upgrade/upgrades/fix_userfile_descriptor_corruption.go @@ -14,70 +14,13 @@ import ( "context" "strings" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" - "github.com/cockroachdb/cockroach/pkg/upgrade" - "github.com/cockroachdb/cockroach/pkg/util/log" ) -// fixInvalidObjectsThatLookLikeBadUserfileConstraint attempts to remove a -// dangling table descriptor mutation for foreign key constraints on -// usefile-related upload_payload tables. -// -// It only proceeds with the fix if -// -// - the fk mutation it the _only_ mutation on the table, -// - the table name, column names, and column type all look like a userfile table, and -// - the fk is referencing a table whose name, column names, and column types all look like a userfile table. -// -// To fix the table, we remove the mutation as the FK constraint is unnecessary -// in the current implementation. -func fixInvalidObjectsThatLookLikeBadUserfileConstraint( - ctx context.Context, _ clusterversion.ClusterVersion, d upgrade.TenantDeps, -) error { - return d.DB.DescsTxn(ctx, func( - ctx context.Context, txn descs.Txn, - ) error { - query := `SELECT * FROM crdb_internal.invalid_objects` - rows, err := txn.QueryIterator(ctx, "find-invalid-descriptors", txn.KV(), query) - if err != nil { - return err - } - defer func() { _ = rows.Close() }() - - var hasNext bool - for hasNext, err = rows.Next(ctx); hasNext && err == nil; hasNext, err = rows.Next(ctx) { - // crdb_internal.invalid_objects has five columns: id, database name, schema name, table name, error. - row := rows.Cur() - tableID := descpb.ID(tree.MustBeDInt(row[0])) - errString := string(tree.MustBeDString(row[4])) - if veryLikelyKnownUserfileBreakage(ctx, txn.KV(), txn.Descriptors(), tableID, errString) { - log.Infof(ctx, "attempting to fix invalid table descriptor %d assuming it is a userfile-related table", tableID) - mutTableDesc, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, tableID) - if err != nil { - return err - } - mutTableDesc.Mutations = nil - mutTableDesc.MutationJobs = nil - if err := txn.Descriptors().WriteDesc(ctx, false, mutTableDesc, txn.KV()); err != nil { - return err - } - } - } - if err != nil { - // TODO(ssd): We always return a nil error here because I'm not sure that this - // would be worth failing an upgrade for. - log.Warningf(ctx, "could not fix broken userfile: %v", err) - } - return nil - }) -} - // veryLikelyKnownUserfileBreakage returns true if the given descriptor id and // error message from crdb_internal.invalid_objects is likely related to a known // userfile-related table corruption. diff --git a/pkg/upgrade/upgrades/fix_userfile_descriptor_corruption_test.go b/pkg/upgrade/upgrades/fix_userfile_descriptor_corruption_test.go deleted file mode 100644 index 49b70f690055..000000000000 --- a/pkg/upgrade/upgrades/fix_userfile_descriptor_corruption_test.go +++ /dev/null @@ -1,110 +0,0 @@ -// Copyright 2022 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/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" - "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" - "github.com/cockroachdb/cockroach/pkg/util/leaktest" - "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/stretchr/testify/require" -) - -// The hex for the descriptor to inject was created by running the following -// commands in a 22.1 binary. -// -// In 22.1 and prior, userfile creation had a bug that produced a foreign -// key constraint mutation without an associated mutation job. -// -// CREATE DATABASE to_backup; -// BACKUP DATABASE to_backup INTO 'userfile://defaultdb.test/data'; -// -// SELECT encode(descriptor, 'hex') -// FROM system.descriptor -// WHERE id = ( -// SELECT id -// FROM system.namespace -// WHERE name = 'test_upload_payload' -// ); -// SELECT encode(descriptor, 'hex') -// FROM system.descriptor -// WHERE id = ( -// SELECT id -// FROM system.namespace -// WHERE name = 'test_upload_files' -// ); -// -// The files table is not broken, but should probably be inserted for completeness. -// -// These are shared with the tests for the upgrade preconditions. -const ( - brokenUserfilePayloadTable = "0ac4040a13746573745f75706c6f61645f7061796c6f6164186b206428033a00422d0a0766696c655f696410011a0d080e10001800300050861760002000300068007000780080010088010098010042300a0b627974655f6f666673657410021a0c08011040180030005014600020003000680070007800800100880100980100422c0a077061796c6f616410031a0c0808100018003000501160002001300068007000780080010088010098010048045290010a18746573745f75706c6f61645f7061796c6f61645f706b657910011801220766696c655f6964220b627974655f6f66667365742a077061796c6f616430013002400040004a10080010001a00200028003000380040005a0070037a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80188a5978797a6b68c17d0010160026a210a0b0a0561646d696e100218020a0a0a04726f6f74100218021204726f6f74180272541801200128013800424a0801120a66696c655f69645f666b1a0c0a0012001800300038004003221e086b10011802206a2a0a66696c655f69645f666b3002380040004800700330003a0a08001a0020002a003003800102880103980100b201320a077072696d61727910001a0766696c655f69641a0b627974655f6f66667365741a077061796c6f61642001200220032803b80101c20100da010c080110818088ef93a5db8d0be80100f2010408001200f801008002009202009a020a0888a5978797a6b68c17b20200b80200c00265c80200e00200800300880304" - brokenUserfileFilesTable = "0a91060a11746573745f75706c6f61645f66696c6573186a206428033a00422d0a0866696c656e616d6510011a0c0807100018003000501960002000300068007000780080010088010098010042400a0766696c655f696410021a0d080e100018003000508617600020002a1167656e5f72616e646f6d5f7575696428293000680070007800800100880100980100422e0a0966696c655f73697a6510031a0c08011040180030005014600020003000680070007800800100880100980100422d0a08757365726e616d6510041a0c0807100018003000501960002000300068007000780080010088010098010042440a0b75706c6f61645f74696d6510051a0d080510001800300050da08600020012a116e6f7728293a3a3a54494d455354414d503000680070007800800100880100980100480652a6010a16746573745f75706c6f61645f66696c65735f706b657910011801220866696c656e616d652a0766696c655f69642a0966696c655f73697a652a08757365726e616d652a0b75706c6f61645f74696d65300140004a10080010001a00200028003000380040005a0070027003700470057a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80188a5978797a6b68c17d001025a7b0a1d746573745f75706c6f61645f66696c65735f66696c655f69645f6b657910021801220766696c655f69643002380140004a10080010001a00200028003000380040005a007a0408002000800100880100900103980100a20106080012001800a80100b20100ba0100c00100c80188a5978797a6b68c17d0010160036a210a0b0a0561646d696e100218020a0a0a04726f6f74100218021204726f6f741802800101880103980100b2014c0a077072696d61727910001a0866696c656e616d651a0766696c655f69641a0966696c655f73697a651a08757365726e616d651a0b75706c6f61645f74696d65200120022003200420052800b80101c20100e80100f2010408001200f801008002009202009a020a0888a5978797a6b68c17b20200b80200c00265c80200e00200800300880303" -) - -// TestFixUserfileRelatedDescriptorCorruptionUpgrade tests that we correctly repair -// a broken userfile table. -func TestFixUserfileRelatedDescriptorCorruptionUpgrade(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - - var ( - v0 = clusterversion.ByKey(clusterversion.TODODelete_V22_2FixUserfileRelatedDescriptorCorruption - 1) - v1 = clusterversion.ByKey(clusterversion.TODODelete_V22_2FixUserfileRelatedDescriptorCorruption) - ) - - ctx := context.Background() - settings := cluster.MakeTestingClusterSettingsWithVersions(v1, v0, false /* initializeVersion */) - require.NoError(t, clusterversion.Initialize(ctx, v0, &settings.SV)) - - tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{ - ServerArgs: base.TestServerArgs{ - Settings: settings, - Knobs: base.TestingKnobs{ - Server: &server.TestingKnobs{ - DisableAutomaticVersionUpgrade: make(chan struct{}), - BinaryVersionOverride: v0, - }, - }, - }, - }) - defer tc.Stopper().Stop(ctx) - - sqlDB := tc.ServerConn(0) - tdb := sqlutils.MakeSQLRunner(sqlDB) - - var parentID, parentSchemaID descpb.ID - tdb.Exec(t, "CREATE TABLE temp_tbl()") - tdb.QueryRow(t, `SELECT "parentID", "parentSchemaID" FROM system.namespace WHERE name = 'temp_tbl'`). - Scan(&parentID, &parentSchemaID) - - decodeTableDescriptorAndInsert(t, ctx, sqlDB, brokenUserfileFilesTable, parentID, parentSchemaID) - decodeTableDescriptorAndInsert(t, ctx, sqlDB, brokenUserfilePayloadTable, parentID, parentSchemaID) - var count int - err := sqlDB.QueryRow(`SELECT count(1) FROM "".crdb_internal.invalid_objects`).Scan(&count) - require.NoError(t, err) - require.Equal(t, 1, count) - - _, err = sqlDB.Exec(`SET CLUSTER SETTING version = $1`, v1.String()) - require.NoError(t, err) - err = sqlDB.QueryRow(`SELECT count(1) FROM "".crdb_internal.invalid_objects`).Scan(&count) - require.NoError(t, err) - require.Equal(t, 0, count) - tdb.CheckQueryResults(t, "SHOW CLUSTER SETTING version", [][]string{{v1.String()}}) -} diff --git a/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade_external_test.go b/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade_external_test.go index 82712b25b1d0..f5710d84caee 100644 --- a/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade_external_test.go +++ b/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade_external_test.go @@ -33,6 +33,38 @@ import ( "github.com/stretchr/testify/require" ) +// The hex for the descriptor to inject was created by running the following +// commands in a 22.1 binary. +// +// In 22.1 and prior, userfile creation had a bug that produced a foreign +// key constraint mutation without an associated mutation job. +// +// CREATE DATABASE to_backup; +// BACKUP DATABASE to_backup INTO 'userfile://defaultdb.test/data'; +// +// SELECT encode(descriptor, 'hex') +// FROM system.descriptor +// WHERE id = ( +// SELECT id +// FROM system.namespace +// WHERE name = 'test_upload_payload' +// ); +// SELECT encode(descriptor, 'hex') +// FROM system.descriptor +// WHERE id = ( +// SELECT id +// FROM system.namespace +// WHERE name = 'test_upload_files' +// ); +// +// The files table is not broken, but should probably be inserted for completeness. +// +// These are shared with the tests for the upgrade preconditions. +const ( + brokenUserfilePayloadTable = "0ac4040a13746573745f75706c6f61645f7061796c6f6164186b206428033a00422d0a0766696c655f696410011a0d080e10001800300050861760002000300068007000780080010088010098010042300a0b627974655f6f666673657410021a0c08011040180030005014600020003000680070007800800100880100980100422c0a077061796c6f616410031a0c0808100018003000501160002001300068007000780080010088010098010048045290010a18746573745f75706c6f61645f7061796c6f61645f706b657910011801220766696c655f6964220b627974655f6f66667365742a077061796c6f616430013002400040004a10080010001a00200028003000380040005a0070037a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80188a5978797a6b68c17d0010160026a210a0b0a0561646d696e100218020a0a0a04726f6f74100218021204726f6f74180272541801200128013800424a0801120a66696c655f69645f666b1a0c0a0012001800300038004003221e086b10011802206a2a0a66696c655f69645f666b3002380040004800700330003a0a08001a0020002a003003800102880103980100b201320a077072696d61727910001a0766696c655f69641a0b627974655f6f66667365741a077061796c6f61642001200220032803b80101c20100da010c080110818088ef93a5db8d0be80100f2010408001200f801008002009202009a020a0888a5978797a6b68c17b20200b80200c00265c80200e00200800300880304" + brokenUserfileFilesTable = "0a91060a11746573745f75706c6f61645f66696c6573186a206428033a00422d0a0866696c656e616d6510011a0c0807100018003000501960002000300068007000780080010088010098010042400a0766696c655f696410021a0d080e100018003000508617600020002a1167656e5f72616e646f6d5f7575696428293000680070007800800100880100980100422e0a0966696c655f73697a6510031a0c08011040180030005014600020003000680070007800800100880100980100422d0a08757365726e616d6510041a0c0807100018003000501960002000300068007000780080010088010098010042440a0b75706c6f61645f74696d6510051a0d080510001800300050da08600020012a116e6f7728293a3a3a54494d455354414d503000680070007800800100880100980100480652a6010a16746573745f75706c6f61645f66696c65735f706b657910011801220866696c656e616d652a0766696c655f69642a0966696c655f73697a652a08757365726e616d652a0b75706c6f61645f74696d65300140004a10080010001a00200028003000380040005a0070027003700470057a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80188a5978797a6b68c17d001025a7b0a1d746573745f75706c6f61645f66696c65735f66696c655f69645f6b657910021801220766696c655f69643002380140004a10080010001a00200028003000380040005a007a0408002000800100880100900103980100a20106080012001800a80100b20100ba0100c00100c80188a5978797a6b68c17d0010160036a210a0b0a0561646d696e100218020a0a0a04726f6f74100218021204726f6f741802800101880103980100b2014c0a077072696d61727910001a0866696c656e616d651a0766696c655f69641a0966696c655f73697a651a08757365726e616d651a0b75706c6f61645f74696d65200120022003200420052800b80101c20100e80100f2010408001200f801008002009202009a020a0888a5978797a6b68c17b20200b80200c00265c80200e00200800300880303" +) + // TestPreconditionBeforeStartingAnUpgrade tests that all defined preconditions // must be met before starting an upgrade. func TestPreconditionBeforeStartingAnUpgrade(t *testing.T) { diff --git a/pkg/upgrade/upgrades/upgrades.go b/pkg/upgrade/upgrades/upgrades.go index a3c262230ba9..c1e36f816de6 100644 --- a/pkg/upgrade/upgrades/upgrades.go +++ b/pkg/upgrade/upgrades/upgrades.go @@ -117,11 +117,6 @@ var upgrades = []upgradebase.Upgrade{ ensureSQLSchemaTelemetrySchedule, "add default SQL schema telemetry schedule", ), - upgrade.NewTenantUpgrade("fix corrupt user-file related table descriptors", - toCV(clusterversion.TODODelete_V22_2FixUserfileRelatedDescriptorCorruption), - upgrade.NoPrecondition, - fixInvalidObjectsThatLookLikeBadUserfileConstraint, - ), upgrade.NewTenantUpgrade("add columns to system.tenants and populate a system tenant entry", toCV(clusterversion.V23_1TenantNamesStateAndServiceMode), upgrade.NoPrecondition,