rishi-rana commented on PR #39882: URL: https://github.com/apache/shardingsphere/pull/39882#issuecomment-5942996152
Thanks, both correctness findings are confirmed and addressed in 709aaf76cd7. **1. Deletion key** `ContextManager.reconcileTable`'s drop branch now resolves the table through `TableRefreshUtils.getTableLoadCandidateName`, the same canonical-storage-name path the create/alter branches already use, instead of the raw parsed identifier. Added `ContextManagerTest.assertReconcileTableDropsUsingCanonicalStorageName`, which fails against the previous code with the exact mismatch you found (`dropTables` called with `FOO_TBL` instead of `foo_tbl`). **2. Data source attribution** Each deferred table is now matched to its own `RouteUnit` by logic table name (`RouteUnit.getLogicTableNames()`), falling back to the first route unit only if none matches. This required changing `DeferredMetaDataRefreshContext.add` from taking a batch of tables with one shared data source to one table at a time, so each gets its own. Added `StandardDatabaseProxyConnectorTest.assertExecuteDefersMetaDataRefreshUsingEachTableOwnRouteUnit`, which reproduces your `[ds_1.public.b]` → `[ds_1.public.b, ds_0.public.b]` scenario and fails against the previous code for the same reason. **4. Declaration order** - `ContextManager`: `reconcileTable` moved after `persistTable`, restoring adjacency with `reloadTable`. - `StandardDatabaseProxyConnector`: `isDeferrableTableDDL` and `isCurrentTransactionCommitRequired` swapped to match their call order in `doExecute`. - `PostgreSQLDDLMetaDataRefreshTestCase`: the third phase moved to be declared third, matching `executeTest`'s call order. Verification: 112 unit tests pass across `mode/core` and `proxy/backend/core`; `test/e2e/operation/transaction` test-compiles; `spotless:check checkstyle:check -Pcheck` reports zero violations on all three modules. **3. Support contract** I don't have authority to settle this myself, so I'd like explicit sign-off before treating it as decided. The contract as it currently stands in `docs/document/content/features/transaction/limitations.en.md` and in this PR's code is: - Permitted inside a transaction: `CREATE TABLE`, `ALTER TABLE`, `DROP TABLE`, `RENAME TABLE`. - Transaction types: LOCAL and XA. BASE is excluded. - Backend scope: routed storage units that report `isSupportTransactionalDDL()`, checked per route unit. - A table created earlier in the same transaction is not visible to a later statement in that transaction (deferred refresh only runs at commit). @terrymanu could you confirm whether this is the scope you're willing to accept, or whether any of the four points above should be narrowed, widened, or deferred to a separate PR? I'll update the docs and/or code to match whatever you decide. -- 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]
