[core] Fix JDBC catalog drop, alter and create failure handling - #10270
Open
LuciferYang wants to merge 2 commits into
Open
LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
dropDatabaseImpl deleted only the JDBC row sets and left the warehouse database directory behind, unlike the file system catalog, so after a cascade drop re-creating the database and a same-name table failed because the schema directory already existed. Delete the database directory too. alterDatabaseImpl never checked that the database exists: an ALTER on a missing database inserted property rows, silently materializing a phantom database. Throw DatabaseNotExistException first. createTableImplWithLock cleaned the committed schema directory only when insertTable returned false, but the realistic failure is the SQLException it throws; clean the directory in the catch as well — but only until the table row is committed, since the table keeps working when a later step like the property sync fails. Assisted-by: GLM-5.3
createTableImplWithLock cleaned the table directory on any pre-registration failure. When createTable fails because an on-disk schema already exists (table present on the file system but missing from the JDBC catalog, a state repairTable is meant to recover), that recursively deleted the pre-existing table's data. Guard the cleanup with a schemaCreated flag so only a directory created by this call is removed. Add a test pinning that the pre-existing directory survives and stays recoverable via repairTable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
JdbcCatalogmishandled three failure paths.Cascade
dropDatabaseImplremoved only the four JDBC row sets and never deleted the database directory, leakingwarehouse/<db>.db/(schema and data) and desyncing the catalog from the filesystem. Recreating the same database and table then collided with the staleschema-0. It now deletes the directory, matchingFileSystemCatalog. This runs only after thedropDatabaseoverride has already done the existence and cascade/empty checks, so the recursive delete is safe.alterDatabaseImpldid no existence check: altering a missing database withignoreIfNotExists = falseinserted a property row and returned success instead of throwingDatabaseNotExistException, anddatabaseExiststhen reported the database as present, materializing a phantom database. It now verifies existence first.createTableImplWithLockleaked the created directory when a create failed after the schema was committed to the filesystem but before the JDBC row was inserted. It now removes the directory on that path, guarded so it deletes only a directory this call created and then failed to register (aschemaCreatedflag combined with the existingregisteredflag). A table that already exists on the filesystem but is absent from the JDBC catalog (the staterepairTablerecovers) is left untouched.This closes #10269.
Tests
testDropDatabaseCascadeDeletesWarehouseDirectorypins that a cascade drop removes the database directory.testAlterDatabaseOnMissingDatabaseThrowspins that altering a missing database throws instead of creating a phantom entry.testCreateTablePreservesUnregisteredOnDiskTableOnFailurepins that a failed create does not delete a pre-existing on-disk table that is absent from the JDBC catalog, and that it stays recoverable viarepairTable.API and Format
No.
Documentation
No.