Deprecate Drupal\Core\Database\Connection::nextId() and the {sequences} table and schema

Fixed failing test (NextIdTest)

Removed white space before a closing "}"

+++ b/core/includes/form.inc
@@ -899,7 +899,9 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
 
     // Assign an arbitrary id: don't rely on a serial column in the 'batch'
     // table, since non-progressive batches skip database storage completely.
-    $batch['id'] = \Drupal::database()->nextId();
+    $key_value_store = \Drupal::service('keyvalue')->get('batch');
+    $batch['id'] = $key_value_store->get('batch_id', 0) + 1;
...
 

I think it's probably fine to start the batch ID from 0 again rather than having an update to copy over from sequences, but just noting in case we think it would be worth it? Everything looks good here otherwise.

We'll need an explicit Drupal 11 (or even Drupal 12?) follow-up to add an update to drop the table.

+++ b/core/includes/form.inc
@@ -899,7 +899,9 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
+    $key_value_store = \Drupal::service('keyvalue')->get('batch');
+    $batch['id'] = $key_value_store->get('batch_id', 0) + 1;
+    $key_value_store->set('batch_id', $batch['id']);

The only concern I have here is that if by chance this happens in the context of a transaction, and the transaction gets rolled back, the counter would be rolled back as well? But I think it's the same with the current API anyway. In the past I saw doctrine/dbal supporting a sequence proxy - but they were opening a separate connection just for the purpose of not meddling with transactional operations.

Added the requested comment by @catch.

Alternatively, why do non-progressive batches even need an ID, if it is never stored? If we can remove that then we can make bid a serial column and then we never need sequences in core at all.

Changed patch to use the lock service to prevent batches from getting the same ID when multiple batch processes try to get a new batch ID by locking the batch process.

+++ b/core/includes/form.inc
@@ -899,7 +899,15 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
+    if ($lock->acquire('batch')) {

Seems unlikely, but we need to do something if we fail to acquire the lock, as we won't have an ID to use.

@chx's concerns about race conditions here still apply, as far as I can see.

The concern has been addressed.
Back to RTBC.

+++ b/core/includes/form.inc
@@ -899,7 +899,15 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
+    // processes try to get a new batch ID.
+    $lock = \Drupal::lock();
+    if ($lock->acquire('batch')) {
+      $batch['id'] = $key_value_store->get('batch_id', 0) + 1;
+      $key_value_store->set('batch_id', $batch['id']);
+      $lock->release('batch');
+    }
 

This won't work - it would mean no batch ID is assigned at all if another process has a lock, which is as or more likely than the original race condition.

As the solution with the lock service does not work, lets try the solution from @mondrake. See: https://www.drupal.org/project/drupal/issues/3257824#comment-14883465 📌 Ignore, testing issue Closed: outdated .

Here's an updated version of the patch referenced in #111.

Unfortunately there is a problem with the update path here - the update itseltf is run in a batch, so using the new code with the old table before it is changed by the upate function is failing.

It's not pretty, but for similar problems, we've hard-coded the database queries in the code that actually runs updates so that it's guaranteed to run before any updates do at all. Obviously this will run every time until that code is removed again in Drupal 11, so needs to be re-entrant and ideally do a minimal check so it can return early.

Confused about why this is not failing on SQLite too.

Let's see this. I think, however, we need a separate issue to do this one, and postpone this one on that.

Tried understanding why #112 was not failing on SQLite, and it looks like when an int is defined as primary key, it is automatically incremented when no value is passed for the PK in INSERT.

The {batch} table is created like

    CREATE TABLE "test35615194"."testmk2" (\n
    "bid" INTEGER NOT NULL CHECK ("bid">= 0), \n
    "token" VARCHAR(64) NOT NULL, \n
    "timestamp" INTEGER NOT NULL, \n
    "batch" BLOB NULL DEFAULT NULL, \n
     PRIMARY KEY ("bid")\n
    )

Then inserting

  'sql' => "INSERT INTO "test35615194"."testmk2" ("timestamp", "token", "batch") VALUES (?, ?, ?)"
  'args' => array:3 [
    0 => 1674582649
    1 => ""
    2 => null
  ]

Yields for SELECT * FROM "test35615194"."testmk2"

  0 => {#3557
    +"bid": "1"
    +"token": ""
    +"timestamp": "1674582649"
    +"batch": null
  }

You never stop learning...

Anyway, for the purpose here this is just fine.

This issue is postponed on https://www.drupal.org/project/drupal/issues/3337513 📌 Fix batch process race conditions by making ‘bid (batch id)’ auto-increment Fixed .

Changed status to 'postponed' (didn't do it before)

📌 Fix batch process race conditions by making ‘bid (batch id)’ auto-increment Fixed is in.

Re-roll for #107 and few changes

The test core/modules/mysql/tests/src/Kernel/mysql/NextIdTest.php is running only on mysql, so probably needs other base test to use

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -739,6 +739,9 @@ protected function installSchema($module, $tables) {
+        @trigger_error('Installing the table sequences with the method KernelTestBase::installSchema() is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3143286', E_USER_DEPRECATED);

Tiny nitpick: We are going to remove the sequences table in Drupal 12 not Drupal 11.

Filed new CR (restored from revision → ) https://www.drupal.org/node/3349345 → and said for removal in 12.0 and updated all deprecations

Updated IS

and the last place

@andypost: The plan is to remove the method Connection::nextId() in Drupal 11 and to remove the table "sequences" in Drupal 12. We wait with dropping the sequences table because we cannot deprecate a database table. That is the plan for now. Better ideas are welcome!

The last submitted patch, 125: 2665216-124.patch, failed testing. View results →

I see no reason to hack into db layer to try throw when table is used, so need to revert 12 to 11 and keep in KernelTestBase

Here's fix for #128

btw drush needs fix for batch too https://github.com/drush-ops/drush/blob/2b890ab5c2b5d70cae7b17ebda63ad57...

📌 Fix batch process race conditions by making ‘bid (batch id)’ auto-increment Fixed got reverted, this needs to wait for that to be fixed

Blocker is in.

re-roll for 10.2

The testbot is failing.

New WorkspaceAssociationTest usage fixed

For PostgreSQL, shouldn't public function nextIdDelete() in its Connection class be deprecated as well?

I bet you meaning Mysql driver because there's only usage of this function could be found, which was added in #3129043: Move core database drivers to modules of their own →

Added deprecation for #138

#139 - yes, and I need more coffee. Thanks!

All code changes look good to me.
All deprecations have testing.
All use of the deprecated methods have been removed.
I have updated the IS and the CR for the deprecation of Drupal\mysql\Driver\Database\mysql\nextIdDelete().
For me it is RTBC.

The last submitted patch, 139: 2665216-139.patch, failed testing. View results →

Needs a reroll. And, BTW, I think MySql Connection::$needsCleanup property should be deprecated too.

Test core/modules/file/tests/src/Kernel/Views/RelationshipUserFileDataTest.php is fixed via 🐛 Adding File Usage "File" relationship results in broken/missing handler Fixed

Added deprecation for #145 as the property protected, I don't think we need magic getter/setter to deprecate it

Back to RTBC.

Read back through this issue and saw all the comments relating to the batch and users table that we eventually split out to other issues with much better implementations, nice to finally get to the point where all this issue is doing is adding deprecations!!

Committed 6204ba9 and pushed to 11.x. Thanks!

Thank you! I renamed follow-up from D12 to D11 - 📌 Drop sequences table in Drupal 12 Active

CR looks ready to be published but I think we should remove the table in D12

Automatically closed - issue fixed for 2 weeks with no activity.