rishi-rana commented on PR #39882: URL: https://github.com/apache/shardingsphere/pull/39882#issuecomment-5754840541
Thanks, all three addressed in 3c86b0011aed3d8a4b0b8e52baad4b5dd1fed6bd. **Scope of this revision** This revision changes behaviour for one case only: table DDL executed inside a transaction on PostgreSQL. `CREATE TABLE`, `ALTER TABLE`, `DROP TABLE` and `RENAME TABLE` are now permitted there and their meta data refresh is deferred to the end of the transaction. Everything else is unchanged from master: - PostgreSQL schema DDL and view DDL in a transaction are still rejected with `TableModifyInTransactionException`. - Every other dialect keeps its existing permission rules and refreshes meta data immediately, because `isSupportTransactionalDDL` is `false` for all of them. - Behaviour outside a transaction is untouched for every dialect. **2. Immediate refresh for implicit-commit dialects** `DialectTransactionOption` gains `isSupportTransactionalDDL`, set only for PostgreSQL. The refresh is deferred only when that capability is set, so MySQL, MariaDB, Oracle, H2, SQL Server and Firebird refresh immediately as before and their meta data does not go stale while the logical transaction is still open. `MySQLDDLMetaDataRefreshTestCase` now asserts from a second connection that the altered column is in meta data while the first connection's transaction is still open, and again after its rollback. **1. Object type and identifier semantics** Deferral is restricted to table DDL. `ProxySQLExecutor` permits `CREATE TABLE`, `ALTER TABLE`, `DROP TABLE` and `RENAME TABLE` in a transaction when `isSupportTransactionalDDL` is set, and PostgreSQL keeps `isSupportMetaDataRefreshInTransaction` as `false`, so schema DDL and view DDL remain rejected in a transaction exactly as before this PR. Those statements therefore never reach the deferred context, which is why it reloads tables only. Identifiers are preserved: the deferred context stores the parsed `IdentifierValue` of each table and calls `ContextManager#reloadTable(ShardingSphereDatabase, String, IdentifierValue)`, so a quoted mixed-case table is reloaded as the quoted identifier rather than an unquoted one. `DeferredMetaDataRefreshContextTest#assertReloadKeepsQuotedIdentifier` covers this, and the connector test below uses a quoted table name. **3. Reachable DDL path and test standards** `assertExecuteDefersMetaDataRefreshForTransactionalDDLDialect` builds a real `AlterTableStatement` through `CommonSQLStatementContext`, so `PushDownMetaDataRefreshEngine#isNeedRefresh` returns `true` from production logic rather than from a stub, and the table identifiers come from the real `TablesContext`. The table name is quoted, so the test also covers the identifier case above. The remaining construction mock is `DatabaseTypeRegistry`, used to supply the `DialectTransactionOption` under test. PostgreSQL's `DialectDatabaseMetaData` SPI is not on `proxy/backend/core`'s test classpath, so the dialect capability is isolated at that boundary rather than by adding a module dependency for the test; `ProxySQLExecutorTest` isolates the same collaborator the same way. `times(1)` is replaced with plain `verify`. **Verification** - `./mvnw -pl proxy/backend/core,database/connector/dialect/postgresql test` - 593 tests, 0 failures. - Transaction E2E on `mysql:5.7` - passed. - Transaction E2E on `postgres:12-alpine` - passed. - `spotless:check` and `checkstyle:check -Pcheck` across all 11 touched modules - 0 violations; `apache-rat:check -Pcheck` - passed. **On the intended scope** Two questions, since both go beyond the reported case and I would rather agree the boundary than assume it. Should the deferral eventually cover the remaining metadata-changing statements rather than table DDL alone? Doing so needs a deferred representation carrying object type, and a view-level reload on `ContextManager`, which is why this revision stops at tables. Should other backends with transactional DDL opt in as well? SQL Server looks like the notable case: it permits metadata-changing DDL in a transaction today through `isSupportMetaDataRefreshInTransaction`, and its DDL is transactional, so the stale meta data described in #35166 appears to apply to it as it did to PostgreSQL. openGauss rejects such DDL in a transaction today, so it is unaffected either way. I left `isSupportTransactionalDDL` as `false` for both rather than change dialects the issue did not report, but I am happy to set it where you think it belongs, in this PR or a follow-up. -- 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]
