I get some test failures if the db driver is set to avoid transaction management. Not sure if these are purely related to test code, or if there are real bugs.

CommentFileSizeAuthor
#12 2874499-12-transaction-enabled.patch6.06 KBmondrake
#11 2874499-11-transaction-enabled.patch6.07 KBmondrake
#7 2874499-7-transaction-disabled.patch8.28 KBmondrake
#7 2874499-7-transaction-enabled.patch5.91 KBmondrake
#6 2874499-6-transaction-disabled.patch8.24 KBmondrake
#6 2874499-6-transaction-enabled.patch5.87 KBmondrake
#6 interdiff_4-6.txt2.25 KBmondrake
#4 2874499-4-transactions-disabled.patch5.98 KBmondrake
#4 2874499-4-transactions-enabled.patch3.62 KBmondrake
#2 2874499-2-test-only.patch2.36 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
StatusFileSize
new 2874499-2-test-only.patch2.36 KB

Test only patch to demonstrate the issue. It just forces all the core drivers to not support transactions.

Status: Needs review » Needs work

The last submitted patch, 2: 2874499-2-test-only.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 2874499-4-transactions-enabled.patch3.62 KB
new 2874499-4-transactions-disabled.patch5.98 KB

At least the TransactionTest failures seem related to test code not checking if transaction support is enabled.

The patches here:

  • transaction-enabled has a fix to the test code only, leaving transaction support enabled as per default.
  • transaction-disabled is built on top of the test-only in #2, with transaction support disabled.

Status: Needs review » Needs work

The last submitted patch, 4: 2874499-4-transactions-disabled.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_4-6.txt2.25 KB
new 2874499-6-transaction-enabled.patch5.87 KB
new 2874499-6-transaction-disabled.patch8.24 KB

I cannot find anywhere code that would add an 'Explicit rollback failed' message to the watchdog, which is checked in the BlockContentCreationTest and NodeCreationTest. Is that just dead code that by setting the driver not to support transactions lives back again?

The PostgresSql failure seems a different problem.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2874499-7-transaction-enabled.patch5.91 KB
new 2874499-7-transaction-disabled.patch8.28 KB

Reroll.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2874499-11-transaction-enabled.patch6.07 KB

Reroll

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2874499-12-transaction-enabled.patch6.06 KB

The last submitted patch, 11: 2874499-11-transaction-enabled.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +Contrib database driver
daffie’s picture

daffie commented
Status: Needs review » Needs work

I have just one remark. The rest of the patch is RTBC for me.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php
@@ -218,10 +218,6 @@ public function testFailedBlockCreation() {
-      // Check that the failed rollback was logged.
-      $records = db_query("SELECT wid FROM {watchdog} WHERE message LIKE 'Explicit rollback failed%'")->fetchAll();
-      $this->assertTrue(count($records) > 0, 'Transactions not supported, and rollback error logged to watchdog.');
     }

+++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
@@ -112,10 +112,6 @@ public function testFailedPageCreation() {
-      // Check that the failed rollback was logged.
-      $records = static::getWatchdogIdsForFailedExplicitRollback();
-      $this->assertTrue(count($records) > 0, 'Transactions not supported, and rollback error logged to watchdog.');

Is it possible that the rollback failure is logged under some other name then what the test is now locking for?

daffie’s picture

daffie commented
daffie’s picture

daffie commented

@mondrake: Do you know if we have a database driver without transaction support in core? If not is it possible to add testing for that?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +Needs reroll

The patch is rather outdated ATM, would certainly need a reroll to start with.

However, more in general:
a) I think all core drivers are set to support transactions, currently (SQLite coming last to the club) so: core de-facto requires transactional db operations
b) in 2020, even in contribland, will there be db platforms NOT supporting transactions?

So - wouldn't it be better to deprecate/remove support for non-transactional db operations instead?

mondrake’s picture

mondrake
Location 🇮🇹
commented

#19 my bad, it seems the last one to join the transactional club in Drupal was MySql when MyISAM was dropped: https://www.drupal.org/node/2278745. Not SQLite then.

Interestingly, from the CR:

[...] Drupal 8 relies on transaction support [...]

and also #2275535: [policy, no patch] Drop support for non-transactional database engines (including MyISAM), #2278971: Deprecate Connection::supportsTransactions().

Apparently, we never got to the bottom of eradicating non-transactional support...

daffie’s picture

daffie commented

So - wouldn't it be better to deprecate/remove support for non-transactional db operations instead?

I am working on creating a database driver for MongoDB and they do transactions. It is implemented differently then SQL databases. So I would like to have the option of disabling the SQL type transactions.

mondrake’s picture

mondrake
Location 🇮🇹
commented

@daffie you probably want to bypass SQL backends to store data, but in this issue we're only talking about SQL databases and their support (or not) of transactional integrity, i.e. when multiple database operations need to be managed as a single commit to the db or rolled back if any of the suboperation fails. Just to say the two things seem independent from each other, to me.

mondrake’s picture

mondrake
Location 🇮🇹
commented

I have revived #2278971: Deprecate Connection::supportsTransactions(), adding a patch there, and am inclined to close this one as a won't fix, but leaving open ATM for any comment.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Closed (won't fix)

#2275535: [policy, no patch] Drop support for non-transactional database engines (including MyISAM) set the policy for must have transaction support, and #2278971: Deprecate Connection::supportsTransactions() is going to enforce that, so this issue becomes redundant.