rishi-rana commented on PR #39882: URL: https://github.com/apache/shardingsphere/pull/39882#issuecomment-5840812386
Thanks. Both points are addressed in b6cb07f42e456c1de4c7b41c9150a046d16b51a8. **1. Transactional DDL contract** The contract is now: - Permitted inside a transaction: `CREATE TABLE`, `ALTER TABLE`, `DROP TABLE` and `RENAME TABLE`. - Transaction types: LOCAL and XA. BASE is excluded. You were right that the non-XA branch did not exclude it, so it inherited table DDL support from the local branch; it now has its own branch reproducing the previous behaviour. - Still rejected with SQLSTATE `0A000`: schema DDL, view DDL, index DDL, and every other metadata-changing statement. - Backends: PostgreSQL only; openGauss does not set the new capability. `limitations.en.md` and `limitations.cn.md` are updated for LOCAL, XA and BASE, and the release note now says "table DDL". On backend scope: I have left `isSupportTransactionalDDL` as `false` for SQL Server. It permits metadata-changing DDL in a transaction today and its DDL is transactional, so #35166 appears to apply to it, but I would rather you decide whether that belongs here or in a follow-up than change a dialect the issue did not report. **2. Protocol dialect against routed storage dialect** The capability is now resolved from the storage units the statement is routed to, requiring every routed unit to keep DDL transactional. Mixed routes and any implicit-commit storage fall back to the previous behaviour, so a PostgreSQL-protocol session routed to MySQL behaves as a MySQL session does. One predicate, `isDeferrableTableDDL`, feeds both the permission check and the deferral, so the two cannot disagree. New unit scenarios cover PostgreSQL protocol with non-transactional storage in LOCAL and XA, and PostgreSQL in BASE. **A correctness problem this surfaced** While covering `CREATE TABLE` I found that `ContextManager#reloadTable` cannot register a new table. With no data node, `SchemaMetaDataUtils#addOneActualTableDataNode` falls back to the first storage unit, so a table created on another unit is not found and `persistTable` drops it instead; it also bypasses `MetaDataManagerPersistService#createTable`, so a single table is never registered in the single-rule configuration. The deferred tables are therefore reconciled through a new `ContextManager#reconcileTable`, which uses the existing loader and manager service to create, alter or drop each table according to its committed state. A reconcile problem is logged rather than propagated, so it can no longer surface as a commit failure after the transaction has committed. A rollback now discards the deferred tables without reconciling, since deferral only happens when the DDL was transactional and therefore undone. **A limitation worth recording** A statement cannot reference a table created earlier in the same transaction: `BEGIN; CREATE TABLE t (...); ALTER TABLE t ...;` fails on the second statement, because the deferred meta data of `t` is only applied when the transaction ends. This is inherent to deferring the refresh, and it means the create-then-rename sequence from your earlier review is not reachable. It is documented in both limitations files. **Verification** - `./mvnw -pl mode/core,proxy/backend/core,database/connector/dialect/postgresql test` - 596 tests, 0 failures. - Transaction E2E on `postgres:12-alpine` - passed for LOCAL, XA with Atomikos and XA with Narayana, including a new case that creates a table inside a transaction and asserts it is present in meta data after commit. - Transaction E2E on `mysql:5.7` - passed. - `spotless:check` and `checkstyle:check -Pcheck` across all 12 touched modules - 0 violations; `apache-rat:check -Pcheck` - passed. -- 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]
