Merge #150747 #152015

150747: sql: unblock user table changes from long-running transactions r=bghal a=bghal

sql: unblock user table changes from long-running transactions

Currently, the `CREATE USER` and the `GRANT role` operations use a
bumped descriptor to invalidate caches of the user table.
This blocked those operations until long-running transactions using the
old table version committed.
This change adds special handling of version bumps to wait for
visibility of the change rather than convergence across the cluster. It
no longer creates (blocking) jobs for the version bumps.

Epic: CRDB-49398
Fixes: #138691

Release note (sql change): The `CREATE USER` and `GRANT role` operations
now wait for full-cluster visibility of the new user table version
rather than blocking on convergence.


152015: backup: remove old incremental backup location lookup r=dt a=kev-cao

In pre-22.2 backups, incremental backups were written to the same directory as the full backup. This was removed in 22.2, but logic was kept in place to maintain backwards compatibility and allow later versions to restore from pre-22.2 backups.

However, given that we are now on 25.4 and are many major versions past the removal of the old default location, the legacy logic is now being removed to facilitate the development of backup indexes.

Epic: None

Release note: None

Co-authored-by: Brendan Gerrity <[email protected]>
Co-authored-by: Kevin Cao <[email protected]>

Author: 3 people

pkg/backup/backup_test.go

@@ -9958,118 +9958,6 @@ func TestUserfileNormalizationIncrementalShowBackup(t *testing.T) {
 	sqlDB.Exec(t, query)
 }
 
-// TestBackupRestoreOldIncrementalDefaults tests that a call to restore
-// will correctly load an incremental backup in the old "default" directory.
-// That old default is literally just "the same directory as the full backup",
-// so write that manually with the `incremental_location` option then check that
-// a read without that option contains the same data.
-func TestBackupRestoreOldIncrementalDefault(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	skip.UnderRace(t, "multinode cluster setup times out under race, likely due to resource starvation.")
-
-	const numAccounts = 1
-	_, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, multiNode, numAccounts, InitManualReplication)
-	defer cleanupFn()
-
-	const c1, c2, c3 = `nodelocal://1/full/`, `nodelocal://1/full/`, `nodelocal://2/full/`
-
-	// Deliberately the same. We're simulating an incremental backup in the old
-	// default directory, i.e. the top-level collection directory.
-	const i1, i2, i3 = `nodelocal://1/full/`, `nodelocal://1/full/`, `nodelocal://2/full/`
-
-	collections := []string{
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", c1, url.QueryEscape("default")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", c2, url.QueryEscape("dc=dc1")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", c3, url.QueryEscape("dc=dc2")),
-	}
-
-	incrementals := []string{
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", i1, url.QueryEscape("default")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", i2, url.QueryEscape("dc=dc1")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", i3, url.QueryEscape("dc=dc2")),
-	}
-	tests := []struct {
-		dest []string
-		inc  []string
-	}{{dest: []string{collections[0]}, inc: []string{incrementals[0]}},
-		{dest: collections, inc: incrementals}}
-
-	for _, br := range tests {
-		dest := strings.Join(br.dest, ", ")
-		inc := strings.Join(br.inc, ", ")
-
-		if len(br.dest) > 1 {
-			dest = "(" + dest + ")"
-			inc = "(" + inc + ")"
-		}
-		// create db
-		sqlDB.Exec(t, `CREATE DATABASE fkdb`)
-		sqlDB.Exec(t, `CREATE TABLE fkdb.fk (ind INT)`)
-
-		for i := 0; i < 10; i++ {
-			sqlDB.Exec(t, `INSERT INTO fkdb.fk (ind) VALUES ($1)`, i)
-		}
-		fb := fmt.Sprintf("BACKUP DATABASE fkdb INTO %s", dest)
-		sqlDB.Exec(t, fb)
-
-		sqlDB.Exec(t, `INSERT INTO fkdb.fk (ind) VALUES ($1)`, 200)
-
-		sib := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location=%s", dest, inc)
-		sqlDB.Exec(t, sib)
-
-		sir := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'inc_fkdb'", dest)
-		sqlDB.Exec(t, sir)
-
-		sqlDB.CheckQueryResults(t, `SELECT * FROM inc_fkdb.fk`, sqlDB.QueryStr(t, `SELECT * FROM fkdb.fk`))
-
-		sqlDB.Exec(t, "DROP DATABASE fkdb")
-		sqlDB.Exec(t, "DROP DATABASE inc_fkdb;")
-	}
-}
-
-func TestBackupRestoreErrorsOnBothDefaultsPopulated(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	const numAccounts = 1
-	_, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, InitManualReplication)
-	defer cleanupFn()
-
-	base := `'nodelocal://1/full/'`
-	oldInc := base
-	newInc := `'nodelocal://1/full/incrementals'`
-
-	// create db
-	sqlDB.Exec(t, `CREATE DATABASE fkdb`)
-	sqlDB.Exec(t, `CREATE TABLE fkdb.fk (ind INT)`)
-
-	for i := 0; i < 10; i++ {
-		sqlDB.Exec(t, `INSERT INTO fkdb.fk (ind) VALUES ($1)`, i)
-	}
-	fb := fmt.Sprintf("BACKUP DATABASE fkdb INTO %s", base)
-	sqlDB.Exec(t, fb)
-
-	sibOld := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location=%s", base, oldInc)
-	sqlDB.Exec(t, sibOld)
-
-	sibNew := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location=%s", base, newInc)
-	sqlDB.Exec(t, sibNew)
-
-	irDefault := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'trad_fkdb'", base)
-	sqlDB.ExpectErr(t, "pq: Incremental layers found in both old and new default locations. "+
-		"Please choose a location manually with the `incremental_location` parameter.", irDefault)
-
-	irOld := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'trad_fkdb_old_default', "+
-		"incremental_location=%s", base, base)
-	sqlDB.Exec(t, irOld)
-
-	irNew := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'trad_fkdb_new_default', "+
-		"incremental_location=%s", base, newInc)
-	sqlDB.Exec(t, irNew)
-}
-
 // TestBackupRestoreSeparateExplicitIsDefault tests that a backup/restore round
 // trip using the 'incremental_location' parameter restores the same db as a BR
 // round trip without the parameter, even when that location is in fact the default.

pkg/backup/backupdest/backup_destination_test.go

@@ -133,10 +133,6 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 				inc5Time := inc4Time.Add(time.Minute * 30)
 				inc6Time := inc5Time.Add(time.Minute * 30)
 				inc7Time := inc6Time.Add(time.Minute * 30)
-				full3Time := inc7Time.Add(time.Minute * 30)
-				inc8Time := full3Time.Add(time.Minute * 30)
-				inc9Time := inc8Time.Add(time.Minute * 30)
-				full4Time := inc9Time.Add(time.Minute * 30)
 
 				// firstBackupChain is maintained throughout the tests as the history of
 				// backups that were taken based on the initial full backup.
@@ -155,6 +151,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 				testCollectionBackup := func(t *testing.T, backupTime time.Time,
 					expectedDefault, expectedSuffix, expectedIncDir string, expectedPrevBackups []string,
 					appendToLatest bool, subdir string, incrementalTo []string) {
+					t.Helper()
 
 					endTime := hlc.Timestamp{WallTime: backupTime.UnixNano()}
 
@@ -335,63 +332,6 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 						true /* intoLatest */, expectedSubdir, customIncrementalTo)
 					writeManifest(t, expectedDefault, inc7Time)
 				}
-
-				// A new full backup: BACKUP INTO collection
-				var backup3Location string
-				{
-					expectedSuffix := "/2020/12/25-103000.00"
-					expectedIncDir := ""
-					expectedDefault := fmt.Sprintf("nodelocal://1/%s%s?AUTH=implicit", t.Name(), expectedSuffix)
-					backup3Location = expectedDefault
-
-					testCollectionBackup(t, full3Time,
-						expectedDefault, expectedSuffix, expectedIncDir, []string(nil),
-						false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
-					writeManifest(t, expectedDefault, full3Time)
-					// We also wrote a new full backup, so let's update the latest.
-					writeLatest(t, collectionLoc, expectedSuffix)
-				}
-
-				// A remote incremental into the third full backup: BACKUP INTO LATEST
-				// IN collection, BUT with a trick. Write a (fake) incremental backup
-				// to the old directory, to be sure that subsequent incremental backups
-				// go there as well (and not the newer incrementals/ subdir.)
-				{
-					expectedSuffix := "/2020/12/25-103000.00"
-					expectedIncDir := "/20201225/110000.00-20201225-103000.00"
-
-					// Writes the (fake) incremental backup.
-					oldStyleDefault := fmt.Sprintf("nodelocal://1/%s%s%s?AUTH=implicit",
-						t.Name(),
-						expectedSuffix, expectedIncDir)
-					writeManifest(t, oldStyleDefault, inc8Time)
-
-					expectedSuffix = "/2020/12/25-103000.00"
-					expectedIncDir = "/20201225/113000.00-20201225-110000.00"
-					expectedSubdir := expectedSuffix
-					expectedDefault := fmt.Sprintf("nodelocal://1/%s%s%s?AUTH=implicit",
-						t.Name(),
-						expectedSuffix, expectedIncDir)
-
-					testCollectionBackup(t, inc9Time,
-						expectedDefault, expectedSuffix, expectedIncDir, []string{backup3Location, oldStyleDefault},
-						true /* intoLatest */, expectedSubdir, noIncrementalStorage)
-					writeManifest(t, expectedDefault, inc9Time)
-				}
-
-				// A new full backup: BACKUP INTO collection
-				{
-					expectedSuffix := "/2020/12/25-120000.00"
-					expectedIncDir := ""
-					expectedDefault := fmt.Sprintf("nodelocal://1/%s%s?AUTH=implicit", t.Name(), expectedSuffix)
-
-					testCollectionBackup(t, full4Time,
-						expectedDefault, expectedSuffix, expectedIncDir, []string(nil),
-						false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
-					writeManifest(t, expectedDefault, full4Time)
-					// We also wrote a new full backup, so let's update the latest.
-					writeLatest(t, collectionLoc, expectedSuffix)
-				}
 			})
 		})
 	}

pkg/backup/backupdest/incrementals.go

@@ -196,45 +196,16 @@ func ResolveIncrementalsBackupLocation(
 		return incPaths, nil
 	}
 
-	resolvedIncrementalsBackupLocationOld, err := backuputils.AppendPaths(fullBackupCollections, subdir)
-	if err != nil {
-		return nil, err
-	}
-
-	// We can have >1 full backup collection specified, but each will have an
-	// incremental layer iff all of them do. So it suffices to check only the
-	// first.
-	// Check we can read from this location, though we don't need the backups here.
-	prevOld, err := backupsFromLocation(ctx, user, execCfg, resolvedIncrementalsBackupLocationOld[0])
-	if err != nil {
-		return nil, err
-	}
-
 	resolvedIncrementalsBackupLocation, err := backuputils.AppendPaths(fullBackupCollections, backupbase.DefaultIncrementalsSubdir, subdir)
 	if err != nil {
 		return nil, err
 	}
 
-	prev, err := backupsFromLocation(ctx, user, execCfg, resolvedIncrementalsBackupLocation[0])
+	_, err = backupsFromLocation(ctx, user, execCfg, resolvedIncrementalsBackupLocation[0])
 	if err != nil {
 		return nil, err
 	}
 
-	// TODO(bardin): This algorithm divides "destination resolution" and "actual backup lookup" for historical reasons,
-	// but this doesn't quite make sense now that destination resolution depends on backup lookup.
-	// Try to figure out a clearer way to organize this.
-	if len(prevOld) > 0 && len(prev) > 0 {
-		return nil, errors.New(
-			"Incremental layers found in both old and new default locations. " +
-				"Please choose a location manually with the `incremental_location` parameter.")
-	}
-
-	// If we have backups in the old default location, continue to use the old location.
-	if len(prevOld) > 0 {
-		return resolvedIncrementalsBackupLocationOld, nil
-	}
-
-	// Otherwise, use the new location.
 	return resolvedIncrementalsBackupLocation, nil
 }
 

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -1,7 +1,7 @@
 exp,benchmark
-14,AlterRole/alter_role_with_1_option
-17,AlterRole/alter_role_with_2_options
-23,AlterRole/alter_role_with_3_options
+12,AlterRole/alter_role_with_1_option
+15,AlterRole/alter_role_with_2_options
+19,AlterRole/alter_role_with_3_options
 13,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint
 13,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints
 13,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints
@@ -15,9 +15,9 @@ exp,benchmark
 14,AlterTableConfigureZone/alter_table_configure_zone_5_replicas
 14,AlterTableConfigureZone/alter_table_configure_zone_7_replicas_
 14,AlterTableConfigureZone/alter_table_configure_zone_ranges
-21,AlterTableDropColumn/alter_table_drop_1_column
-21,AlterTableDropColumn/alter_table_drop_2_columns
-21,AlterTableDropColumn/alter_table_drop_3_columns
+22,AlterTableDropColumn/alter_table_drop_1_column
+22,AlterTableDropColumn/alter_table_drop_2_columns
+22,AlterTableDropColumn/alter_table_drop_3_columns
 13,AlterTableDropConstraint/alter_table_drop_1_check_constraint
 13,AlterTableDropConstraint/alter_table_drop_2_check_constraints
 13,AlterTableDropConstraint/alter_table_drop_3_check_constraints
@@ -28,20 +28,20 @@ exp,benchmark
 11,AlterTableUnsplit/alter_table_unsplit_at_2_values
 14,AlterTableUnsplit/alter_table_unsplit_at_3_values
 2-4,Audit/select_from_an_audit_table
-20,CreateRole/create_role_with_1_option
-23,CreateRole/create_role_with_2_options
-26,CreateRole/create_role_with_3_options
-21,CreateRole/create_role_with_no_options
+13,CreateRole/create_role_with_1_option
+16,CreateRole/create_role_with_2_options
+19,CreateRole/create_role_with_3_options
+14,CreateRole/create_role_with_no_options
 14,"Discard/DISCARD_ALL,_1_tables_in_1_db"
 19,"Discard/DISCARD_ALL,_2_tables_in_2_dbs"
 0,"Discard/DISCARD_ALL,_no_tables"
 15,DropDatabase/drop_database_0_tables
 16,DropDatabase/drop_database_1_table
 16,DropDatabase/drop_database_2_tables
 16,DropDatabase/drop_database_3_tables
-29-30,DropRole/drop_1_role
-37,DropRole/drop_2_roles
-45-49,DropRole/drop_3_roles
+22,DropRole/drop_1_role
+30,DropRole/drop_2_roles
+39,DropRole/drop_3_roles
 13,DropSequence/drop_1_sequence
 15,DropSequence/drop_2_sequences
 17,DropSequence/drop_3_sequences
@@ -59,8 +59,8 @@ exp,benchmark
 8,Grant/grant_all_on_1_table
 8,Grant/grant_all_on_2_tables
 8,Grant/grant_all_on_3_tables
-15,GrantRole/grant_1_role
-19,GrantRole/grant_2_roles
+12,GrantRole/grant_1_role
+16,GrantRole/grant_2_roles
 12-15,Jobs/cancel_job
 3,Jobs/crdb_internal.system_jobs
 3-5,Jobs/jobs_page_default
@@ -79,19 +79,19 @@ exp,benchmark
 4,ORMQueries/django_column_introspection_1_table
 4,ORMQueries/django_column_introspection_4_tables
 4,ORMQueries/django_column_introspection_8_tables
-6,ORMQueries/django_comment_introspection_with_comments
-6,ORMQueries/django_table_introspection_1_table
-6,ORMQueries/django_table_introspection_8_tables
+5,ORMQueries/django_comment_introspection_with_comments
+5,ORMQueries/django_table_introspection_1_table
+5,ORMQueries/django_table_introspection_8_tables
 0,ORMQueries/has_column_privilege_using_attnum
 0,ORMQueries/has_column_privilege_using_column_name
 0,ORMQueries/has_schema_privilege
 0,ORMQueries/has_sequence_privilege
 0,ORMQueries/has_table_privilege
-5,ORMQueries/hasura_column_descriptions
+6,ORMQueries/hasura_column_descriptions
 20,ORMQueries/hasura_column_descriptions_8_tables
 5,ORMQueries/hasura_column_descriptions_modified
 4,ORMQueries/information_schema._pg_index_position
-4,ORMQueries/introspection_description_join
+5,ORMQueries/introspection_description_join
 4,ORMQueries/liquibase_migrations
 8,ORMQueries/liquibase_migrations_on_multiple_dbs
 5,ORMQueries/npgsql_fields
@@ -105,14 +105,14 @@ exp,benchmark
 3,ORMQueries/pg_namespace
 4,ORMQueries/pg_type
 6,ORMQueries/prisma_column_descriptions
-3,ORMQueries/prisma_column_descriptions_updated
+4,ORMQueries/prisma_column_descriptions_updated
 5,ORMQueries/prisma_types_16
 5,ORMQueries/prisma_types_4
 8,Revoke/revoke_all_on_1_table
 8,Revoke/revoke_all_on_2_tables
 8,Revoke/revoke_all_on_3_tables
-13,RevokeRole/revoke_1_role
-15,RevokeRole/revoke_2_roles
+10,RevokeRole/revoke_1_role
+12,RevokeRole/revoke_2_roles
 4,ShowGrants/grant_2_roles
 4,ShowGrants/grant_3_roles
 4,ShowGrants/grant_4_roles
@@ -129,7 +129,7 @@ exp,benchmark
 0,UDFResolution/select_from_udf
 2,UseManyRoles/use_2_roles
 2,UseManyRoles/use_50_roles
-1,VirtualTableQueries/select_crdb_internal.invalid_objects_with_1_fk
+2,VirtualTableQueries/select_crdb_internal.invalid_objects_with_1_fk
 1,VirtualTableQueries/select_crdb_internal.tables_with_1_fk
 0,VirtualTableQueries/show_create_all_routines
 0,VirtualTableQueries/show_create_all_triggers

pkg/cli/testdata/doctor/test_examine_cluster

@@ -3,5 +3,5 @@ debug doctor examine cluster
 debug doctor examine cluster
 Examining 69 descriptors and 68 namespace entries...
   ParentID 100, ParentSchemaID 101: relation "foo" (105): expected matching namespace entry, found none
-Examining 13 jobs...
+Examining 12 jobs...
 ERROR: validation failed

pkg/cli/testdata/doctor/test_examine_cluster_dropped

@@ -2,5 +2,5 @@ debug doctor examine cluster
 ----
 debug doctor examine cluster
 Examining 68 descriptors and 68 namespace entries...
-Examining 11 jobs...
+Examining 10 jobs...
 No problems found!