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 ✨ Introduce a Connection::executeDdlStatement method RTBC

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.