From b8de5ff50d760cb50805ecad1b31bd9d386ad173 Mon Sep 17 00:00:00 2001 From: Jeff Date: Wed, 3 Nov 2021 14:27:43 -0400 Subject: [PATCH 1/2] sqlproxy: add a hint to the error returned by the connection throttle The most common cause of connection throttling is a misconfigured user or password. Providing a hint should make it clear to a user what they need to do to fix it. There was a bug in the toPgError handler that prevented it from handling hints properly. It was taking hints from the internal code error instead of the wrapping error. Release note: None --- pkg/ccl/sqlproxyccl/authentication.go | 10 +++++----- pkg/ccl/sqlproxyccl/authentication_test.go | 13 +++++++++---- pkg/ccl/sqlproxyccl/proxy.go | 2 +- pkg/ccl/sqlproxyccl/proxy_handler.go | 12 +++++++++--- pkg/ccl/sqlproxyccl/proxy_handler_test.go | 2 +- 5 files changed, 25 insertions(+), 14 deletions(-) diff --git a/pkg/ccl/sqlproxyccl/authentication.go b/pkg/ccl/sqlproxyccl/authentication.go index 974001e53a3f..f11b22f1c2dc 100644 --- a/pkg/ccl/sqlproxyccl/authentication.go +++ b/pkg/ccl/sqlproxyccl/authentication.go @@ -18,7 +18,7 @@ import ( // authenticate handles the startup of the pgwire protocol to the point where // the connections is considered authenticated. If that doesn't happen, it // returns an error. -var authenticate = func(clientConn, crdbConn net.Conn, throttleHook func(throttler.AttemptStatus) *pgproto3.ErrorResponse) error { +var authenticate = func(clientConn, crdbConn net.Conn, throttleHook func(throttler.AttemptStatus) error) error { fe := pgproto3.NewBackend(pgproto3.NewChunkReader(clientConn), clientConn) be := pgproto3.NewFrontend(pgproto3.NewChunkReader(crdbConn), crdbConn) @@ -70,10 +70,10 @@ var authenticate = func(clientConn, crdbConn net.Conn, throttleHook func(throttl case *pgproto3.AuthenticationOk: throttleError := throttleHook(throttler.AttemptOK) if throttleError != nil { - if err = feSend(throttleError); err != nil { + if err = feSend(toPgError(throttleError)); err != nil { return err } - return newErrorf(codeProxyRefusedConnection, "connection attempt throttled") + return throttleError } if err = feSend(backendMsg); err != nil { return err @@ -84,10 +84,10 @@ var authenticate = func(clientConn, crdbConn net.Conn, throttleHook func(throttl case *pgproto3.ErrorResponse: throttleError := throttleHook(throttler.AttemptInvalidCredentials) if throttleError != nil { - if err = feSend(throttleError); err != nil { + if err = feSend(toPgError(throttleError)); err != nil { return err } - return newErrorf(codeProxyRefusedConnection, "connection attempt throttled") + return throttleError } if err = feSend(backendMsg); err != nil { return err diff --git a/pkg/ccl/sqlproxyccl/authentication_test.go b/pkg/ccl/sqlproxyccl/authentication_test.go index c91ae4772b2d..ab791402375f 100644 --- a/pkg/ccl/sqlproxyccl/authentication_test.go +++ b/pkg/ccl/sqlproxyccl/authentication_test.go @@ -19,7 +19,7 @@ import ( "github.com/stretchr/testify/require" ) -var nilThrottleHook = func(state throttler.AttemptStatus) *pgproto3.ErrorResponse { +var nilThrottleHook = func(state throttler.AttemptStatus) error { return nil } @@ -105,7 +105,12 @@ func TestAuthenticateThrottled(t *testing.T) { msg, err = fe.Receive() require.NoError(t, err) - require.Equal(t, msg, &pgproto3.ErrorResponse{Message: "throttled"}) + require.Equal(t, msg, &pgproto3.ErrorResponse{ + Severity: "FATAL", + Code: "08004", + Message: "codeProxyRefusedConnection: connection attempt throttled", + Hint: throttledErrorHint, + }) // Try reading from the connection. This check ensures authorize // swallowed the OK/Error response from the sql server. @@ -139,9 +144,9 @@ func TestAuthenticateThrottled(t *testing.T) { go server(t, sqlServer, &pgproto3.AuthenticationOk{}) go client(t, sqlClient) - err := authenticate(proxyToClient, proxyToServer, func(status throttler.AttemptStatus) *pgproto3.ErrorResponse { + err := authenticate(proxyToClient, proxyToServer, func(status throttler.AttemptStatus) error { require.Equal(t, throttler.AttemptOK, status) - return &pgproto3.ErrorResponse{Message: "throttled"} + return throttledError }) require.Error(t, err) require.Contains(t, err.Error(), "connection attempt throttled") diff --git a/pkg/ccl/sqlproxyccl/proxy.go b/pkg/ccl/sqlproxyccl/proxy.go index 00ac6de13dfa..e9fdf2dfda8c 100644 --- a/pkg/ccl/sqlproxyccl/proxy.go +++ b/pkg/ccl/sqlproxyccl/proxy.go @@ -60,7 +60,7 @@ func toPgError(err error) *pgproto3.ErrorResponse { Severity: "FATAL", Code: pgCode, Message: msg, - Hint: errors.FlattenHints(codeErr.err), + Hint: errors.FlattenHints(err), } } // Return a generic "internal server error" message. diff --git a/pkg/ccl/sqlproxyccl/proxy_handler.go b/pkg/ccl/sqlproxyccl/proxy_handler.go index cff6c9fd7f8e..a1e0d745363a 100644 --- a/pkg/ccl/sqlproxyccl/proxy_handler.go +++ b/pkg/ccl/sqlproxyccl/proxy_handler.go @@ -133,7 +133,13 @@ type proxyHandler struct { certManager *certmgr.CertManager } -var throttledError = newErrorf(codeProxyRefusedConnection, "connection attempt throttled") +const throttledErrorHint string = `Connection throttling is triggered by repeated authentication failure. Make +sure the username and password are correct. +` + +var throttledError = errors.WithHint( + newErrorf(codeProxyRefusedConnection, "connection attempt throttled"), + throttledErrorHint) // newProxyHandler will create a new proxy handler with configuration based on // the provided options. @@ -398,11 +404,11 @@ func (handler *proxyHandler) handle(ctx context.Context, incomingConn *proxyConn defer func() { _ = crdbConn.Close() }() // Perform user authentication. - if err := authenticate(conn, crdbConn, func(status throttler.AttemptStatus) *pgproto3.ErrorResponse { + if err := authenticate(conn, crdbConn, func(status throttler.AttemptStatus) error { err := handler.throttleService.ReportAttempt(ctx, throttleTags, throttleTime, status) if err != nil { log.Errorf(ctx, "throttler refused connection after authentication: %v", err.Error()) - return toPgError(throttledError) + return throttledError } return nil }); err != nil { diff --git a/pkg/ccl/sqlproxyccl/proxy_handler_test.go b/pkg/ccl/sqlproxyccl/proxy_handler_test.go index b48c0d53d6bf..357c03dc5195 100644 --- a/pkg/ccl/sqlproxyccl/proxy_handler_test.go +++ b/pkg/ccl/sqlproxyccl/proxy_handler_test.go @@ -1000,7 +1000,7 @@ func newTester() *tester { // Record successful connection and authentication. originalAuthenticate := authenticate te.restoreAuthenticate = - testutils.TestingHook(&authenticate, func(clientConn, crdbConn net.Conn, throttleHook func(status throttler.AttemptStatus) *pgproto3.ErrorResponse) error { + testutils.TestingHook(&authenticate, func(clientConn, crdbConn net.Conn, throttleHook func(status throttler.AttemptStatus) error) error { err := originalAuthenticate(clientConn, crdbConn, throttleHook) te.setAuthenticated(err == nil) return err From 2276b0ada01da6b38e9aca019dec7e2a5b687673 Mon Sep 17 00:00:00 2001 From: richardjcai Date: Fri, 22 Oct 2021 17:03:44 -0400 Subject: [PATCH 2/2] backupccl: Remove dependencies on defaultdb and postgres for full cluster restore Previously we hardcoded some cases where defaultdb and postgres were assumed to be present during the full cluster restore. With this change, we make it so we treat defaultdb/postgres to be regular databases for restore - they are dropped before the restore happens. If the restore fails, the databases are recreated on rollback. Additionally pg_temp_schemas are now restored to their original parent db and not defaultdb. Release note (enterprise change): temporary tables are now restored to their original database instead of defaultdb during a full cluster restore. Furthermore, defaultdb and postgres are dropped before a full cluster restore and will only be restored if they're present in the backup being restored. --- pkg/ccl/backupccl/backup_test.go | 62 ++++++----- .../full_cluster_backup_restore_test.go | 83 +++++++++++++- pkg/ccl/backupccl/restore_job.go | 20 +++- .../restore_mid_schema_change_test.go | 4 +- pkg/ccl/backupccl/restore_planning.go | 101 ++++++++---------- pkg/ccl/backupccl/targets.go | 62 ++++------- .../testdata/backup-restore/multiregion | 12 ++- .../testdata/backup-restore/temp-tables | 13 +-- pkg/sql/catalog/catalogkeys/keys.go | 7 +- 9 files changed, 220 insertions(+), 144 deletions(-) diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index a7a47d8ba781..34299849b390 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -2238,8 +2238,17 @@ INSERT INTO d.tb VALUES ('hello'), ('hello'); defer cleanupRestore() // We should get an error when restoring the table. sqlDBRestore.ExpectErr(t, "sst: no such file", `RESTORE FROM $1`, LocalFoo) + + // Make sure the temp system db is not present. row := sqlDBRestore.QueryStr(t, fmt.Sprintf(`SELECT * FROM [SHOW DATABASES] WHERE database_name = '%s'`, restoreTempSystemDB)) require.Equal(t, 0, len(row)) + + // Make sure defaultdb and postgres are recreated. + sqlDBRestore.CheckQueryResults(t, + `SELECT * FROM system.namespace WHERE name = 'defaultdb' OR name ='postgres'`, [][]string{ + {"0", "0", "defaultdb", "70"}, + {"0", "0", "postgres", "71"}, + }) } func TestBackupRestoreUserDefinedSchemas(t *testing.T) { @@ -6065,7 +6074,6 @@ func TestBackupRestoreCorruptedStatsIgnored(t *testing.T) { var tableID int sqlDB.QueryRow(t, `SELECT id FROM system.namespace WHERE name = 'bank'`).Scan(&tableID) - fmt.Println(tableID) sqlDB.Exec(t, `BACKUP data.bank TO $1`, dest) // Overwrite the stats file with some invalid data. @@ -8199,23 +8207,19 @@ func TestFullClusterTemporaryBackupAndRestore(t *testing.T) { sqlDBRestore.Exec(t, `RESTORE FROM 'nodelocal://0/full_cluster_backup'`) // Before the reconciliation job runs we should be able to see the following: - // - 4 synthesized pg_temp sessions in defaultdb. - // We synthesize a new temp schema for each unique backed-up - // tuple of a temporary table descriptor. + // - 2 synthesized pg_temp sessions in defaultdb and 1 each in db1 and db2. + // We synthesize a new temp schema for each unique backed-up schemaID + // of a temporary table descriptor. // - All temp tables remapped to belong to the associated synthesized temp - // schema, and in the defaultdb. - checkSchemasQuery := `SELECT schema_name FROM [SHOW SCHEMAS] WHERE schema_name LIKE 'pg_temp_%' ORDER BY -schema_name` - sqlDBRestore.CheckQueryResults(t, checkSchemasQuery, - [][]string{{"pg_temp_0_0"}, {"pg_temp_0_1"}, {"pg_temp_0_2"}, {"pg_temp_0_3"}}) + // schema in the original db. + checkSchemasQuery := `SELECT count(*) FROM [SHOW SCHEMAS] WHERE schema_name LIKE 'pg_temp_%'` + sqlDBRestore.CheckQueryResults(t, checkSchemasQuery, [][]string{{"2"}}) - checkTempTablesQuery := `SELECT schema_name, -table_name FROM [SHOW TABLES] ORDER BY schema_name, table_name` - sqlDBRestore.CheckQueryResults(t, checkTempTablesQuery, [][]string{{"pg_temp_0_0", "t"}, - {"pg_temp_0_1", "t"}, {"pg_temp_0_2", "t"}, {"pg_temp_0_3", "t"}}) + checkTempTablesQuery := `SELECT table_name FROM [SHOW TABLES] ORDER BY table_name` + sqlDBRestore.CheckQueryResults(t, checkTempTablesQuery, [][]string{{"t"}, {"t"}}) // Sanity check that the databases the temporary tables originally belonged to - // are restored and empty because of the remapping. + // are restored. sqlDBRestore.CheckQueryResults(t, `SELECT database_name FROM [SHOW DATABASES] ORDER BY database_name`, [][]string{{"d1"}, {"d2"}, {"defaultdb"}, {"postgres"}, {"system"}}) @@ -8228,26 +8232,36 @@ table_name FROM [SHOW TABLES] ORDER BY schema_name, table_name` sqlDBRestore.QueryRow(t, checkCommentQuery).Scan(&commentCount) require.Equal(t, commentCount, 2) - // Check that show tables in one of the restored DBs returns an empty result. + // Check that show tables in one of the restored DBs returns the temporary + // table. sqlDBRestore.Exec(t, "USE d1") - sqlDBRestore.CheckQueryResults(t, "SHOW TABLES", [][]string{}) + sqlDBRestore.CheckQueryResults(t, checkTempTablesQuery, [][]string{ + {"t"}, + }) + sqlDBRestore.CheckQueryResults(t, checkSchemasQuery, [][]string{{"1"}}) sqlDBRestore.Exec(t, "USE d2") - sqlDBRestore.CheckQueryResults(t, "SHOW TABLES", [][]string{}) + sqlDBRestore.CheckQueryResults(t, checkTempTablesQuery, [][]string{ + {"t"}, + }) + sqlDBRestore.CheckQueryResults(t, checkSchemasQuery, [][]string{{"1"}}) testutils.SucceedsSoon(t, func() error { ch <- timeutil.Now() <-finishedCh - // Check that all the synthesized temp schemas have been wiped. - sqlDBRestore.CheckQueryResults(t, checkSchemasQuery, [][]string{}) + for _, database := range []string{"defaultdb", "d1", "d2"} { + sqlDBRestore.Exec(t, fmt.Sprintf("USE %s", database)) + // Check that all the synthesized temp schemas have been wiped. + sqlDBRestore.CheckQueryResults(t, checkSchemasQuery, [][]string{{"0"}}) - // Check that all the temp tables have been wiped. - sqlDBRestore.CheckQueryResults(t, checkTempTablesQuery, [][]string{}) + // Check that all the temp tables have been wiped. + sqlDBRestore.CheckQueryResults(t, checkTempTablesQuery, [][]string{}) - // Check that all the temp table comments have been wiped. - sqlDBRestore.QueryRow(t, checkCommentQuery).Scan(&commentCount) - require.Equal(t, commentCount, 0) + // Check that all the temp table comments have been wiped. + sqlDBRestore.QueryRow(t, checkCommentQuery).Scan(&commentCount) + require.Equal(t, commentCount, 0) + } return nil }) } diff --git a/pkg/ccl/backupccl/full_cluster_backup_restore_test.go b/pkg/ccl/backupccl/full_cluster_backup_restore_test.go index 7a9a3da9b237..e77d841647b6 100644 --- a/pkg/ccl/backupccl/full_cluster_backup_restore_test.go +++ b/pkg/ccl/backupccl/full_cluster_backup_restore_test.go @@ -181,11 +181,14 @@ CREATE TABLE data2.foo (a int); // Check that zones are restored during pre-restore. t.Run("ensure zones are restored during pre-restore", func(t *testing.T) { <-restoredZones - checkZones := "SELECT * FROM system.zones" + // Not specifying the schema makes the query search using defaultdb first. + // which ends up returning the error + // pq: database "defaultdb" is offline: restoring + checkZones := "SELECT * FROM system.public.zones" sqlDBRestore.CheckQueryResults(t, checkZones, sqlDB.QueryStr(t, checkZones)) // Check that the user tables are still offline. - sqlDBRestore.ExpectErr(t, "database \"data\" is offline: restoring", "SELECT * FROM data.bank") + sqlDBRestore.ExpectErr(t, "database \"data\" is offline: restoring", "SELECT * FROM data.public.bank") // Check there is no data in the span that we expect user data to be imported. store := tcRestore.GetFirstStoreFromServer(t, 0) @@ -610,7 +613,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) { // Note that the system tables here correspond to the temporary tables // imported, not the system tables themselves. sqlDBRestore.CheckQueryResults(t, - `SELECT name FROM crdb_internal.tables WHERE state = 'DROP' ORDER BY name`, + `SELECT name FROM system.crdb_internal.tables WHERE state = 'DROP' ORDER BY name`, [][]string{ {"bank"}, {"comments"}, @@ -700,7 +703,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) { // Note that the system tables here correspond to the temporary tables // imported, not the system tables themselves. sqlDBRestore.CheckQueryResults(t, - `SELECT name FROM crdb_internal.tables WHERE state = 'DROP' ORDER BY name`, + `SELECT name FROM system.crdb_internal.tables WHERE state = 'DROP' ORDER BY name`, [][]string{ {"bank"}, {"comments"}, @@ -1029,3 +1032,75 @@ CREATE TABLE bar (id INT); BACKUP TO 'nodelocal://1/foo' WITH revision_history; `) } + +func TestRestoreWithRecreatedDefaultDB(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + sqlDB, tempDir, cleanupFn := createEmptyCluster(t, singleNode) + _, _, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{}) + defer cleanupFn() + defer cleanupEmptyCluster() + + sqlDB.Exec(t, ` +DROP DATABASE defaultdb; +CREATE DATABASE defaultdb; +`) + sqlDB.Exec(t, `BACKUP TO $1`, LocalFoo) + + sqlDBRestore.Exec(t, `RESTORE FROM $1`, LocalFoo) + + // Since we dropped and recreated defaultdb, defaultdb has the next available + // id which is 52. + expectedDefaultDBID := "52" + + sqlDBRestore.CheckQueryResults(t, `SELECT * FROM system.namespace WHERE name = 'defaultdb'`, [][]string{ + {"0", "0", "defaultdb", expectedDefaultDBID}, + }) +} + +func TestRestoreWithDroppedDefaultDB(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + sqlDB, tempDir, cleanupFn := createEmptyCluster(t, singleNode) + _, _, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{}) + defer cleanupFn() + defer cleanupEmptyCluster() + + sqlDB.Exec(t, ` +DROP DATABASE defaultdb; +`) + sqlDB.Exec(t, `BACKUP TO $1`, LocalFoo) + + sqlDBRestore.Exec(t, `RESTORE FROM $1`, LocalFoo) + + sqlDBRestore.CheckQueryResults(t, `SELECT count(*) FROM system.namespace WHERE name = 'defaultdb'`, [][]string{ + {"0"}, + }) +} + +func TestRestoreToClusterWithDroppedDefaultDB(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + sqlDB, tempDir, cleanupFn := createEmptyCluster(t, singleNode) + _, _, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{}) + defer cleanupFn() + defer cleanupEmptyCluster() + + expectedRow := sqlDB.QueryRow(t, `SELECT * FROM system.namespace WHERE name = 'defaultdb'`) + var parentID, parentSchemaID, ID int + var name string + expectedRow.Scan(&parentID, &parentSchemaID, &name, &ID) + + sqlDB.Exec(t, `BACKUP TO $1`, LocalFoo) + + sqlDBRestore.Exec(t, ` +DROP DATABASE defaultdb; +`) + sqlDBRestore.Exec(t, `RESTORE FROM $1`, LocalFoo) + sqlDBRestore.CheckQueryResults(t, `SELECT * FROM system.namespace WHERE name = 'defaultdb'`, [][]string{ + {fmt.Sprint(parentID), fmt.Sprint(parentSchemaID), name, fmt.Sprint(ID)}, + }) +} diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index df66f85e4e89..2965598ccea7 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -2109,7 +2109,25 @@ func (r *restoreResumer) OnFailOrCancel(ctx context.Context, execCtx interface{} } } - return r.dropDescriptors(ctx, execCfg.JobRegistry, execCfg.Codec, txn, descsCol) + if err := r.dropDescriptors(ctx, execCfg.JobRegistry, execCfg.Codec, txn, descsCol); err != nil { + return err + } + + if details.DescriptorCoverage == tree.AllDescriptors { + // We've dropped defaultdb and postgres in the planning phase, we must + // recreate them now if the full cluster restore failed. + ie := p.ExecCfg().InternalExecutor + _, err := ie.Exec(ctx, "recreate-defaultdb", txn, "CREATE DATABASE IF NOT EXISTS defaultdb") + if err != nil { + return err + } + + _, err = ie.Exec(ctx, "recreate-postgres", txn, "CREATE DATABASE IF NOT EXISTS postgres") + if err != nil { + return err + } + } + return nil }); err != nil { return err } diff --git a/pkg/ccl/backupccl/restore_mid_schema_change_test.go b/pkg/ccl/backupccl/restore_mid_schema_change_test.go index 1798d67d9c45..6183665eb5bd 100644 --- a/pkg/ccl/backupccl/restore_mid_schema_change_test.go +++ b/pkg/ccl/backupccl/restore_mid_schema_change_test.go @@ -143,10 +143,12 @@ func expectedSCJobCount(scName string, isClusterRestore, after bool) int { numBackgroundSCJobs = 1 } + // We drop defaultdb and postgres for full cluster restores + numBackgroundDropDatabaseSCJobs := 2 // Since we're doing a cluster restore, we need to account for all of // the schema change jobs that existed in the backup. if isClusterRestore { - expNumSCJobs += numBackgroundSCJobs + expNumSCJobs += numBackgroundSCJobs + numBackgroundDropDatabaseSCJobs // If we're performing a cluster restore, we also need to include the drop // crdb_temp_system job. diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index 16c380e0f1cb..972e95257ab8 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -37,6 +37,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" @@ -264,19 +265,12 @@ func maybeFilterMissingViews( } func synthesizePGTempSchema( - ctx context.Context, p sql.PlanHookState, schemaName string, -) (descpb.ID, descpb.ID, error) { + ctx context.Context, p sql.PlanHookState, schemaName string, dbID descpb.ID, +) (descpb.ID, error) { var synthesizedSchemaID descpb.ID - var defaultDBID descpb.ID err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { var err error - defaultDBID, err = lookupDatabaseID(ctx, txn, p.ExecCfg().Codec, - catalogkeys.DefaultDatabaseName) - if err != nil { - return err - } - - sKey := catalogkeys.NewNameKeyComponents(defaultDBID, keys.RootNamespaceID, schemaName) + sKey := catalogkeys.NewNameKeyComponents(dbID, keys.RootNamespaceID, schemaName) schemaID, err := catalogkv.GetDescriptorID(ctx, txn, p.ExecCfg().Codec, sKey) if err != nil { return err @@ -292,15 +286,7 @@ func synthesizePGTempSchema( return p.CreateSchemaNamespaceEntry(ctx, catalogkeys.EncodeNameKey(p.ExecCfg().Codec, sKey), synthesizedSchemaID) }) - return synthesizedSchemaID, defaultDBID, err -} - -// dbSchemaKey is used when generating fake pg_temp schemas for the purpose of -// restoring temporary objects. Detailed comments can be found where it is being -// used. -type dbSchemaKey struct { - parentID descpb.ID - schemaID descpb.ID + return synthesizedSchemaID, err } // allocateDescriptorRewrites determines the new ID and parentID (a "DescriptorRewrite") @@ -481,26 +467,17 @@ func allocateDescriptorRewrites( // represented as a descriptor and thus is not picked up during a full // cluster BACKUP. // To overcome this orphaned schema pointer problem, when restoring a - // temporary object we create a "fake" pg_temp schema in defaultdb and add - // it to the namespace table. We then remap the temporary object descriptors - // to point to this schema. This allows us to piggy back on the temporary + // temporary object we create a "fake" pg_temp schema in temp table's db and + // add it to the namespace table. + // We then remap the temporary object descriptors to point to this schema. + // This allows us to piggy back on the temporary // reconciliation job which looks for "pg_temp" schemas linked to temporary // sessions and properly cleans up the temporary objects in it. - haveSynthesizedTempSchema := make(map[dbSchemaKey]bool) - var defaultDBID descpb.ID + haveSynthesizedTempSchema := make(map[descpb.ID]bool) var synthesizedTempSchemaCount int for _, table := range tablesByID { if table.IsTemporary() { - // We generate a "fake" temporary schema for every unique - // tuple of the backed-up temporary table descriptors. - // This is important because post rewrite all the "fake" schemas and - // consequently temp table objects are going to be in defaultdb. Placing - // them under different "fake" schemas prevents name collisions if the - // backed up tables had the same names but were in different temp - // schemas/databases in the cluster which was backed up. - dbSchemaIDKey := dbSchemaKey{parentID: table.GetParentID(), - schemaID: table.GetParentSchemaID()} - if _, ok := haveSynthesizedTempSchema[dbSchemaIDKey]; !ok { + if _, ok := haveSynthesizedTempSchema[table.GetParentSchemaID()]; !ok { var synthesizedSchemaID descpb.ID var err error // NB: TemporarySchemaNameForRestorePrefix is a special value that has @@ -514,7 +491,7 @@ func allocateDescriptorRewrites( // which the cluster was started. schemaName := sql.TemporarySchemaNameForRestorePrefix + strconv.Itoa(synthesizedTempSchemaCount) - synthesizedSchemaID, defaultDBID, err = synthesizePGTempSchema(ctx, p, schemaName) + synthesizedSchemaID, err = synthesizePGTempSchema(ctx, p, schemaName, table.GetParentID()) if err != nil { return nil, err } @@ -523,13 +500,9 @@ func allocateDescriptorRewrites( // specific pg_temp schema to point to this synthesized schema when we // are performing the table rewrites. descriptorRewrites[table.GetParentSchemaID()] = &jobspb.RestoreDetails_DescriptorRewrite{ID: synthesizedSchemaID} - haveSynthesizedTempSchema[dbSchemaIDKey] = true + haveSynthesizedTempSchema[table.GetParentSchemaID()] = true synthesizedTempSchemaCount++ } - - // Remap the temp table descriptors to belong to the defaultdb where we - // have synthesized the temp schema. - descriptorRewrites[table.GetID()] = &jobspb.RestoreDetails_DescriptorRewrite{ParentID: defaultDBID} } } } @@ -888,6 +861,25 @@ func allocateDescriptorRewrites( return descriptorRewrites, nil } +// If we're doing a full cluster restore - to treat defaultdb and postgres +// as regular databases, we drop them before restoring them again in the +// restore. +func dropDefaultUserDBs(ctx context.Context, execCfg *sql.ExecutorConfig) error { + return sql.DescsTxn(ctx, execCfg, func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error { + ie := execCfg.InternalExecutor + _, err := ie.Exec(ctx, "drop-defaultdb", nil, "DROP DATABASE IF EXISTS defaultdb") + if err != nil { + return err + } + + _, err = ie.Exec(ctx, "drop-postgres", nil, "DROP DATABASE IF EXISTS postgres") + if err != nil { + return err + } + return nil + }) +} + func resolveTargetDB( ctx context.Context, txn *kv.Txn, @@ -901,27 +893,12 @@ func resolveTargetDB( return intoDB, nil } - if descriptorCoverage == tree.AllDescriptors && descriptor.GetParentID() < catalogkeys.MaxDefaultDescriptorID { - // This is a table that is in a database that already existed at - // cluster creation time. - defaultDBID, err := lookupDatabaseID(ctx, txn, p.ExecCfg().Codec, catalogkeys.DefaultDatabaseName) - if err != nil { - return "", err - } - postgresDBID, err := lookupDatabaseID(ctx, txn, p.ExecCfg().Codec, catalogkeys.PgDatabaseName) - if err != nil { - return "", err - } - + if descriptorCoverage == tree.AllDescriptors && descriptor.GetParentID() < keys.MaxReservedDescID { var targetDB string if descriptor.GetParentID() == systemschema.SystemDB.GetID() { // For full cluster backups, put the system tables in the temporary // system table. targetDB = restoreTempSystemDB - } else if descriptor.GetParentID() == defaultDBID { - targetDB = catalogkeys.DefaultDatabaseName - } else if descriptor.GetParentID() == postgresDBID { - targetDB = catalogkeys.PgDatabaseName } return targetDB, nil } @@ -1882,6 +1859,7 @@ func doRestorePlan( if err != nil { return err } + sqlDescs = append(sqlDescs, newTypeDescs...) if err := maybeUpgradeDescriptors(ctx, sqlDescs, restoreStmt.Options.SkipMissingFKs); err != nil { @@ -1960,6 +1938,17 @@ func doRestorePlan( if err != nil { return err } + + // When running a full cluster restore, we drop the defaultdb and postgres + // databases that are present in a new cluster. + // This is done so that they can be restored the same way any other user + // defined database would be restored from the backup. + if restoreStmt.DescriptorCoverage == tree.AllDescriptors { + if err := dropDefaultUserDBs(ctx, p.ExecCfg()); err != nil { + return err + } + } + descriptorRewrites, err := allocateDescriptorRewrites( ctx, p, diff --git a/pkg/ccl/backupccl/targets.go b/pkg/ccl/backupccl/targets.go index 96708de37418..638bedad470d 100644 --- a/pkg/ccl/backupccl/targets.go +++ b/pkg/ccl/backupccl/targets.go @@ -23,7 +23,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -228,45 +227,6 @@ func getAllDescChanges( return res, nil } -func lookupDatabaseID( - ctx context.Context, txn *kv.Txn, codec keys.SQLCodec, name string, -) (descpb.ID, error) { - found, id, err := catalogkv.LookupDatabaseID(ctx, txn, codec, name) - if err != nil { - return descpb.InvalidID, err - } - if !found { - return descpb.InvalidID, errors.Errorf("could not find ID for database %s", name) - } - return id, nil -} - -func fullClusterTargetsRestore( - allDescs []catalog.Descriptor, lastBackupManifest BackupManifest, -) ([]catalog.Descriptor, []catalog.DatabaseDescriptor, []descpb.TenantInfoWithUsage, error) { - fullClusterDescs, fullClusterDBs, err := fullClusterTargets(allDescs) - if err != nil { - return nil, nil, nil, err - } - filteredDescs := make([]catalog.Descriptor, 0, len(fullClusterDescs)) - for _, desc := range fullClusterDescs { - if _, isDefaultDB := catalogkeys.DefaultUserDBs[desc.GetName()]; !isDefaultDB && desc.GetID() != keys.SystemDatabaseID { - filteredDescs = append(filteredDescs, desc) - } - } - filteredDBs := make([]catalog.DatabaseDescriptor, 0, len(fullClusterDBs)) - for _, db := range fullClusterDBs { - if _, isDefaultDB := catalogkeys.DefaultUserDBs[db.GetName()]; !isDefaultDB && db.GetID() != keys.SystemDatabaseID { - filteredDBs = append(filteredDBs, db) - } - } - - // Restore all tenants during full-cluster restore. - tenants := lastBackupManifest.GetTenants() - - return filteredDescs, filteredDBs, tenants, nil -} - // fullClusterTargets returns all of the tableDescriptors to be included in a // full cluster backup, and all the user databases. func fullClusterTargets( @@ -308,6 +268,28 @@ func fullClusterTargets( return fullClusterDescs, fullClusterDBs, nil } +func fullClusterTargetsRestore( + allDescs []catalog.Descriptor, lastBackupManifest BackupManifest, +) ([]catalog.Descriptor, []catalog.DatabaseDescriptor, []descpb.TenantInfoWithUsage, error) { + fullClusterDescs, fullClusterDBs, err := fullClusterTargets(allDescs) + var filteredDescs []catalog.Descriptor + var filteredDBs []catalog.DatabaseDescriptor + for _, desc := range fullClusterDescs { + if desc.GetID() != keys.SystemDatabaseID { + filteredDescs = append(filteredDescs, desc) + } + } + for _, desc := range fullClusterDBs { + if desc.GetID() != keys.SystemDatabaseID { + filteredDBs = append(filteredDBs, desc) + } + } + if err != nil { + return nil, nil, nil, err + } + return filteredDescs, filteredDBs, lastBackupManifest.GetTenants(), nil +} + // fullClusterTargetsBackup returns the same descriptors referenced in // fullClusterTargets, but rather than returning the entire database // descriptor as the second argument, it only returns their IDs. diff --git a/pkg/ccl/backupccl/testdata/backup-restore/multiregion b/pkg/ccl/backupccl/testdata/backup-restore/multiregion index 5c386365aab5..1d5f3896bb54 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/multiregion +++ b/pkg/ccl/backupccl/testdata/backup-restore/multiregion @@ -123,6 +123,10 @@ exec-sql SET CLUSTER SETTING sql.defaults.primary_region = 'eu-north-1'; RESTORE FROM 'nodelocal://1/no_region_cluster_backup/'; ---- +NOTICE: setting the PRIMARY REGION as eu-north-1 on database defaultdb +HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults.primary_region = 'region' or use RESET CLUSTER SETTING sql.defaults.primary_region to disable this behavior +NOTICE: setting the PRIMARY REGION as eu-north-1 on database postgres +HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults.primary_region = 'region' or use RESET CLUSTER SETTING sql.defaults.primary_region to disable this behavior NOTICE: setting the PRIMARY REGION as eu-north-1 on database no_region_db HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults.primary_region = 'region' or use RESET CLUSTER SETTING sql.defaults.primary_region to disable this behavior NOTICE: setting the PRIMARY REGION as eu-north-1 on database no_region_db_2 @@ -131,10 +135,10 @@ HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults query-sql SHOW DATABASES; ---- -defaultdb root {} +defaultdb root eu-north-1 {eu-north-1} zone no_region_db root eu-north-1 {eu-north-1} zone no_region_db_2 root eu-north-1 {eu-north-1} zone -postgres root {} +postgres root eu-north-1 {eu-north-1} zone system node {} query-sql @@ -151,9 +155,9 @@ RESTORE DATABASE eu_central_db FROM 'nodelocal://1/eu_central_database_backup/'; query-sql SHOW DATABASES; ---- -defaultdb root {} +defaultdb root eu-north-1 {eu-north-1} zone eu_central_db root eu-central-1 {eu-central-1} zone no_region_db root eu-north-1 {eu-north-1} zone no_region_db_2 root eu-north-1 {eu-north-1} zone -postgres root {} +postgres root eu-north-1 {eu-north-1} zone system node {} diff --git a/pkg/ccl/backupccl/testdata/backup-restore/temp-tables b/pkg/ccl/backupccl/testdata/backup-restore/temp-tables index dcf2e2afebb2..a846f97fe9ec 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/temp-tables +++ b/pkg/ccl/backupccl/testdata/backup-restore/temp-tables @@ -105,7 +105,7 @@ USE defaultdb; RESTORE FROM 'nodelocal://0/full_cluster_backup/'; ---- -# The pg_temp schema from the BACKUP should not show up after restoration. +# The pg_temp schema from the BACKUP should show up in its original database. query-sql USE d1; SELECT schema_name FROM [SHOW SCHEMAS] ORDER BY schema_name @@ -114,10 +114,10 @@ crdb_internal information_schema pg_catalog pg_extension +pg_temp_0_0 public -# We should see a synthesized pg_temp schema. query-sql USE defaultdb; SELECT schema_name FROM [SHOW SCHEMAS] ORDER BY schema_name @@ -126,21 +126,14 @@ crdb_internal information_schema pg_catalog pg_extension -pg_temp_0_0 public -# On full cluster restore we remap the temp tables to defaultdb so we should not -# see them in the restored db. +# On full cluster restore we restore temp tables to its original database. query-sql USE d1; SELECT table_name FROM [SHOW TABLES] ORDER BY table_name ---- perm_table - -query-sql -USE defaultdb; -SELECT table_name FROM [SHOW TABLES] ORDER BY table_name ----- temp_seq temp_table diff --git a/pkg/sql/catalog/catalogkeys/keys.go b/pkg/sql/catalog/catalogkeys/keys.go index f23d58f63fb4..1c3cc1421b80 100644 --- a/pkg/sql/catalog/catalogkeys/keys.go +++ b/pkg/sql/catalog/catalogkeys/keys.go @@ -33,14 +33,13 @@ const ( ) // DefaultUserDBs is a set of the databases which are present in a new cluster. -var DefaultUserDBs = map[string]struct{}{ - DefaultDatabaseName: {}, - PgDatabaseName: {}, +var DefaultUserDBs = []string{ + DefaultDatabaseName, PgDatabaseName, } // MaxDefaultDescriptorID is the maximum ID of a descriptor that exists in a // new cluster. -var MaxDefaultDescriptorID = keys.MaxReservedDescID + descpb.ID(len(DefaultUserDBs)) +var MaxDefaultDescriptorID = descpb.ID(keys.MaxReservedDescID) + descpb.ID(len(DefaultUserDBs)) // IsDefaultCreatedDescriptor returns whether or not a given descriptor ID is // present at the time of starting a cluster.