rishi-rana commented on PR #39882: URL: https://github.com/apache/shardingsphere/pull/39882#issuecomment-5778867536
Both points are fair. Here is the contract I propose, so the boundary is agreed before I change anything further. **1. Supported statements and transaction types** Proposed contract: - Permitted inside a transaction: `CREATE TABLE`, `ALTER TABLE`, `DROP TABLE` and `RENAME TABLE`. - Transaction types: LOCAL and XA. - Still rejected with SQLSTATE `0A000`, unchanged from today: schema DDL, view DDL, index DDL, and any other metadata-changing statement. - Backends: PostgreSQL only. openGauss keeps the current restriction because it does not set the new capability. If that is the boundary you want, I will align the published contract with it: - `docs/document/content/features/transaction/limitations.en.md` and `limitations.cn.md`: narrow the LOCAL bullet so it states that only the four table DDL statements are supported for PostgreSQL and that other metadata-changing DDL is still rejected, and update the XA bullet that refers to it. - `RELEASE-NOTES.md`: change "DDL" to "table DDL" so the entry matches the code. One question inside this point. Should the contract be written as "PostgreSQL" or as "backends whose DDL is transactional"? SQL Server is the case that decides the wording: it permits metadata-changing DDL in a transaction today through `isSupportMetaDataRefreshInTransaction`, and its DDL is transactional, so the stale meta data in #35166 appears to apply to it as well. I have left `isSupportTransactionalDDL` as `false` for it in this PR, and I am happy either to keep the contract PostgreSQL-only or to include SQL Server here or in a follow-up, whichever you prefer. **2. Protocol dialect against routed storage dialect** You are right that both decisions currently read the statement dialect, and that this is wrong for DB Gateway. Proposed rule: - Resolve `isSupportTransactionalDDL` from the routed storage unit types, through `database.getResourceMetaData().getStorageUnits()` keyed by each route unit's actual data source name, in the same way routing already resolves the storage dialect. - Permit and defer only when every routed storage unit is a backend whose DDL is transactional. - If any routed storage unit is not, including a mixed route, behave exactly as today: the permission decision uses the existing options and the refresh happens immediately. A PostgreSQL-protocol session routed to MySQL storage therefore behaves as a MySQL session does. - Use that same resolution for the permission gate and the deferral decision, so the two cannot disagree about a given statement. `ProxySQLExecutor#checkExecutePrerequisites` is called after `generateExecutionContext`, so the route context is already available at that point and only its signature needs to widen. For coverage I would add unit tests for a PostgreSQL-protocol session routed to MySQL storage, and for a mixed route, asserting that DDL is rejected rather than deferred in both. Please confirm or correct these two, and I will implement and update the documentation accordingly. -- 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]
