sql/catalog/dbdesc: repair old descriptors corrupted due to DROPPED SCHEMA name#68738
Conversation
|
This change is |
8e67036 to
9d2a6f4
Compare
9d2a6f4 to
dbc80a0
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 11 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nihalpednekar and @sajjadrizvi)
pkg/sql/catalog/dbdesc/database_desc.go, line 441 at r1 (raw file):
// maybeFixDroppedSchemaName fixes the name of a dropped child schema in a // database descriptor.
This first sentence is too vague and might lead somebody to believe it's a very general function. How about:
// maybeRemoveDroppedSelfEntryFromSchemas removes an entry in the Schemas map corresponding to the
// database itself which was added due to a bug in prior versions when dropping any user-defined schema.
// The bug inserted an entry for the database rather than the schema being dropped. This function fixes the
// problem by deleting the erroneous entry.
func maybeRemoveDroppedSelfEntryFromSchemas(dbDesc *descpb.DatabaseDescriptor) bool {pkg/sql/catalog/dbdesc/database_desc.go, line 452 at r1 (raw file):
} if sc, ok := dbDesc.Schemas[dbDesc.Name]; ok { if sc.Dropped && sc.ID == dbDesc.ID {
I don't think the Dropped check is necessary. The database should never think of itself as a schema inside of itself.
pkg/sql/catalog/dbdesc/database_desc_builder.go, line 63 at r1 (raw file):
privsChanged := descpb.MaybeFixPrivileges(ddb.maybeModified.ID, ddb.maybeModified.ID, &ddb.maybeModified.Privileges, privilege.Database) nameChanged := maybeFixDroppedSchemaName(ddb.maybeModified)
nameChanged is a surprising name for this variable. The name of the descriptor did not change. Can you change it to something more descriptive like `re
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/dbdesc/database_desc_builder.go, line 63 at r1 (raw file):
Previously, ajwerner wrote…
nameChangedis a surprising name for this variable. The name of the descriptor did not change. Can you change it to something more descriptive like `re
heh didn't finish that. removedSelfEntryInSchemas?
sajjadrizvi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/dbdesc/database_desc.go, line 441 at r1 (raw file):
Previously, ajwerner wrote…
// maybeFixDroppedSchemaName fixes the name of a dropped child schema in a // database descriptor.This first sentence is too vague and might lead somebody to believe it's a very general function. How about:
// maybeRemoveDroppedSelfEntryFromSchemas removes an entry in the Schemas map corresponding to the // database itself which was added due to a bug in prior versions when dropping any user-defined schema. // The bug inserted an entry for the database rather than the schema being dropped. This function fixes the // problem by deleting the erroneous entry. func maybeRemoveDroppedSelfEntryFromSchemas(dbDesc *descpb.DatabaseDescriptor) bool {
Done.
dbc80a0 to
6e51cc4
Compare
This commit adds a mechanism to delete wrong schema-info entries from database descriptors as part of post-deserialization changes. Release note: None
…info entries This commit adds a test for fix_descriptor_migration to validate that a corrupted database descriptor is fixed during migration. Release note: None
6e51cc4 to
88bf4ba
Compare
|
TFTR! r=ajwerner |
|
bors r=ajwerner |
|
Build succeeded: |
#63119 fixes a bug that corrupts a database descriptor when a child schema was
dropped, adding an entry in schema-info structure erroneously with database name
instead of schema name . Although the bug was fixed , there can be database backups
with corrupted descriptors.
This commit adds a post-deserialization function to repair a corrupted descriptor. Moreover,
it adds a test function to ensure that descriptors with such corruption are fixed during
migration.
Release note: None
Fixes: #63148