Postgres driver can duplicate mimic_implicit_commit

I am going to bump up the priority since essentially makes it impossible to save nodes while not an administrator.

To help this along I have replicated this with the following steps:

1) Install latest official / dev of Drupal 8.4.x
2) Install standard profile
3) Install + Enable small mimic_implicit module attached as a *.tgz
4) Add an editor user with the newly added editor role
5) Login as the newly created editor user w/editor role
6) Create a new piece of content of the newly added type "Landing"

You will recieve the error mentioned above and not be able to save the page

In essence my small module just adds an editor role + permissions + landing content type and simply adds the following hook:

use Drupal\Core\Session\AccountInterface;

/**
 * Implements hook_node_grants().
 */
function mimic_implicit_node_grants(AccountInterface $account, $op) {
  $grants = &drupal_static(__FUNCTION__);

  return $grants[$uid];
}

I was hoping porting the following logic to workbench_moderation would have worked but to no avail:

#2821716: Fatal error when viewing node with content moderation enabled if a module which implements hook_node_grants() is enabled

Hmm it doesn't seem like content_moderation has the same issue. I did trace the errors having to do with entityPresave() in workbench_moderation where this logic doesn't exist in content moderation entityPresave() anymore:

      // There's currently a bug in core where $entity->original always points
      // to the default revision, for now work around this by loading the latest
      // revision.
      $latest_revision = $this->moderationInfo->getLatestRevision($entity->getEntityTypeId(), $entity->id());
      $state_before = !empty($latest_revision) ? $latest_revision->moderation_state->target_id : NULL;
      // @todo: Revert to this simpler version when https://www.drupal.org/node/2700747 is fixed.
      // $state_before = isset($entity->original) ? $entity->original->moderation_state->target_id : NULL;

workbench moderation: http://cgit.drupalcode.org/workbench_moderation/tree/src/EntityOperation...
content moderation: http://cgit.drupalcode.org/drupal/tree/core/modules/content_moderation/s...

Can see a stack trace here: https://www.drupal.org/files/issues/behat-callstack.txt

Related #2981448: mimic_implicit_commit is already in use

We are honestly not entirely sure what changed in 8.7.2 to cause this to start biting us, outside of it being some combination of view_unpublished and entityqueue modues interacting badly with something in the update. However, being bitten is a good way to start looking at a solution, so here is a patch that is a first pass at attempting to resolve this issue. I took the approach to move the $wrap_with_savepoint logic in pgsql\Connection wrapping to a public method, which allows it to be called by the pgsql\Select::execute() to determine whether or not to make that addSavepoint() call.

Applied #11 patch, seems to work alright.

The patch at #11 just saved me as well. Functionally it works, I have not had time to review the code though.

A couple of minor issues here with the patch.

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -454,6 +442,36 @@ public function upsert($table, array $options = []) {
   +   * See https://www.drupal.org/project/drupal/issues/2920527.
   +   *
   

   I don't think we need this in core to point to an existing. issue. The functionality is already described in the database driver already.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -454,6 +442,36 @@ public function upsert($table, array $options = []) {
   +    // We need to wrap queries with a savepoint if:
   +    // - Currently in a transaction.
   +    // - A 'mimic_implicit_commit' does not exist already.
   +    // - The query is not a savepoint query.
   

   I think this comment should be in the function comment above.

One thing I am curious to see is if there are more failed transactions or less during the Drupal install process with this patch (using minimal for sanity).

Here I have made changes as suggested in comment #17.

Seems like the module has changed structure significantly. I tried replicating the patch but it is still failing. Drupal core version 9.4.0 currently.

I have uploaded the in progress version of the patch but it currently has the error Error: Call to undefined method Drupal\pgsql\Driver\Database\pgsql\Connection::wrapWithSavepoint() in Drupal\pgsql\Driver\Database\pgsql\Connection->query() (line 167 of /core/modules/pgsql/src/Driver/Database/pgsql/Connection.php).

Thank you for the patch, jakegibs617.

+++ b/core/modules/pgsql/src/Driver/Database/pgsql/Upsert.php
@@ -133,4 +133,33 @@ public function __toString() {
+  public function wrapWithSavepoint($query = NULL) {

It's because this method got added to Upsert rather than Connection class.

Ok for 9.4.0-dev Drupal core.
Attached is what seems to me as a working patch. Ready for others to review.

Patch in #25 had some issue, so rerolled the patch in #18 again. Also uploaded a diff file between #18 & #26 and a interdiff file between #25 & #26.

Thanks!

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

As a bug this will need a test case to show the issue.
Could the issue summary be updated to the default template showing proposed solution and remaining tasks please.
Tagged for manual testing for someone who uses postgresql.

Thanks @ankithashetty the patch #26 it's work for me!

I can't apply #26 on 10.1

adding a remake of 2920527-26.patch but fitting to druapl 10.2.2

Thanks @jakegibs617 that works for me.

@abelass are you able to clear the cache with the patch?
when I run drush cr
I am getting:
A transaction named mimic_implicit_commit is already in use. Active stack: 65b129b004e365.44430355\drupal_transaction > 65b129b020e3c2.42669084\mimic_implicit_commit

without the patch, if I log in as not an admin and edit a node then save I still see errors:

[Error] [node] Drupal\Core\Database\TransactionNameNonUniqueException: A transaction named mimic_implicit_commit is already in use. Active stack: 65b13376db52f8.60161248\drupal_transaction > 65b13376e58df9.82646498\mimic_implicit_commit in Drupal\Core\Database\Transaction\TransactionManagerBase->push() (line 220 of /core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php).
[Error] [php] Drupal\Core\Entity\EntityStorageException: A transaction named mimic_implicit_commit is already in use. Active stack: 65b13376db52f8.60161248\drupal_transaction > 65b13376e58df9.82646498\mimic_implicit_commit in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of /core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

looks like transactionLayers was deprecated.

!isset($this->transactionLayers['mimic_implicit_commit']) &&

Can you give this one a shot? 2920527-37.patch

@jakegibs617 sorry for my late answer, we only could test the patch yesterday. #37 worked fine in our tests, thanks.

I updated Group contrib to 2.3.1 and noticed this issue. I applied patch #37 and even updated Core to 10.4.1 but neither fixed the issue.

I see the same error after upgrading groups module and trying to migrate user realationship.

[error]  Drupal\Core\Database\TransactionNameNonUniqueException: A transaction named mimic_implicit_commit is already in use. Active stack: 677fe54f644a06.04798895\drupal_transaction > 677fe54f67f767.51173260\mimic_implicit_commit in Drupal\Core\Database\Transaction\TransactionManagerBase->push() (line 254 of /var/www/docroot/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php).

I tried patch from #37 - but this does not solve the error for me.

Thanks a lot Jake (@jakegibs617) for the contributed patch above at #37, the great documentation of the issue and getting the ball rolling with this patch.

I've tried to mention your work as much as possible in the MR to associate the credit.

Quick follow-up:
Created initial merge request above at #44:
1 - Rolled in patch from #37

2 - Added additional fix based on stack trace:

Drupal\Core\Entity\EntityStorageException: A transaction named mimic_implicit_commit is already in use. Active stack: 677ffe0d432d66.20858170\drupal_transaction > 677ffe0d4c76f6.93298422\mimic_implicit_commit in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
Drupal\pgsql\Driver\Database\pgsql\Connection->startTransaction('mimic_implicit_commit') (Line: 429)
Drupal\pgsql\Driver\Database\pgsql\Connection->addSavepoint() (Line: 149)
Drupal\pgsql\Driver\Database\pgsql\Schema->queryTableInformation('cache_flexible_permissions') (Line: 35)
Drupal\pgsql\Driver\Database\pgsql\Upsert->execute() (Line: 312)
Drupal\Core\Cache\DatabaseBackend->doSetMultiple(Array) (Line: 227)
Drupal\Core\Cache\DatabaseBackend->setMultiple(Array) (Line: 211)
Drupal\Core\Cache\DatabaseBackend->set('flexible_permissions:outsider:[user.roles]=authenticated', Object, -1, Array) (Line: 136)
Drupal\Core\Cache\VariationCache->set(Array, Object, Object, Object) (Line: 172)
Drupal\flexible_permissions\ChainPermissionCalculator->calculatePermissions(Object, 'outsider') (Line: 39)
Drupal\group\Access\GroupPermissionCalculator->calculateFullPermissions(Object) (Line: 40)
Drupal\group\QueryAccess\GroupQueryAlter->doAlter('view') (Line: 143)
Drupal\group\QueryAccess\QueryAlterBase->alter(Object, Object) (Line: 333)
group_query_entity_query_alter(Object, NULL, NULL) (Line: 552)
Drupal\Core\Extension\ModuleHandler->alter('query', Object) (Line: 494)
Drupal\Core\Database\Query\Select->preExecute() (Line: 519)
Drupal\Core\Database\Query\Select->execute() (Line: 157)
Drupal\pgsql\Driver\Database\pgsql\Select->execute() (Line: 272)
Drupal\Core\Entity\Query\Sql\Query->result() (Line: 85)
Drupal\Core\Entity\Query\Sql\Query->execute() (Line: 391)
Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection->validateReferenceableEntities(Array) (Line: 133)
Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator->validate(Object, Object) (Line: 202)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object, '0000000000000e150000000000000000', Array) (Line: 154)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object) (Line: 164)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object, Array, 1) (Line: 106)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object, NULL, NULL) (Line: 93)
Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object) (Line: 132)
Drupal\Core\TypedData\TypedData->validate() (Line: 518)
Drupal\Core\Entity\ContentEntityBase->validate() (Line: 25)
Drupal\group\Entity\GroupMembership->preSave(Object) (Line: 528)
Drupal\Core\Entity\EntityStorageBase->doPreSave(Object) (Line: 753)
Drupal\Core\Entity\ContentEntityStorageBase->doPreSave(Object) (Line: 483)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 806)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object) (Line: 147)
Drupal\group\Entity\Group->addRelationship(Object, 'group_membership', Array) (Line: 183)
Drupal\group\Entity\Group->addMember(Object) (Line: 214)
Drupal\CUSTOM->submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 129)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 67)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 597)
Drupal\Core\Form\FormBuilder->processForm('free_trial_checkout_form', Array, Object) (Line: 326)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 53)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 741)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Mostly :
Drupal\pgsql\Driver\Database\pgsql\Schema->queryTableInformation('cache_flexible_permissions') (Line: 35)

Tested and fixed issue in our project (similar versions mentioned above at #41) \o/
==> It probably still needs work, testing, but for now it fixes the issue in our project and allows us to move forward.

Any reviews and feedback would be greatly appreciated.
Thanks in advance!

@dydave - is your MR expected to also solve the error on
Drupal\Core\Database\TransactionNameNonUniqueException
mentioned in #36 and #43?

Thanks for the prompt feedback @vistree:

is your MR expected to also solve the error on
Drupal\Core\Database\TransactionNameNonUniqueException
mentioned in #36 and #43?

Really not sure about that, since we haven't come across this particular error.
Could you please try providing a backtrace, similar to the one above at #45?

Otherwise, perhaps more straight forward: Give the patch at:
https://git.drupalcode.org/project/drupal/-/merge_requests/10859.diff
a quick test and see if it fixes the issue in your project?

Thanks Matthew (@mradcliffe) for the help reviewing and cleaning up the MR.
Cheers!

I have pushed a change to fix the issue I was encountering.

Rather than having to explicitly check if we should wrap a query in a savepoint before calling "Connection::addSavepoint()", wouldn't it be better to do the check inside of "Connection::addSavepoint()" instead?

@dydave - I found a backtrace in Watchdog. Is this helpfull? I am still on Drupal 10.4 by the way!! Therefor patch https://git.drupalcode.org/project/drupal/-/merge_requests/10859.diff will not apply.
Error appears when trying to migrate Group User Relations with drush migrate:

Drupal\Core\Database\TransactionNameNonUniqueException: A transaction named mimic_implicit_commit is already in use. Active stack: 677fb68df0dbc1.32969210\drupal_transaction > 677fb68df23611.91989256\mimic_implicit_commit in Drupal\Core\Database\Transaction\TransactionManagerBase->push() (Zeile 254 in /data/html/docroot/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php).

#0 /data/html/docroot/core/modules/pgsql/src/Driver/Database/pgsql/Connection.php(558): Drupal\Core\Database\Transaction\TransactionManagerBase->push()
#1 /data/html/docroot/core/modules/pgsql/src/Driver/Database/pgsql/Connection.php(431): Drupal\pgsql\Driver\Database\pgsql\Connection->startTransaction()
#2 /data/html/docroot/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php(149): Drupal\pgsql\Driver\Database\pgsql\Connection->addSavepoint()
#3 /data/html/docroot/core/modules/pgsql/src/Driver/Database/pgsql/Upsert.php(35): Drupal\pgsql\Driver\Database\pgsql\Schema->queryTableInformation()
#4 /data/html/docroot/core/lib/Drupal/Core/Cache/DatabaseBackend.php(312): Drupal\pgsql\Driver\Database\pgsql\Upsert->execute()
#5 /data/html/docroot/core/lib/Drupal/Core/Cache/DatabaseBackend.php(227): Drupal\Core\Cache\DatabaseBackend->doSetMultiple()
#6 /data/html/docroot/core/lib/Drupal/Core/Cache/DatabaseBackend.php(215): Drupal\Core\Cache\DatabaseBackend->setMultiple()
#7 /data/html/docroot/core/lib/Drupal/Core/Cache/VariationCache.php(176): Drupal\Core\Cache\DatabaseBackend->set()
#8 /data/html/docroot/modules/contrib/flexible_permissions/src/ChainPermissionCalculator.php(172): Drupal\Core\Cache\VariationCache->set()
#9 /data/html/docroot/modules/contrib/group/src/Access/GroupPermissionCalculator.php(39): Drupal\flexible_permissions\ChainPermissionCalculator->calculatePermissions()
#10 /data/html/docroot/modules/contrib/group/src/QueryAccess/GroupQueryAlter.php(40): Drupal\group\Access\GroupPermissionCalculator->calculateFullPermissions()
#11 /data/html/docroot/modules/contrib/group/src/QueryAccess/QueryAlterBase.php(143): Drupal\group\QueryAccess\GroupQueryAlter->doAlter()
#12 /data/html/docroot/modules/contrib/group/group.module(333): Drupal\group\QueryAccess\QueryAlterBase->alter()
#13 /data/html/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(552): group_query_entity_query_alter()
#14 /data/html/docroot/core/lib/Drupal/Core/Database/Query/Select.php(494): Drupal\Core\Extension\ModuleHandler->alter()
#15 /data/html/docroot/core/lib/Drupal/Core/Database/Query/Select.php(519): Drupal\Core\Database\Query\Select->preExecute()
#16 /data/html/docroot/core/modules/pgsql/src/Driver/Database/pgsql/Select.php(157): Drupal\Core\Database\Query\Select->execute()
#17 /data/html/docroot/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(272): Drupal\pgsql\Driver\Database\pgsql\Select->execute()
#18 /data/html/docroot/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(85): Drupal\Core\Entity\Query\Sql\Query->result()
#19 /data/html/docroot/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php(401): Drupal\Core\Entity\Query\Sql\Query->execute()
#20 /data/html/docroot/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php(133): Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection->validateReferenceableEntities()
#21 /data/html/docroot/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(202): Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator->validate()
#22 /data/html/docroot/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(154): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints()
#23 /data/html/docroot/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(164): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode()
#24 /data/html/docroot/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(106): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode()
#25 /data/html/docroot/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php(93): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate()
#26 /data/html/docroot/core/lib/Drupal/Core/TypedData/TypedData.php(132): Drupal\Core\TypedData\Validation\RecursiveValidator->validate()
#27 /data/html/docroot/core/lib/Drupal/Core/Entity/ContentEntityBase.php(518): Drupal\Core\TypedData\TypedData->validate()
#28 /data/html/docroot/modules/contrib/group/src/Entity/GroupMembershipTrait.php(25): Drupal\Core\Entity\ContentEntityBase->validate()
#29 /data/html/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(528): Drupal\group\Entity\GroupMembership->preSave()
#30 /data/html/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(753): Drupal\Core\Entity\EntityStorageBase->doPreSave()
#31 /data/html/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(483): Drupal\Core\Entity\ContentEntityStorageBase->doPreSave()
#32 /data/html/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(806): Drupal\Core\Entity\EntityStorageBase->save()
#33 /data/html/docroot/core/lib/Drupal/Core/Entity/EntityBase.php(354): Drupal\Core\Entity\Sql\SqlContentEntityStorage->save()
#34 /data/html/docroot/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(237): Drupal\Core\Entity\EntityBase->save()
#35 /data/html/docroot/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(175): Drupal\migrate\Plugin\migrate\destination\EntityContentBase->save()
#36 /data/html/docroot/core/modules/migrate/src/MigrateExecutable.php(248): Drupal\migrate\Plugin\migrate\destination\EntityContentBase->import()
#37 /data/html/vendor/drush/drush/includes/drush.inc(62): Drupal\migrate\MigrateExecutable->import()
#38 /data/html/vendor/drush/drush/includes/drush.inc(53): drush_call_user_func_array()
#39 /data/html/docroot/modules/contrib/migrate_tools/src/Drush/Commands/MigrateToolsCommands.php(1074): drush_op()
#40 /data/html/docroot/modules/contrib/migrate_tools/src/Drush/Commands/MigrateToolsCommands.php(483): Drupal\migrate_tools\Drush\Commands\MigrateToolsCommands->executeMigration()
#41 [internal function]: Drupal\migrate_tools\Drush\Commands\MigrateToolsCommands->import()
#42 /data/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(276): call_user_func_array()
#43 /data/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback()
#44 /data/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter()
#45 /data/html/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(391): Consolidation\AnnotatedCommand\CommandProcessor->process()
#46 /data/html/vendor/symfony/console/Command/Command.php(326): Consolidation\AnnotatedCommand\AnnotatedCommand->execute()
#47 /data/html/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run()
#48 /data/html/vendor/symfony/console/Application.php(324): Symfony\Component\Console\Application->doRunCommand()
#49 /data/html/vendor/symfony/console/Application.php(175): Symfony\Component\Console\Application->doRun()
#50 /data/html/vendor/drush/drush/src/Runtime/Runtime.php(110): Symfony\Component\Console\Application->run()
#51 /data/html/vendor/drush/drush/src/Runtime/Runtime.php(40): Drush\Runtime\Runtime->doRun()
#52 /data/html/vendor/drush/drush/drush.php(139): Drush\Runtime\Runtime->run()
#53 /data/html/vendor/drush/drush/drush(4): require('/data/html/docr...')
#54 /data/html/vendor/bin/drush(119): include('/data/html/docr...')
#55 {main}

Nice one @vistree!

I am still on Drupal 10.4 by the way!! Therefor patch https://git.drupalcode.org/project/drupal/-/merge_requests/10859.diff will not apply.

Just did the test with D10.4.1 and the patch applies very well.

Additionally, looking at your stack trace, see:
#3 /data/html/docroot/core/modules/pgsql/src/Driver/Database/pgsql/Upsert.php(35): Drupal\pgsql\Driver\Database\pgsql\Schema->queryTableInformation()

Same as pointed above at #45.

Therefore: The patch should apply and fix the issue for your project! \o/

Could you please do a quick round of tests and report back?

Thanks in advance!

Thanks Elim (@elimw), re #48:

Rather than having to explicitly check if we should wrap a query in a savepoint before calling "Connection::addSavepoint()", wouldn't it be better to do the check inside of "Connection::addSavepoint()" instead?

Sounds good! I'm not super familiar with the overall code of the pgsql module, but if you do a quick search around and see where the functions are used and if there would be any unexpected impacts, it would be great!

Could you perhaps create a new merge request with a different patch?
We would be able to give the patch a round of tests and see if it could be equivalent to the current MR!10859.

Thanks in advance!

@dydave -After upgrading to current Drupal core 10.4.1 patch from MR (https://git.drupalcode.org/project/drupal/-/merge_requests/10859.diff) applied and - what great news - solved the error on running my migration ;-)

So, I can confirm that the MR solves the error regarding "Drupal\Core\Database\TransactionNameNonUniqueException" on group relationship migrations.

Thanks a lot @vistree for the feedback! Glad it's working for you 🥳

Hopefully we'll get more reviews, feedback and perhaps a better version of the patch (see #51/refactoring), but for now, hopefully, this version will allow all of us to keep moving forward on our projects!
Cheers!

@dydave, @vistree, I've created separate MR which checks if a savepoint with the same name already exists before pushing a transaction.

@elimw: is your MR a replacement or an addition for MR 10859 ?

Thanks a lot Elim (@elimw)! Great job!

We've just given a round of tests with the merge request you created MR!10896 and it fixed the issue as well for us 🥳
We've updated the patch in our project to use the one from #54.

@vistree:
Yes indeed, the patch created by Elim can be tested here:
https://git.drupalcode.org/project/drupal/-/merge_requests/10896.diff

It should fix the issue in your project as well.

It's a different (better) way of writing the same thing as the previous patch.

Thanks again for the work Elim and in advance for your feedback @vistree!

The merge request 10896 (#45) seems to be 1770 commits behind the target branch and needs a rebase. It's currently failing tests.

The issue is a little hard to follow and I think next if someone could address the Needs issue summary update tag that would be helpful to clarify proposed resolutions and remaining tasks.

https://git.drupalcode.org/project/drupal/-/merge_requests/10859.diff solved this issue for me after installing entity_reference_integrity on Drupal 10.3.10.

https://git.drupalcode.org/project/drupal/-/merge_requests/10896.diff also worked!

@mradcliffe: Both merge requests have been rebased.
Since Elim's MR!10896 seems to fix the issue for us and is a much simpler version we would recommend going with this MR (Tests are passing 🟢).

Issue summary still needs to be updated.

Thanks!

I can confirm that also the MR https://git.drupalcode.org/project/drupal/-/merge_requests/10896.diff solves the error regarding "Drupal\Core\Database\TransactionNameNonUniqueException" on group relationship migrations.

Was previously tagged for IS update which still appears to be needed.

Also there are 2 MRs up so one needs to be closed or hidden.

Have not reviewed but will need test coverage is missing (whichever MR is picked)

As per #62:
We're going with MR!10896 from #54 for now, so the other MR was hidden.

Still remaining to do if anyone has time to work on this issue: Write tests.

Create an automated test case allowing to reproduce the issue.
This would allow to better describe and summarize the IS, in particular with steps to reproduce or a piece of code that currently results in the Exception.

Elim (@elimw) perhaps if you find some time, it would be great if you could look into this and see if you could put together a small piece of code that could reproduce the exception (could be executed in devel_php, for example).
We could use it in an automated Test class added to the MR and then also document it in the IS.

Any help, advice or feedback would be greatly appreciated.
Thanks in advance!

@dydave, I've added a PHPUnit test for the change and all tests are passing. I hope this is what you were after?

Sorry it's taken me so long to get around to doing this.

Super nice @elimw! Great job! 🥳

Looks like everything requested above is now in the MR, with the added Unit Test 👌

Let's see if reviewers and maintainers could take another look at this issue.
Thanks again for the great help and work @elimw!

Can someone please update the issue summary to include an explanation of what the proposed change is to the logic, and why it is necessary? There are plenty of comments in this thread that say "the patch works!" but very little about what the broader implications of the change might be. Reading the code diff doesn't provide any of this, and it raises further questions.

Changing if ($this->inTransaction()) { to if ($this->inTransaction() && !$this->transactionManager()->has($savepoint_name)) means that the addSavepoint() logic will NOT run in certain scenarios anymore. Is this OK? Has thought been given to how this might behave in other contexts? If so, I don't see any of that documented here (unless I missed it?) and if this causes issues later, future developers will have a hard time looking back at this issue and understanding why the decision was made.

I'm not critiquing the solution itself - it might be the right answer! But I'd like to see more of an explanation so that it's easier to understand at a glance.

Setting back to "Needs work" for the issue summary update, per @smustgrave's comment #62 above.

public function addSavepoint($savepoint_name = 'mimic_implicit_commit')

Very naively (having just jumped into this issue for the first time myself), could we leverage the $savepoint_name method argument as a solution instead?

It sounds like the issue is primarily a conflict in the mimic_implicit_commit name itself. Presumably changing $savepoint_name = 'mimic_implicit_commit' to $savepoint_name = 'some_other_name' would also solve this problem? (I'm not suggesting that... just posing the thought.)

I don't know all of the code that calls addSavepoint(), but would it be possible to override that in the downstream modules that are affected by this issue, such that the default argument can be changed to avoid the conflict?

Just throwing out alternative ideas here to stimulate the conversation a bit, while I wrap my head around this issue myself. Apologies if these have already been explored. :-)

I have rebased this and did some work to get phpstan to pass.

Would it work to just use a different name for the save point? Instead of always using mimic_implicit_commit, append a unique number, mimic_implicit_commit_1234.

I like it. I am trying to remember if there was a reason not to do that back in 2015 or so. Whether the caller knew the transaction name or not.

Tests are passing.

It could be so that if the caller gives a name, it uses it. But if they do not, instead of always using mimic_implicit_commit, it generates a unique name. Instead of mimic_implicit_commit being the default for the $savepoint_name param, use null as the default. If it's null, generate the unique name.

Adding related issues.

I have tested the merge request. It fixes the problem for me.

This patch is the current state of the merge request.

See #69 for notes about the issue summary update needed.

Added a couple of minor comments in a code review.

Ran the manual 'Test-only' test. The output:

https://git.drupalcode.org/issue/drupal-2920527/-/jobs/9293971

LGTM. Seems the test coverage is failing as it should.

I think @liam morland is right in #3583849-5: Deprecate PgSql Connection::*Savepoint() methods, and that issue is practically making this one outdated. #3583849: Deprecate PgSql Connection::*Savepoint() methods is solving this problem in a more generic way i.e. removing the savepoints array that is pgsql-specific, and managing the savepoints via the base TransactionManager. The TransactionManager already fails if a duplicate savepoint name is detected.

#3583849: Deprecate PgSql Connection::*Savepoint() methods has been resolved but the fix may just move the mimic_implicit_commit problem to Connection::query().

#81 I do not think so, this code

    // We need to wrap queries with a savepoint if:
    // - Currently in a transaction.
    // - A 'mimic_implicit_commit' does not exist already.
    // - The query is not a savepoint query.
    if (
      $this->inTransaction() &&
      !$this->transactionManager()->has('mimic_implicit_commit') &&
      !(is_string($query) && (
        stripos($query, 'ROLLBACK TO SAVEPOINT ') === 0 ||
        stripos($query, 'RELEASE SAVEPOINT ') === 0 ||
        stripos($query, 'SAVEPOINT ') === 0
      ))
    ) {
      // Create a savepoint so we can rollback a failed query. This is so we can
      // mimic MySQL and SQLite transactions which don't fail if a single query
      // fails. This is important for tables that are created on demand. For
      // example, \Drupal\Core\Cache\DatabaseBackend.
      $savepoint = $this->startTransaction('mimic_implicit_commit');
      try {
        $return = parent::query($query, $args, $options);
        $savepoint->commitOrRelease();
      }
      catch (\Exception $e) {
        $savepoint->rollback();
        throw $e;
      }
    }
    else {
      $return = parent::query($query, $args, $options);
    }

already prevents to start a second savepoint named mimic_implicit_commit if one exists, I think.

If not, a test would help to show where the problem is, then.

Yes, I think you are right.

Looks like it could be closed in favour of #3495728: [meta] Drop implicit autocommit on Transaction destruction

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.

Rerolled.

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.

::addSavepoint() has been deprecated but code to start transactions called mimic_implicit_commit has been added in a bunch of places. I am concerned that this may still lead to nested transactions with the same name.

Fastpatch from 10.6.x branch

Liam, you are right that mimic_implicit_commit has been added in other places. The TAC Lite module was triggering it for us today. I just made https://www.drupal.org/project/tac_lite/issues/3618911 for that problem, and posted a patch that works for it.

Added MR !17015 against main, adapting the savepoint-existence guard from !10859 to the current Select::execute() transaction API.

Reproduced on Drupal 11.4.6, PostgreSQL 16, PHP 8.4.22. An outer transaction plus a SELECT whose query-alter hook executes another SELECT throws TransactionNameNonUniqueException: mimic_implicit_commit is already in use. The new core-only regression reproduces this without contributed modules.

With the guard: Select suite 44 tests / 111 assertions; PostgreSQL transaction suite 21 tests / 271 assertions (2 existing skips). No failures. Dependency deprecations are reported. The regression also checks repeated nested queries, failed-query recovery, and rollback of the outer transaction.

The original moderated-node draft continuation scenario, with node grants and an existing node-reference field, changes from HTTP 500 to passing 30 functional assertions. No access, validation, or revision checks were removed. Main CI still needs verification; the local results above are for 11.4.6.

Ran test-only test for MR!17015. https://git.drupalcode.org/issue/drupal-2920527/-/jobs/12007998. It fails on the new test code. Good sign.

Thanks, oily, for running the test-only job. That confirms the new regression test fails without the fix.

With the fix applied, the PostgreSQL 18 / PHP 8.5 pipeline passed all automatic jobs: https://git.drupalcode.org/issue/drupal-2920527/-/pipelines/949568

The regression also checks repeated nested queries, recovery after a failed query, and rollback of the outer transaction. The original moderated-node draft creation and continuation scenario now passes with the patch on Drupal 11.4.6 / PostgreSQL 16 / PHP 8.4. No access, validation, or revision checks were removed.

After reading #95 setting to RTBTC.

This is tagged for an issue summary update and that should be done before setting to RTBC.

Thanks, quietone. Updated the issue summary with the current reproduction, the savepoint guard in MR !17015, regression coverage, and the test-only and passing CI results. Replaced the stale implementation task with final committer review and commit, and removed the Needs issue summary update tag.

Maybe it would be better to remove all added savepoints. See: https://www.drupal.org/project/drupal/issues/3615690

Re: #101 daffie it would seem that this issue should be postponed pending https://www.drupal.org/project/drupal/issues/3615690 or closed as a duplicate. Re: #100 for example, we aren't going to need any savepoint guards when https://www.drupal.org/project/drupal/issues/3615690 lands.

daffie, Just wondering if there are any downsides to postgresql without savepoints or any reason to believe that we might end up having to use rely on them even if less than before? I noticed https://www.drupal.org/project/drupal/issues/3615690 is currently hitting test failures due to removal of savepoints (it looks like). Does that just mean fixing the tests to remove the savepoint-related code?

I’ve contributed a rollback regression and draft correction to #3615690. Its proposed table-existence cache can retain an entry after rolling back table creation, causing subsequent operations to abort the transaction.

On #102: PostgreSQL still needs savepoints around failures that callers intend to catch and recover from inside a transaction. Removing blanket savepoints changes the recovery responsibilities of those callers; the tests need to verify that behavior.

#3615690 is marked major-version-only. Our reproduction here affects Drupal 11.4.6, so supported-branch handling remains relevant alongside the main-branch redesign. The smaller guard in !17015 remains available for that discussion.

With Drupal 11.4.6 and Group 3.3.5, even after applying patch !17015, I am still encountering an error when adding a user to a group using the addMember() function

@phpsubbarao Try https://git.drupalcode.org/project/drupal/-/merge_requests/10896.diff? This is the patch that solved my problem.

@#107 Merge request 10896 is changing a deprecated method, so I doubt it is a long-lasting solution.

Merge request 17015 fixes the issue for us. I have rebased this merge request.

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 conflict is in merge request 10896, which is closed.

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.

To prevent the bot from changing the status, I fixed the conflict in merge request 10896. I did not update the test.