Removal of unnecessary savepoints from PostgreSQL

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. 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.

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.

I think since we are removing the savepoint handling from all queries we would want a change record to tell contrib. and custom module maintainers that use the core pattern that they will need to use savepoint handling or not use that core pattern anymore to maintain compatibility.

Flipped this to critical as at least one other critical related issue has been postponed because this one will fix it.

Doesn't hit critical

The service provider seems risky, because what if another module also wants to swap out one of those services? This doesn't feel very scalable unfortunately; the fact that we also have to swap workspaces.menu.tree_storage here points to that.

What if we added a recoverable or similar option to queries, which informs the Postgres driver (and others that might need to know) that it must wrap the query in a savepoint but leave the transaction usable?

Or, perhaps core explicitly needs to use transactions for these cases?

Added a draft correction targeting the existing issue branch, with a regression for TableVerifyTrait.

Create a cache bin inside a transaction, roll back its creation, then reuse the same backend inside an active transaction. The retained table-existence entry skips verification, and the next write fails with SQLSTATE 25P02. Both root transaction and nested savepoint rollback reproduce this.

The correction retains verification only outside transactions. Both new tests fail without it and pass with it: 2 tests, 14 assertions. Local PostgreSQL module tests pass with two skips. Remote PHPCS, PHPStan, PostgreSQL kernel and functional jobs pass.

Full CI remains red: the PostgreSQL JavaScript retry still fails one ThemeSettingsFormTest case after configuration submission. Its cause remains unestablished.

The draft adds table-existence queries during transactions, so its performance tradeoff needs review. The service-replacement question in #21–22 remains open.

Follow-up to #24: reproduced the ThemeSettingsFormTest failure locally. The hidden field and successful-save message appeared immediately after the failed lookup. The draft now waits for successful submission before checking file permanence. Ten local repetitions passed (20 tests, 170 assertions), and both cases pass in PostgreSQL CI.

All required CI jobs pass after separate CKEditor, media-library and layout-builder failures cleared on unchanged retries. Allowed PHP 8.6 unit warnings remain.

The correction has material cost. A local PostgreSQL 18/PHP 8.5 cache-backend benchmark, with 14 samples of 500 operations per variant, measured transactional reads at 0.261 → 0.709 ms/op and writes at 0.526 → 1.324 ms/op. Query counts doubled from one to two per operation; outside-transaction counts were unchanged. These are operation timings, not whole-site results.

Keeping the correction in draft. Safe table-information caching or transaction/savepoint-aware invalidation needs consideration; root and nested rollback coverage should remain.

Replaced the per-operation checks in the draft with verification cached against transaction and schema versions. Root transaction completion, savepoint rollback, and schema invalidation force a fresh check. Ordinary operations and savepoint releases reuse it.

Eight regressions cover query reuse, root/nested rollback, another backend creating the table, aborted COMMIT, drops, and renames: 51 assertions pass. The full PostgreSQL suite passes 111 tests with two skips.

In an alternating local benchmark, warmed transactional reads fell from 0.510 to 0.199 ms/op and writes from 0.917 to 0.349 ms/op. Each variant used 14 samples of 500 operations. Query counts fell from two to one per operation, matching the original proposal. These are operation timings, not whole-site results.

Required CI jobs pass after unchanged retries. The missing cache_file_parsing table failure matched the symptom in #3621147 and did not recur. Allowed PHP 8.6 unit warnings and a non-blocking log-copy warning remain. The MR remains draft for design review.

I have re-read the most recent comments and related issues.

#24 to #28 look like progress.

edit: It looks safe to say #21 has been resolved.

Returning to #22:

What if we added a recoverable or similar option to queries, which informs the Postgres driver (and others that might need to know) that it must wrap the query in a savepoint but leave the transaction usable?

Or, perhaps core explicitly needs to use transactions for these cases?

• Looks like we have not yet added a recoverable option. Has that turned out to be unnecessary? Or is it still a possibility?
• Given the apparent progress made by jmcerda in his draft, is it time to merge in that code? If we are not ready for that, why? What more is required it?
• Where longwave states, 'Or, perhaps core explicitly needs to use transactions for these cases?' Has that question been resolved by #24 to #28?
• The IS predicts performance improvements. Have those been achieved? Or is there more work to do to? What more needs to be done?

Re-reading the IS and #28, it looks like daffie's new MR!17042 is close to RTBTC. However, jmcerda's draft MR contains more feedback on performance statistics. However as asserted in #28 the pipeline is all green except for one test. daffie has created a novice follow-up to zap that gnat. It would still be useful to get clarity on the questions asked in #29.

@oily Thanks for following up. !17042 addresses the service-override concern in #21, but there are still a few things to resolve before I would consider it ready.

My draft fixes a specific rollback problem: a backend creates a table inside a transaction, that creation is rolled back, but the backend still remembers the table as existing. Reusing the same backend then skips verification, and a query against the missing table aborts the transaction.

Reading the current !17042 diff, the shared TableVerifyTrait still caches that result without rollback invalidation. Moving the trait therefore does not resolve this case. I have not yet rerun my regression tests against !17042. The draft targets the earlier branch, so the correction needs adapting to the shared trait and testing there. The invalidation approach also needs design review; whichever implementation we use should retain coverage for both root transaction and nested savepoint rollback.

On the recoverable option from #22: !17042 does not add one. It checks for missing tables before querying them and uses an explicit savepoint around table creation to handle a race with another process. That addresses one recovery case, but code that expects another kind of query failure and then continues an existing PostgreSQL transaction still needs a savepoint established before the query, followed by rollback to that savepoint on failure. Catching the PHP exception alone does not restore the transaction.

So the explicit-transaction approach is present in !17042, including a recovery test, but the broader API question remains open. A recoverable option is still a possibility. My draft does not settle that choice, and contrib/custom-code callers will need a change record explaining the resulting behavior.

The performance numbers in #25–26 compare two ways of fixing the rollback problem. The first correction checked table existence on every transactional operation, adding substantial overhead. The revised draft reuses verification until transaction or schema changes invalidate it. That restored the measured warmed cache operations to one query per operation.

Those results show that the revised correction avoids the repeated-check penalty. They do not independently confirm the issue summary's speedups against unmodified core, and they are not benchmarks of !17042 or whole-site performance. We should repeat the comparison on the combined implementation.

My next step would be to carry the rollback correction and tests into !17042. Alongside that, we still need agreement on the recovery behavior, a change record, and performance verification of the combined changes. The unrelated failing test is not the only remaining item.

Re: #32 and 33, thank you for the detailed responses. Moving to RTBTC. I won't be hurt if someone changes it back to Needs Review or other as it is quite a big, complex issue. But hopefully can get more feedback from a maintainer.

Verified the incorporated changes at b89a9bae97. The rollback and PostgreSQL lock tests pass locally: 13 tests, 65 assertions. The broader PostgreSQL/transaction suite passes 133 cases; SQLite transaction/cache tests pass 43, with existing skips.

Required CI and the PostgreSQL child pipeline pass. Allowed PHP 8.6 unit warnings remain. I closed my overlapping draft !2 as superseded.

I repeated the cache benchmark against the incorporated implementation, alternating with core d107690b1c: 14 samples of 200 operations per scenario, PostgreSQL 18/PHP 8.5. Warmed transactional reads measured 0.532 → 0.195 ms/op; writes 1.502 → 0.276. Logged queries fell from three to one.

First use after commit or savepoint rollback adds verification. With a commit before every operation, writes measured 1.556 → 1.728 ms/op. These are operation timings, not whole-site results.

Native and fallback merge paths still catch integrity exceptions and retry without an explicit savepoint. I have not exercised those concurrency cases. The broader recovery contract and change record remain open for framework review.

Following up on #36, I reproduced both merge retry failures at b89a9bae97 with PostgreSQL 18 and PHP 8.5.

One connection inserts a key without committing. A second connection attempts that key through Drupal's merge API. The first commits only after PostgreSQL confirms the second is blocked on its transaction.

Inside an enclosing transaction, both native MERGE and the generic fallback fail with SQLSTATE 25P02 during retry. A subsequent SELECT 1 also fails before cleanup. Supplying an explicit serial key exercises the fallback.

I repeated five cases three times against both branches. Baseline d107690b1c passes all five. This branch fails the two transactional merge cases consistently. Autocommit merges and transactional upsert pass on both.

This fits the direction in #33: native conflict handling works for the simple keyed-write control. We still need to preserve Drupal's merge semantics, including the fallback, before treating these retries as safe.

Moving back to Needs work for these reproducible transaction failures.

The correction and reproducer are in draft !3, targeting the current issue branch at b89a9bae97.

Native MERGE and the generic fallback insert establish a savepoint inside an existing transaction, then restore it before retrying a conflict. Ordinary query savepoints remain removed. The shared merge class exposes its insert through a protected method for the PostgreSQL override.

The tests coordinate two database sessions through confirmed blocking. They also check that unrelated unique-constraint errors preserve their original SQLSTATE and preceding writes, including nested transactions.

Local PostgreSQL validation passes: 162 tests, 1,668 assertions, four existing skips. SQLite merge tests pass: 10 tests, 40 assertions. CSpell, PHPStan, and PHPCS pass. CI, including its PostgreSQL child pipeline, is running.

This retains savepoint round trips for transactional native MERGE. The draft preserves existing merge behavior; it does not replace the full merge API with upsert. That tradeoff remains for maintainer review.

Main CI and the PostgreSQL pipeline now pass at a2d999025e. The allowed PHP 8.6 deprecation warnings remain.

I applied upstream revert cbef6228ca as a separate commit to clear the inherited text update failure discussed in #3621680. The text module also passes locally: 26 tests, 340 assertions. The merge correction remains isolated in c958f40cde.

The PostgreSQL JavaScript shard passed on retry after a workspace media-filter assertion failed. The earlier ManageDisplay element lookup failure also passed unchanged on retry. The new merge concurrency and constraint-preservation tests passed in both pipeline runs.

!3 is ready for review. Moving back to Needs review; the merge savepoint tradeoff and broader recovery contract still need maintainer review.

Re: #38 and #39 I have reviewed the code and the pipeline/ tests. There is quite a lot of new code in the latest commit. Whether it could be reduced somehow I am not sure, but a maintainer has a lot of detail describing what has been done and why. I think given the pipeline is green RTBTC so maintainer can evaluate.

@jmcerda Your comments read a lot like AI output. Please read the AI contribution policy, specifically around using your own words and not just copy and pasting from an LLM.

https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

@longwave. I am familiar with the policy. Thank you.