Introduce a Schema::executeDdlStatement method

Another opportunity for such a method: disable square brackets replacement, and fix #3391222: Postgres Connection::quoteIdentifiers() should not quote square brackets when they are part of normal SQL

Rebased, now up for review.

Added draft CR https://www.drupal.org/node/3402926

rebased

Rebased, after #3159113: PostgreSQL Duplicate table error for alter index queries

Rebased. Cannot find in the MR the comments mentioned in #16. Maybe a review not published?

Applied the suggestions and answered the question. Back to RTBC, no code was changed.

Needs to be adjusted after #3410476: Add event for statement execution failure, on it.

Done. Added dispatching of StatementExecutionFailureEvent for the case of failure when executing SQL via Connection::executeSql()

Replied to the inline comment. Please post something on the d.o. issue too when commenting the MR so that it becomes visible on the d.o. dashboard, otherwise it's just casual to get to know about the MR comment. Thanks!

Failures seem rather unrelated,

PHP Fatal error: Constant expression contains invalid operations in core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php on line 43

This was set to Critical because of #3405976: Transaction autocommit during shutdown relies on unreliable object destruction order (xdebug 3.3+ enabled), but that got solved anyway differently in the meantime. Being a Feature request, I think "Normal" is appropriate at this stage.

I think I tried that somewhere. It doesn’t work. You get anyway an error on the DML transaction, even if you keep the DDL one separate.

In fact what happens when you perform a DDL is that you are changing (MySql) the internal information schema of the database. That is enough to cause an error 1412 - Table definition has changed, please retry transaction - on ANY other active transaction in the server in that moment.

That’s IMHO a sufficient argument to reconsider, for example, doing on-the-fly table creation. DDL should be really limited to maintenance time…

FWIW I do not really feel this as complex, honestly. It only affects database drivers - hopefully no one performs DDL from outer code, only via calling Schema methods.

Vice versa, not doing this makes life complicated for the potential mysqli driver.

And yes, #3410312-13: Flood database backend ::isAllowed() should call ::ensureTableExists() and #3410480: [META] Use events in Database Schema operations have started a discussion for doing something different for delivering table creation; that for me is independent and should happen anyway. But I see that quite far away for the moment.

@alexpott

it's not going to make MySQL actually do transactional DDL, I think what it does, is commit any open transactions, do the DDL, and then re-open the transactions

There's no way to make MySQL do transactional DDL, and we should not assume this will happen.

What happens today in HEAD is that once a DDL is executed in the middle of a transaction, MySQL (the database engine, not the driver) actually closes that transaction. How we react to that fact is what we can work on.

The MR does not do any commit/reopening of the transaction, only accounts for the fact that the client transaction got closed by ensuring the TransactionManager voids any open Drupal transaction object so that when they get destructed, they will not try to perform SQL transaction statements on the client connection anymore. IIRC, PDO MySql would fail explicitly if it tried that (because it keeps track of the state of the client transaction in the driver), Mysqli would fail silently (because it does not).

Now time has passed since I wrote this - I think yes, the MR would run the post transaction callbacks when the transaction manager detects the DDL (since the client transaction is closed at that point). If it's more desirable to wait until destruction of all voided Drupal transaction objects, I think we can still do that.

Working on #40.

If I am not mistaken, the behavior in #38

If DDL occurs mid node save when do the post transaction callbacks fire? Are they moved from after the entity has completed being saved and are now triggered in the middle?

is confusing even in HEAD today, for MySQL - $this->voidClientTransaction(), that is processing the callbacks, is called not upon commit of the root transaction but also in other cases.

We are missing proper test coverage.

Filed #3440848: Ensure post transaction callbacks are only at the end of the root Drupal transaction, needs to be solved before this.

The SQLite test failure is unrelated; should be fixed by #3458975: GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled fails on sqlite.

I'd like to add more specific tests for the case of attempted rollback after a DDL statement is executed. On that.

Back to NR.

Adjusted draft CR to address #37.

#40 was addressed by #3440848: Ensure post transaction callbacks are only at the end of the root Drupal transaction.

If open questions/doubts yet, can they please be summarized again.

From my POV, this is blocking #3398767: Allow returning explicitly to the prior nesting level in transactions (aka allow explicit COMMIT in Transaction objects) and the stream to remove commit-on-destruction that caused a lot of troubles recently with XDebug 3 unpredictably changing object destruction order, and #3259709: Create the database driver for MySQLi and the stream to introduce async querying.

Addressed comments and opened #3482452: Revise Truncate with regards to autoincrement reset and returned value for follow-up.

Working on this.

@alexpott thanks for the question in #55

what happens in head when we try to rollback after a DDL query on MySQL?

In HEAD a rollback after a DDL silently fails, without exception thrown. At that point in code the autocommit occurred already, and any DML change was committed.

IMHO trying a rollback on a transaction that was already committed SHOULD throw an exception, as obviously there's an inconsistency between the transaction state and the intent of the rollback, and by letting it fail silently we can incur in data corruption.

The test for the earlier version of the MR demonstrate that, at least in test covered HEAD core, we were not having the case of a rollback after a DDL, as no exceptions appeared during the test runs (besides those captured in the new tests).

However, rightly, throwing an exception means a behavior change, and we cannot assume that it will not affect contrib. So in the latest version of the MR I propose to trigger a deprecation for now, to be converted to an exception in drupal 12.

Looking at the MR currently, I realize that we may avoid entirely surfacing a new API method: Connection::executeDdlStatement is only called

• from Schema methods, and a protected method on the abstract class could do instead
• from Truncate, and voiding directly the transaction is also possible with existing API

I'll implement this in the next iteration of the MR.

Done #58. Now the changes are really reduced to the Schema class only, and internal to it. Plus obviously tracking of the occurred autocommit in the transaction manager. It even turns out we do not need any change to Truncate, since there is safety code that uses the 'TRUNCATE' DDL statement only if the Drupal truncate happens OUTSIDE of a transaction.

Addressed #60, changed the deprecation to a user warning. Now to #61!

@alexpott re #61

what happens to post transaction callbacks when the rollback fails. Ie. what happens in HEAD and what happens after this MR?

I was expecting this question... :)

In HEAD it's a mess now, and test coverage is missing: pgsql and sqlite, allowing transactional ddl, process the callback with $success = FALSE to indicate a rollback happened; mysql processes the callback with $success = TRUE to indicate a commit happened. Which, it's the reality in fact.

So core databases currently react differently. See the attached test only patch, testRootTransactionEndCallbackCalledAfterDdlAndRollbackForTransactionalDdlDatabase and testRootTransactionEndCallbackFailureUponDdlAndRollbackForNonTransactionalDdlDatabase.

Good news is that the MR adds test coverage, and when we have an agreed solution we can adjust the test accordingly and have a lighthouse for the future.

EDIT - ah sorry, what the MR is currently doing is cleaning up the callbacks so they are NOT executed at all. fwiw. Sure we need to decide what to do and change accordingly.

#64 or alternatively, fail (silently for now) any attempt of rollback after a DDL in any database, commit the transaction on the spot on the attempt and run the callbacks with $success = TRUE.

I know it's on the opposite side of things but at least it would make the behaviour (and data) consistent across all dbs.

So, implemented #64 in the MR.

What I mean is 'hey, you cannot rollback, we have already committed your transaction and will process your callbacks as per successful commit mode'. Note that callbacks are only processed upon destruction of all transaction objects in the stack.

Anyway, #64 is OK for me, implemented in the MR and a step forward vs current. This can also unblock working on explicit commit vs commit-on-destruct and maybe in the future once that will be in we may reconsider.

Reverted not relevant code that was emptying post-transaction callbacks.

#70 yes, from that perspective I agree. If $success === TRUE --> the transaction was successfully committed; $success === FALSE --> transaction was rolled back or failed for any other reason. Is that it? If so then I think we should document it clearly somehow - it's not ATM, unless I am mistaken.

OK we are on the same page. My point is to clarify what ‘transaction was successful’ means. It means, as we speak, ‘transaction was successfully committed’. Will adjust in next iteration of the MR.

Documented #72 #73, did also some more doc edits - the Transaction class was still hinting at dbs that do not support transactions which is no longer a thing in Drupal.

Working on latest comment.

Updated CR to reflect the new method is now a protected method of the Schema class.