[PP-1] Introduce a Connection::executeDdlStatement method

Another opportunity for such a method: disable square brackets replacement, and fix 🐛 Postgres Connection::quoteIdentifiers() should not quote square brackets in array definitions Active

Rebased, now up for review.

Can we get a change record for the new method.

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

CR reads well, thanks for including the before/after examples I find those help a bunch.

rebased

Rebased, after 🐛 PostgreSQL Duplicate table error for alter index queries Fixed

I'm triaging RTBC issues → . I read the IS and the comments. Thank you for the well written issue summary. I didn't find any unanswered questions.

I read the MR (not a code review). I enjoyed reading the very clear comments, especially in an area that I don't know. I thought some comments needed a little grammar tweak so I left some suggestions. However, one is a question. I am going to set need work for these points.

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

@mondrake, thanks for the ping. Yes, I did forget to publish my comments. There are here now.

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

@mondrake said in #3405976-84: Transaction autocommit during shutdown relies on unreliable object destruction order → :

We are currently blocked by ✨ [PP-1] Introduce a Connection::executeDdlStatement method Active

As that issue is critical and this issue is a blocker for that issue, this issue should also be critical.

PHP 8.4 just commited https://wiki.php.net/rfc/pdo_driver_specific_subclasses

Needs to be adjusted after ✨ Add event for statement execution failure Active , 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!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Is it an issue that the mariaDB job failed?

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

That case seems all threads have been addressed. Seems to have gone through a few eyes and being crticial going to mark now.

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

This is just a thought... but can we open another connection to do the DDL on MySQL so that we don't have to think what happens to in play transactions?

And in fact can we do that on all databases just so it's the same everywhere?

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…

@mondrake doesn't that mean we should consider not doing this and coming up with something else for the auto-table generation stuff. It feels like this approach introduces more complexity but doesn't actually give us a solution because we just can't do what we want to do. Which is have transactions and DDL at the same time on all DBs... there's no way to make it work the same or even similarly on all db types. And doing a commit and then re-opening the transaction feels like a not good place to be.

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 ✨ Use events in Database Schema operations Active 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.

The CR says

This will allow proper handling of transactions when executed for databases that do not support transactional DDL

But it does not detail what that proper handling is. Afaics 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. I think what it means by proper needs to be detailed on the CR. Also I think that as this still does not not ensure matching behaviour between the drivers we need to be very careful as to how we proceed. Putting back to needs review in order to get the updates to the CR and for more review and thought about this issue.

Looking at the test I think this issue will introduce new failure modes that I'm not sure about. 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?

@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.

I think we do want to ensure that the moment the post transaction callback runs it consistent accross db types. And regardless of whether DDL has occurred. Interestingly I think that most things that cause DDL in a transaction should probably be moved to a post transaction callback :)

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 🐛 Ensure post transaction callbacks are always executed ONLY at the end of the root Drupal transaction Needs review , needs to be solved before this.

🐛 Ensure post transaction callbacks are always executed ONLY at the end of the root Drupal transaction Needs review has landed.

The SQLite test failure is unrelated; should be fixed by 🐛 GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled fails on sqlite Needs review .

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.

MariaDB failure appears to be random.

Believe all feedback on this one has been addressed

Adjusted draft CR to address #37.

It feels like @alexpott still has doubts/open questions here. Not sure of what the correct status is, but I don't think this is RTBC just yet.

#40 was addressed by 🐛 Ensure post transaction callbacks are always executed ONLY at the end of the root Drupal transaction Needs review .

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

From my POV, this is blocking ✨ Allow explicit COMMIT in Transaction objects Needs review and the stream to remove commit-on-destruction that caused a lot of troubles recently with XDebug 3 unpredictably changing object destruction order, and 📌 [PP-1] Create the database driver for MySQLi Postponed and the stream to introduce async querying.

Posted some more thoughts. My main concern now is the code duplication and way this is mixing responsibilities that were \Drupal\Core\Database\StatementWrapperIterator::execute() in the Connection class.

Addressed comments and opened 📌 Revise Truncate with regards to autoincrement reset and returned value Active for follow-up.

So the code looks much better now - thanks @mondrake for addressing my feedback.

I guess my question now is what happens in head when we try to rollback after a DDL query on MySQL? We're adding a test here that shows with this change an exception is thrown. My concern is that this does not happen in HEAD and this behaviour change will have consequences.

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.

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.

Hmmm... this is really really hard because we cannot possibly make our db drivers work the same way. I think we should not be doing a deprecation here - the rollback is silently failing at the moment. I think it would be better to log a warning that the transaction rollback has failed. And then in a follow-up we can consider turning this into an exception.

@mondrake the latest MR is getting really nice from a public API change perspective. Great work!

One more question related to #60 what happens to post transaction callbacks when the rollback fails. Ie. what happens in HEAD and what happens after this MR?

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

@alexpott re #61

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.

So for #63 for me I think the correct behaviour is psql and sqlite's but the problem is when the MySQL behaviour happens we're essentially broken. I'm really not sure about what we should do when the MySQL behaviour happens and we're trying to rollback a non existing transaction. The commit that has occurred means that the site is in an inconsistent state. I think the most consistent thing is to trigger the callbacks with success = false too...

#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.

But given allegedly trying to a rollback how does triggering with $success = true make sense? There's been an exception thrown during the transaction and we can't be sure what's happened.

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.

But $success is for post transaction callbacks is supposed to indicate whether it's been successful. It hasn't been even though on mysql there's been a partial commit.

#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.

We have

   * An argument is passed to the callbacks that indicates whether the
   * transaction was successful or not.

In the docs for \Drupal\Core\Database\Transaction\TransactionManagerInterface::addPostTransactionCallback. If we're calling them after a rollback - even if the rollback cannot be performed because MySQL does not support transactional DDL then I think $success should be FALSE.

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.

Looks good to me.
The CI pipeline is green for all supported databases.
For me it is RTBC.

I have one concern left - see question on MR. Anyone can rtbc once the question is answered.

Working on latest comment.

The point of @alexpott has been addressed.
Back to RTBC.

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

Getting this into 11.2.x nice and early - before it is open. So it has a long time in the up-coming branch.

Committed 3004fc8 and pushed to 11.x. Thanks!

mondrake → closed merge request !4699