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

Re-rolling patch for 8.1.x-dev

Rerolled #11.
Simple rebase fixed it.

Tagging this issue.

Re-roll.

We will work on it at DCM2017

Rerolled the patch.

reroll

Clean-up all new usage and tune a bit ID generation

Also removed useless method and renamed ensureSequuencesTableExists()

reroll & fix few new ones

it should fix most of failed tests

Fix one more test

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -351,7 +351,15 @@ public function nextId($existing = 0) {
-    $id = $this->query("SELECT nextval('" . $sequence_name . "')")->fetchField();
+    try {
+      $id = $this->query("SELECT nextval('" . $sequence_name . "')")->fetchField();
+    }
+    catch (\Exception $e) {
+      if (!$this->ensureSequencesTableExists()) {
+        throw $e;
+      }
+      $id = 0;
+    }

Quick point: I do not think that PostgreSQL uses the sequences table.

@daffie sadly all databases doing it, and onlyto generate user id

sadly all databases doing it, and onlyto generate user id

That maybe the case, only the PostgreSQL driver does not use the sequences table to store the sequence.

@daffie as I see PG using sequence from this table instead of the table(

#2664322: key_value table is only used by a core service but it depends on system install has landed.

re-roll but it needs work for pgsql driver, just wanna make sure what bot will tell

This patch should fix the testbot failures for MySQL.

This should fix the testbot failures for SQLite.

@daffie Thank you a lot!

btw I'm thinking about implementation... most of databases does not need table to create sequence, the only usage of this table is user entity and batch, see #3028709: Change behavior of PostgreSQL to not share sequence for batch and users

Moreover this API (\Drupal\Core\Database\Connection::nextId() via sequences table) is more like workaround for limitations of each supported database so naming is still a question for me

- getSequencesSchema() should it remain public? as we can't define sequence with schema
- ensureSequencesTableExists() should it remain mention "table"? as other databases can create just sequences for batch and users

Probably both needs better docblocks

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -263,7 +263,15 @@ public function nextId($existing = 0) {
+    try {
+      $id = $this->query("SELECT nextval('" . $sequence_name . "')")->fetchField();
+    }
+    catch (\Exception $e) {
+      if (!$this->ensureSequencesTableExists()) {
+        throw $e;

So only this thing left to decide, pgsql needs only named sequence but creates a table

@andypost: Thank you for the review!

Changes:
1. Changed the method getSequencesSchema() from public to protected.
2. Updated the docblock of the method ensureSequencesTableExists().
3. Removed the create table part from the PostgreSQL driver.

Fixing the single coding standard violation.

Used to check for nextId() usage in contrib

There's only 7 usages, mostly database drivers (mongodb, oracle, sqlserv)

Maybe decouple this api into new service and deprecate the nextId() method in connection?

It could use key_value a sequences collection instead of database specific implementation

Added CR and updated the Is for the fact that calling KernelTestBase::installSchema() for the table sequences is deprecated.
Rerolled the patch and added the deprecation.

+++ b/core/lib/Drupal/Core/Database/Connection.php
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -170,6 +170,11 @@ abstract class Connection {

@@ -170,6 +170,11 @@ abstract class Connection {
    */
   protected $escapedNames = [];
 
+  /**
+   * The sequences table name.
+   */
+  const SEQUENCES_TABLE_NAME = 'sequences';
+
   /**
    * List of escaped table names, keyed by unescaped names.
    *
@@ -2083,4 +2088,48 @@ public function getPagerManager(): PagerManagerInterface {

@@ -2083,4 +2088,48 @@ public function getPagerManager(): PagerManagerInterface {
     return \Drupal::service('pager.manager');
   }
 
+  /**
+   * Defines the schema for the sequences table.
+   *
+   * @return array
+   *   Schema definition array.
+   */
+  protected static function sequencesSchemaDefinition() {
+    return [
+      'description' => 'Stores IDs.',
+      'fields' => [
+        'value' => [
+          'description' => 'The value of the sequence.',
+          'type' => 'serial',
+          'unsigned' => TRUE,
+          'not null' => TRUE,
+        ],
+      ],
+      'primary key' => ['value'],
+    ];
+  }
+
+  /**
+   * Check if the table sequences exists and create it if not.
+   *
+   * @return bool
+   *   TRUE when table sequences exists, FALSE otherwise.
+   */
+  protected function ensureSequencesTableExists() {
+    try {
+      $schema = $this->schema();
+      if (!$schema->tableExists(static::SEQUENCES_TABLE_NAME)) {
+        $schema->createTable(static::SEQUENCES_TABLE_NAME, $this->sequencesSchemaDefinition());
+        return TRUE;
+      }
+    }
+    // If another process has already created the table, attempting to
+    // recreate it will throw an exception. In this case just catch the
+    // exception and do nothing.
+    catch (SchemaObjectExistsException $e) {
+      return TRUE;
+    }
+    return FALSE;
+  }
+
 }

How about not adding this to the base Connection class, but just to the MySql and SQLite implementations? Assuming that each RDBMS should have its own way to manage sequences. Yes, it's a bit boilerplate for MySql and SQLite, but would be cleaner in general.

EDIT - BTW then you won't even need a schema, just a 'CREATE TABLE' statement that can be driver specific at that point.

@mondrake Thank you for your review!

How about not adding this to the base Connection class, but just to the MySql and SQLite implementations? Assuming that each RDBMS should have its own way to manage sequences. Yes, it's a bit boilerplate for MySql and SQLite, but would be cleaner in general.

I can move the code into a new trait called "sequencesTrait.php" and add to trait to the connection classes of MySQL and SQLite. I that a good solution?

BTW then you won't even need a schema, just a 'CREATE TABLE' statement that can be driver specific at that point.

I know I can do that and I did not because in similar situations in lib/Drupal/Core have used the same split as in this patch. This solution is also used by Batch, Cache, Config, Flood, KeyValue, Lock, Menu, Queue and Routing.

I am only a bit worried about a BC break. This happens when you use a contrib database driver to create a new site. The problem that I do not see how this can be fixed in core. Maybe it would be better to fix this in the contrib database drivers. What do you think?

I see no reason in trait and the table at all, it used to store only 1 row which could be stored in key value table - no extra API needed for that

#64 I'm not sure. That would mean replacing a DB feature (using sequences where supported by the RDBMS, like in PGSql and Oracle, or emulating them, like in MySql and SQLite) with a service which is much more overhead. IIUC this issue's focus is on cleaning up the DB API layer.

Separate discussion IMHO is if we need Connection::nextId() at all in core: it seems to be used only in UserStorage and form.inc, and mentioned in KeyValueEntityStorage but not actually used. If we can find alternatives there, we might deprecate nextId altogether, but I'd say this is for a separate issue.

Maybe it is better to look if we can deprecate the Connection::nextId() functionality.

We have 2 instances in core that use the functionality:

1.     // 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();
     

   I think we can replaced it here by just generating a random integer.

2.     $entity->uid->value = $this->database->nextId($this->database->query('SELECT MAX([uid]) FROM {' . $this->getBaseTable() . '}')->fetchField());
     

   I do not understand we just do not use a serial field for the user id. It is an entity like all other entities. Or is it for user id zero is the anonymous user. Is that the problem? If so, is it still valid?
   Can we solve it in another way, something specific in the user module and deprecate it in the database system? Let the user module store the value in the keyvalue table.

It would be great to remove this from the database system.

@andypost and @mondrake: Thank you for your help!

BTW, this

$entity->uid->value = $this->database->nextId($this->database->query('SELECT MAX([uid]) FROM {' . $this->getBaseTable() . '}')->fetchField());

is rather bad because if anything else than UserStorage is using the sequence generated by nextId() and expecting it to be incrementing, this statement may (ok, in edge situations) reset the sequence to a lower value if the max(id) of the user base table is lower than the current value of the sequence.

So... now I am +1 for deprecating instead.

@mondrake If you like the patch then I will update the title, summary and the CR. I have deprecated the methodConnection::nextId() and the table sequences. The table can be removed in D10. The usage of the method have been replaced in form.inc and userstorage.php.

IMHO #68 makes sense; we need an IS update to reflect the new direction of the issue.

1. +++ b/core/includes/form.inc
   @@ -898,7 +898,7 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
   -    $batch['id'] = \Drupal::database()->nextId();
   +    $batch['id'] = mt_rand(1, 2147483647);
   

   Maybe overkilling here: would it make sense to use Lock::acquire() to lock the ID while in use by the batch?

2. +++ b/core/modules/system/system.install
   @@ -1326,6 +1326,8 @@ function system_install() {
   +  // @deprecated The sequences table has been deprecated in drupal:9.3.0 and is
   +  // removed from drupal:10.0.0. See https://www.drupal.org/node/3220378.
   

   Not sure using @deprecated is correct here. Maybe just open a followup for the removal in D10 and use @todo to refer to it. BTW I think in D10 we should also have an update function that will actually remove the {sequences} table in live sites.

3. +++ b/core/modules/user/src/UserStorage.php
   @@ -21,8 +21,14 @@ protected function doSaveFieldItems(ContentEntityInterface $entity, array $names
   +      $key_value = \Drupal::service('keyvalue');
   

   I think this can be injected, by overriding the ::createInstance and ::__construct methods.

4. +++ b/core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php
   @@ -194,6 +192,8 @@ public function testScriptLoad() {
   +    // Special handling of sequences table which is created on demand.
   +    $schema->dropTable('sequences');
   

   Is this still needed?

5. +++ b/core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php
   @@ -46,6 +62,8 @@ public function testDbNextId() {
   +    $this->expectDeprecation('Connection::nextId() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Modules should use instead the keyvalue storage for the last used id. See https://www.drupal.org/node/3220378');
   +
        // Only run this test for the 'mysql' driver.
        $driver = $this->connection->driver();
   

   expectDeprecation should be moved to after the block checking if the test should be skipped.

Updated the title, the IS and the CR.

@mondrake: Thank you for your review.

For 69.1: I have changed the solution to use the keyvalue store to store the batch ID. The Lock::acquire() look to me as a bit of a overkill and such a lock does expire. Where a keyvalue store one does not.

For #69.2: I am not sure what the correct way is to deprecate a table. I do not think we have one. My idea was that the release manager would do a code base search for "@deprecated" before he/she would release D10.0.0 and therefor the comment would be found and fixed. I have less confidence that a @todo will be fixed in the D10.0.0 release. Technically I think that a @todo is better then a @deprecated. Maybe I should ask a release manager about this. ;-)

For #69.3: Done!

For #69.4: Removed!

For #69.5: Done!

Assuming this is green, it's RTBC for me now.

I was wondering whether we should also deprecate Connection::makeSequenceName(). It's only used internally by PgSql so maybe it does no longer need to be public API. But that could be a followup.

For the official BC policy (https://www.drupal.org/about/core/policies/core-change-policies/drupal-8...):

While the database query API and schema API itself are stable, the schema of any particular table is not. Writing queries directly against core tables is not recommended and may result in code that breaks between minor versions.

Adjusting title, we are not deprecating KernelTestBase::installSchema() altogether.

Edited CR and filed #3221101: Deprecate Drupal\Core\Database\Connection::makeSequenceName(), make it internal to PostgreSQL .

1. +++ b/core/modules/user/src/UserStorage.php
   @@ -14,6 +24,57 @@
   +    $this->keyValueStore = $key_value_store;
   

   We need to make this optional and trigger a deprecation if it is not passed in. Just in case.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php
   @@ -23,13 +24,28 @@ class NextIdTest extends DatabaseTestBase {
   -    $this->installSchema('system', 'sequences');
   +
   +    $table_specification = [
   +      'description' => 'Stores IDs.',
   +      'fields' => [
   +        'value' => [
   +          'description' => 'The value of the sequence.',
   +          'type' => 'serial',
   +          'unsigned' => TRUE,
   +          'not null' => TRUE,
   +        ],
   +       ],
   +      'primary key' => ['value'],
   +    ];
   +    $this->connection->schema()->createTable('sequences', $table_specification);
   

   This change is not necessary - it is a legacy test so triggering a deprecation won't cause the test to fail and then we'll keep test coverage of system_schema()

3. I think we need to deprecate \Drupal\Core\Database\Driver\mysql\Connection::nextIdDelete() too. It now should never be called during regular runtime - which means in D10 \Drupal\Core\Database\Driver\mysql\Connection::__destruct() can be removed.
4. One big concern I have is database locking while multiple processes create users. The current implementation of nextId() is highly performant for getting the next ID - but the clean-up can cause issues.
5. We need an update path to migrate the current value to state - if this is where we should store it.

@alexpott Looking at #1650930: Use READ COMMITTED by default for MySQL transactions I'm not sure 4) is "highly performant"

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -309,7 +309,11 @@ public function mapConditionOperator($operator) {
     $new_id = $this->query('INSERT INTO {sequences} () VALUES ()', [], ['return' => Database::RETURN_INSERT_ID]);
     // This should only happen after an import or similar event.
     if ($existing_id >= $new_id) {

It makes 3 queries instead of using auto-increment

https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/lib/Drupal/C...

@andypost the three queries don't happen in usually... normally it is just $new_id = $this->query('INSERT INTO {sequences} () VALUES ()', [], ['return' => Database::RETURN_INSERT_ID]); and an insert is definitely quicker than what we're doing with state... we're doing a select and update on every user creation. And the cleanup of the sequences table does not occur on user time but at shutdown.

Reading the recent comments got me thinking about trying to use NO_AUTO_VALUE_ON_ZERO and autoincrement for users.

@alexpott found #838992: Change the uid field from integer to serial by leveraging NO_AUTO_VALUE_ON_ZERO on MySQL which I'll move to Drupal 9 now.

If that's possible, I think we'd need:

1. An issue to change the setting
2. An issue specifically for user storage to switch to autoincrement away from sequences
3. This issue minus the user bits.

#80 sounds like a great plan - also looking at the Batch stuff I;'m not sure what that uses this either - the queue table is a serial and maybe the memory stuff could create it's own ID... I think we should delegate ID creation to the batch backend and not use key value.

Used to dig history a bit

Looks first attempt by @chx is #49836: Getting rid of the sequences table, using db_last_insert_id() instead of db_next_id() and #55516: Remove database locking

Then #356074: Provide a sequences API

For #80 there's #2886441: Sequences API (mysql) kills MySQL server performance due to cleanup phase

The current implementation of the sequences API, while it has it's own problems, is atomic.

The new patch, on the other hand is race prone.

1. Process #1 reads the K-V value, it's X
2. Process #2 reads the K-V value, it's X
3. Process #1 increments X (in PHP)
4. Process #2 increments X (in PHP)
5. Process #1 saves the database record
6. Process #2 also saves the database record with the same ID. Hopefully, a UNIQUE index is set which causes it to die instead of corrupting the database. If it handles the database error then it can retry, however there is no guarantee it won't be stuck in a retry loop for example if two drush commands are running in parallel which inserts user records.

Eliminating the uses of this in core is valuable and adding a note discouraging users from it is also great. Moving the cleanup to cron to increase performance would be a separate fix. But I would not remove this useful API as there is no replacement for it.

The method Drupal\Core\Database\Connection::nextId() is programmed that it only works when it is used for a single sequence. When a contrib or custom module starts using the method and sets the sequence ID for getting a new ID, the chance is very high that sooner then later that the database will throw an error that insert a record with an existing ID.

While it certainly returns only one sequence of integers it never reuses them so there's no way this can happen. If a module starts using this API where existing IDs exist the API provides for this. I do not understand this.

Also when getting a new bach ID, the existing sequence Id is not changed.

I do not understand this either. Every nextId changes the sequence value. That's what nextId does. It is the only thing it does.

So I think this issue should revert to #61 and also some other fixes as mentioned already would be valuable:

1. #838992: Change the uid field from integer to serial by leveraging NO_AUTO_VALUE_ON_ZERO on MySQL
2. PP on that, remove the use of nextId in userstorage
3. Remove the use of nextId in batch separately from this issue
4. Move cleanup to cron to avoid a stampede in #2886441: Sequences API (mysql) kills MySQL server performance due to cleanup phase
5. Document the usage of nextId is discouraged and the return value of insert execute is preferred.

I have rerolled the patch for 10.1.x

fixing coding standards

Style guide fix (previous comments where not added because of spam protection)

Needs codestyle fixes and deprecation version

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1653,6 +1653,11 @@ public function commit() {
+   * @deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Modules

+++ 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:9.3.0 and is removed from drupal:10.0.0. The table is now lazy loaded and therefore will be installed automatically when used. See https://www.drupal.org/node/3143286', E_USER_DEPRECATED);

it should be 10.1.0 and removed from 11.0.0

Code style fixes and deprecation version changes

missing part of previous patch added to current patch

+++ 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. The table is now lazy loaded and therefore will be installed automatically when used. See https://www.drupal.org/node/3143286', E_USER_DEPRECATED);

This text is not right. The part "The table is now lazy loaded and therefore will be installed automatically when used." can be removed.

Addressed #98.
Please review.

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.

+++ 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']);

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

If modules that previously used sequences are expected to do this themselves, should we add an explicit API that can safely increment a value and return it? Other key value stores such as Memcache and Redis offer an atomic increment operation, for example. This could be part of the keyvalue API or a separate service.

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.

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 #3337513: Fix batch process race conditions by making ‘bid (batch id)’ auto-increment.

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

#3337513: Fix batch process race conditions by making ‘bid (batch id)’ auto-increment 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!

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...

#3337513: Fix batch process race conditions by making ‘bid (batch id)’ auto-increment 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 MySql, 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.

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 #2628230: Adding File Usage "File" relationship results in broken/missing handler

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 - #3335756: Drop sequences table in Drupal 12

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