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
backupccl: fix codec selection during restore #116316
Conversation
Previously, the codec selection logic only looked at a single manifest to determine whether the backup data requires a system or tenant codec to be read. In older release branches if the full backup was empty (but a subsequent incremental was not) then we could incorrectly use a system codec for a backup that actually required a tenant codec. To fix this we now process all the manifests in the backup chain and use the first non-empty manifest. Fixes: cockroachdb#115773 Release note(bug fix): an empty full backup, followed by non-empty incrementals taken inside an application tenant may be unrestoreable because of the use of an incorrect SQL codec
}, | ||
expectedCodec: keys.SystemSQLCodec, | ||
}, | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! nice tests!
sqlDB.Exec(t, "CREATE TABLE create_and_drop.a (k int, v string)") | ||
sqlDB.Exec(t, "INSERT INTO create_and_drop.a VALUES (1, 'foo')") | ||
sqlDB.QueryRow(t, "SELECT cluster_logical_timestamp()").Scan(&tsBefore) | ||
sqlDB.Exec(t, "DROP TABLE create_and_drop.a") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious: why did you need to drop the table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't necessarily have to, just copy pasted this test from another test where I initially noticed this behaviour
Thanks! bors r=msbutler |
Build failed: |
bors retry |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from a75b261 to blathers/backport-release-23.1-116316: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. error creating merge commit from a75b261 to blathers/backport-release-23.2-116316: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
@adityamaru Heads up that your auto-backport failed here. |
Previously, the codec selection logic only looked at a single manifest to determine whether the backup data requires a system or tenant codec to be read. In older release branches if the full backup was empty (but a subsequent incremental was not) then we could incorrectly use a system codec for a backup that actually required a tenant codec.
To fix this we now process all the manifests in the backup chain and use the first non-empty manifest.
Fixes: #115773
Release note(bug fix): an empty full backup, followed by non-empty incrementals taken inside an application tenant may be unrestoreable because of the use of an incorrect SQL codec