Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bulkio: database with user defined schema not cleaned up properly on restore fail #72248

Closed
RichardJCai opened this issue Oct 29, 2021 · 2 comments · Fixed by #73139
Closed
Assignees
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery

Comments

@RichardJCai
Copy link
Contributor

RichardJCai commented Oct 29, 2021

Reproduction:

func TestRestoreTypeDescriptorsRollBack(t *testing.T) {
	defer leaktest.AfterTest(t)()
	defer log.Scope(t).Close(t)

	_, tc, sqlDB, _, cleanupFn := BackupRestoreTestSetup(t, singleNode, 0, InitManualReplication)
	defer cleanupFn()

	for _, server := range tc.Servers {
		registry := server.JobRegistry().(*jobs.Registry)
		registry.TestingResumerCreationKnobs = map[jobspb.Type]func(raw jobs.Resumer) jobs.Resumer{
			jobspb.TypeRestore: func(raw jobs.Resumer) jobs.Resumer {
				r := raw.(*restoreResumer)
				r.testingKnobs.beforePublishingDescriptors = func() error {
					return errors.New("boom")
				}
				return r
			},
		}
	}

	sqlDB.Exec(t, `
CREATE DATABASE db;
CREATE SCHEMA db.s; // THIS LINE WAS ADDED
CREATE TYPE db.typ AS ENUM();
CREATE TABLE db.table (k INT PRIMARY KEY, v db.typ);
`)

	// Back up the database, drop it, and restore into it.
	sqlDB.Exec(t, `BACKUP DATABASE db TO 'nodelocal://0/test/1'`)
	sqlDB.Exec(t, `DROP DATABASE db`)
	sqlDB.ExpectErr(t, "boom", `RESTORE DATABASE db FROM 'nodelocal://0/test/1'`)
	sqlDB.CheckQueryResults(t, `SELECT count(*) FROM system.namespace WHERE name = 'typ'`, [][]string{{"0"}})

	// Back up database with user defined schema.
	sqlDB.Exec(t, `
CREATE DATABASE db;
CREATE SCHEMA db.s;
CREATE TYPE db.s.typ AS ENUM();
CREATE TABLE db.s.table (k INT PRIMARY KEY, v db.s.typ);
`)

	// Back up the database, drop it, and restore into it.
	sqlDB.Exec(t, `BACKUP DATABASE db TO 'nodelocal://0/test/2'`)
	sqlDB.Exec(t, `DROP DATABASE db`)
	sqlDB.ExpectErr(t, "boom", `RESTORE DATABASE db FROM 'nodelocal://0/test/2'`)
	sqlDB.CheckQueryResults(t, `SELECT count(*) FROM system.namespace WHERE name = 'typ'`, [][]string{{"0"}})
}

modified the test in backup_test.go

See the comment // THIS LINE WAS ADDED to see the diff between now and master.

If the database has an UDS, the descriptor is not deleted properly.
We run into the error pq: failed to resolve targets specified in the BACKUP stmt: duplicate database name: "db" used for ID 59 and 64

During the second BACKUP, the descriptors seem to still contain the descriptor for the original "db" with ID 59.

@RichardJCai RichardJCai added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-disaster-recovery T-disaster-recovery labels Oct 29, 2021
@blathers-crl
Copy link

blathers-crl bot commented Oct 29, 2021

cc @cockroachdb/bulk-io

@RichardJCai
Copy link
Contributor Author

RichardJCai commented Nov 1, 2021

@adityamaru before you start working on this I think I have a fix for this already.

It's fixed as part of this commit: aaab40f
in #72000

adityamaru added a commit to adityamaru/cockroach that referenced this issue Nov 24, 2021
This change fixes a bug where during restore cleanup we
would delete the database descriptor, and namespace entry
only to add the descriptor entry back when clearing the schema
references in the database desc slice.

We fix this by first clearing the schema reference in the db
descroptor slice, and then going through the motions of dropping
the database descriptor.

Fixes: cockroachdb#72248

Release note (bug fix): Fix bug in database and schema restore
cleanup that results in a dangling descriptor entry on job failure.
craig bot pushed a commit that referenced this issue Nov 25, 2021
72660: roachpb/server: add new endpoint for getting a table's index stats r=lindseyjin a=lindseyjin

Partially addresses #67647

Previously, we had an RPC endpoint for retrieving all index usage
statistics. However, this endpoint should only be used for internal
purposes. We needed an endpoint that can be exposed to the frontend, for
use in the Index Stats table (which will be created in a following
commit).

Hence, this commit creates a new RPC endpoint and functions that read
the required index usage statistics directly from the database using
SQL. Instead of returning all index usage statistics, we only retrieve
the indexes for a specified database and table.

Release note (api change): Add new api endpoint for getting a table's
index statistics.

73135: ui: save search query on cache for Transactions page r=maryliag a=maryliag

Previously, a search  was not maintained when
the page change (e.g. coming back from Transaction details).
This commits saves the selected value to be used.

Partially adresses #71851

Release note: None

73139: backupccl: fix bug in database and schema restore cleanup r=stevendanna,RichardJCai a=adityamaru

This change fixes a bug where during restore cleanup we
would delete the database descriptor, and namespace entry
only to add the descriptor entry back when clearing the schema
references in the database desc slice.

We fix this by first clearing the schema reference in the db
descroptor slice, and then going through the motions of dropping
the database descriptor.

Fixes: #72248

Release note (bug fix): Fix bug in database and schema restore
cleanup that results in a dangling descriptor entry on job failure.

Co-authored-by: Lindsey Jin <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Aditya Maru <[email protected]>
@craig craig bot closed this as completed in 465369d Nov 25, 2021
adityamaru added a commit to adityamaru/cockroach that referenced this issue Dec 2, 2021
This change fixes a bug where during restore cleanup we
would delete the database descriptor, and namespace entry
only to add the descriptor entry back when clearing the schema
references in the database desc slice.

We fix this by first clearing the schema reference in the db
descroptor slice, and then going through the motions of dropping
the database descriptor.

Fixes: cockroachdb#72248

Release note (bug fix): Fix bug in database and schema restore
cleanup that results in a dangling descriptor entry on job failure.
adityamaru added a commit to adityamaru/cockroach that referenced this issue Dec 16, 2021
This change fixes a bug where during restore cleanup we
would delete the database descriptor, and namespace entry
only to add the descriptor entry back when clearing the schema
references in the database desc slice.

We fix this by first clearing the schema reference in the db
descroptor slice, and then going through the motions of dropping
the database descriptor.

Fixes: cockroachdb#72248

Release note (bug fix): Fix bug in database and schema restore
cleanup that results in a dangling descriptor entry on job failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants