JingsongLi commented on PR #10270:
URL: https://github.com/apache/paimon/pull/10270#issuecomment-5936470741
I found three introduced failure/isolation problems. The normal JDK 8
reactor run of `JdbcCatalogTest` passed all 81 tests; the additional
reproductions below use actual local SQLite metadata and compare with the
merge-base `JdbcCatalog` implementation.
**[P1] Delete data only after metadata deletion succeeds —
`JdbcCatalog.java:308`.** The directory is now removed before the first JDBC
DELETE. If that DELETE fails (for example, permission denial or a
storage/connection failure), the operation throws but registered tables have
already lost their schema and data. I created and committed a real row `42`,
then used a SQLite trigger to reject deletion from `paimon_tables`: HEAD
reports the DROP failure and still lists the table, but its entire directory is
gone; the same program with the merge-base catalog preserves the table and
reads row `42`. Please preserve files when metadata deletion fails and perform
cleanup after successful metadata removal. `dropTableImpl` already uses this
ordering.
**[P1] Restrict cleanup to this catalog's owned tables —
`JdbcCatalog.java:308`.** JDBC SQL filters by `catalogKey`, but
`newDatabasePath(name)` is only `warehouse/<name>.db`. Different catalog keys
can therefore have independent databases and different tables in the same
warehouse directory. In a real two-catalog test, A and B share the JDBC
database and warehouse, both create `db`, and only B owns `b_table` with
committed row `42`. A's `listTables("db")` is empty, so `A.dropDatabase("db",
false, false)` succeeds, then HEAD deletes B's files while B still lists its
registered table. The merge-base preserves and reads B's row. The documented
catalog-key isolation must survive a DROP in another catalog; recursively
deleting the shared directory is unsafe. Enumerate and clean this catalog's
registered table paths, preserving other catalogs and recoverable unregistered
tables.
**[P2] A thrown INSERT call does not prove registration failed —
`JdbcCatalog.java:585–586`.** `JdbcUtils.insertTable` executes an
auto-committed INSERT inside try-with-resources. If the INSERT commits and
`PreparedStatement.close()` throws, the assignment to `registered` never
happens, so this new catch deletes a table that is already registered. I
injected a close failure *after* the real SQLite INSERT/statement close: HEAD
still lists the committed table row but deletes its schema; the merge-base
leaves it registered and readable. A lost response after commit has the same
ambiguous outcome. Please confirm persistent registration/transaction rollback
before removing the created directory, rather than treating `registered ==
false` as proof that no catalog row exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
