New serial columns do not own sequences

I'm working on #826552: Altered serial columns break sequences and it seems that this needs to be fixed first :-)

Bit of clean-up after re-roll

1) module handler should not be used in loop from \Drupal
2) .module file is optional
3) variables should be snake_case

+++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
@@ -0,0 +1,47 @@
+      __DIR__ . '/../../../fixtures/update/drupal-8.6.0.bare.testing.php.gz',

Looks it still exists after https://www.drupal.org/node/3098322

Here I have tried to address comment #21.

The patch looks good.

1. +++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
   @@ -84,7 +84,10 @@ public function exists($name) {
   +      $raw = $this->connection->query('SELECT data FROM {' . $this->connection->escapeTable($this->table) . '} WHERE collection = :collection AND name = :name ORDER BY collection, name', [
   

   Why are we adding the ORDER BY? If it is not really needed, then please do not add it. It does not make Drupal faster.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -455,6 +455,45 @@ public function upsert($table, array $options = []) {
   +   * Retrieves a sequence owned by a table and column.
   

   Maybe better: "Retrieves a sequence name that is owned by the table and column."

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -455,6 +455,45 @@ public function upsert($table, array $options = []) {
   +      ->query("SELECT c.relname FROM pg_class as c INNER JOIN pg_namespace as ns ON (c.relnamespace = ns.oid) WHERE c.relkind = 'S' AND c.relname = :name", $args)
   

   Why are we adding the join to this query? It is not being used.

4. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -455,6 +455,45 @@ public function upsert($table, array $options = []) {
   +   * Checks if a sequence exists at all.
   

   Maybe better: "Checks if a sequence exists"

5. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -1053,6 +1053,24 @@ protected function _createKeys($table, $new_keys) {
   +  public function changeSequenceOwnership($table, $column) {
   

   I think that updateSequenceOwnership is better. Change implies that you can do different things. This method does only one thing.

6. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -1053,6 +1053,24 @@ protected function _createKeys($table, $new_keys) {
   +   * This is used for updating orphaned sequences.
   

   Can we add some documentation about why this method exists and add a link to this issue/change record.

7. +++ b/core/modules/system/system.install
   @@ -2521,3 +2523,122 @@ function system_update_8805() {
   +      $sandbox['max'] = count($sandbox['tables']);
   +      $sandbox['progress'] = 0;
   

   Why are these 2 lines inside the foreach loop?

8. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -1053,6 +1053,24 @@ protected function _createKeys($table, $new_keys) {
   +    $this->connection->query('ALTER SEQUENCE ' . $seq . ' OWNED BY ' . $table_name . '.' . $column);
   

   What happens when there is no sequence by that name in the database?

9. Do we really need to update entity tables with the id/revision key being an integer. Is that not why we have #2928906: The field schema incorrectly stores serial fields as int.

10. +++ b/core/modules/system/system.install
    @@ -2521,3 +2523,122 @@ function system_update_8805() {
    +    // Ensures that a sequence is not owned first, then ensures that the a
    +    // sequence exists at all before trying to alter it.
    

    I would rather that we do that in the method changeSequenceOwnership.

I'm working on it.

@ravi.shankar I am not able to apply patch.

Hi prabha1997,

It Looks like needs a re-roll.

Re-rolled the patch.

The conflict was with the two functions:
function system_update_8901 and function system_update_8806

I have placed system_update_8806 function code next to system_update_8901 function code and merge went well.

Need to work on the suggestions by daffie.

+++ b/core/modules/system/system.install
@@ -12,12 +12,16 @@
+<<<<<<< HEAD
 use Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema;
+=======
+>>>>>>> Applying patch from issue 3028706 comment c3cdfab79ec

Looks like merge conflict is not resolved fully.

Patch re-rolled to address the same.

Thanks to all that worked on rerolling the patch for this issue.
Putting the issue bad to "needs work". My remarks from comment #23 still need to be addressed.

The attached patch is a reroll for Drupal version 9.1

It addresses comments 1-7 in #23, with 8-10 TBD

Attached please find patch 35.
Patch 33 appears to have been "backwards" — my apologies.

In regard to 23.9 and 23.10 —
I having trouble getting my head around (colloquial English, means 'understanding') the need to scan the tables in the database
on system.install

if a custom entity didn't have a working sequence, won't inserts already be failing?

Version 37 changes the core version requirement for the test module.

Also, I misinterpreted how you were doing the loop in the update (in system.install line 1538) so I changed it back to what you had originally.

Updated the patch in #37 to fix custom command failure errors, thanks!

+++ b/core/modules/system/system.install
@@ -1509,3 +1510,124 @@ function _system_advisories_requirements(array &$requirements): void {
+            $transaction = $connection->startTransaction($sequence_name);
+            try {
+              $connection
+                ->schema()
+                ->updateSequenceOwnership($table_info['table'], $table_info['column']);
+
+              $sandbox['fixed']++;
+            }
+            catch (DatabaseExceptionWrapper $e) {
+              $transaction->rollBack();
+            }

How come this is in a transaction? There's only a single statement in updateSequenceOwnership(). Also this code will eat database exceptions - should we log when this fails or ignore it?

Patch looks good!

1. +++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
   @@ -0,0 +1,47 @@
   +      $sequence_exists = (bool) $database
   +        ->query("SELECT pg_get_serial_sequence('{" . $database->prefixTables('pgsql_sequence_test') . "}', 'sequence_field')")
   +        ->fetchField();
   +      $this->assertTrue($sequence_exists, 'Sequence is owned by the table and column.');
   
   +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -161,6 +164,13 @@ public function testSchema() {
   +      $sequence_exists = (bool) $this->connection
   +        ->query("SELECT pg_get_serial_sequence('{test_table}', 'test_serial')")
   +        ->fetchField();
   +      $this->assertTrue($sequence_exists, 'New sequence is owned by its table.');
   

   Using the function pg_get_serial_sequence() does return the sequence name that is used for the field. However it does not say anything about who owns the sequence. The query you need for that is:

   +      $seq_name = $connection->makeSequenceName('users', 'uid');
   +      $seq_owner = $connection->query("SELECT d.refobjid::regclass as table_name, a.attname as field_name
   +        FROM pg_depend d
   +        JOIN pg_attribute a ON a.attrelid = d.refobjid AND a.attnum = d.refobjsubid
   +        WHERE d.objid = :seq_name::regclass
   +        AND d.refobjsubid > 0
   +        AND d.classid = 'pg_class'::regclass", [':seq_name' => 'public.' . $seq_name])->fetchObject();
   +      $this->assertEquals($connection->tablePrefix('users') . 'users', $seq_owner->table_name);
   +      $this->assertEquals('uid', $seq_owner->field_name);
   

   The query is from the patch from #838992: Change the uid field from integer to serial by leveraging NO_AUTO_VALUE_ON_ZERO on MySQL. Maybe it is best to create a new method in the class Schema.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -368,6 +368,45 @@ public function rollbackSavepoint($savepoint_name = 'mimic_implicit_commit') {
   +  public function getSequence($table, $column) {
   

   Maybe rename the method to getSequenceName().

3. +++ b/core/modules/system/system.install
   @@ -1509,3 +1510,124 @@ function _system_advisories_requirements(array &$requirements): void {
   +function system_update_9101(&$sandbox) {
   

   This has missed the window for 9.1. Therefor change the name to system_update_9301()

4. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -368,6 +368,45 @@ public function rollbackSavepoint($savepoint_name = 'mimic_implicit_commit') {
   +  public function getSequence($table, $column) {
   ...
   +  public function sequenceExists($name) {
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -1029,6 +1029,25 @@ protected function _createKeys($table, $new_keys) {
   +  public function updateSequenceOwnership($table, $column) {
   

   Can we add type hinting to the new methods. Something like public function getSequence(string $table, string $column): ?string {, public function sequenceExists(string $name): bool { and public function updateSequenceOwnership(string $table, string $column) {.

5. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -368,6 +368,45 @@ public function rollbackSavepoint($savepoint_name = 'mimic_implicit_commit') {
   +  public function getSequence($table, $column) {
   ...
   +  public function sequenceExists($name) {
   

   Can we move these 2 methods to the class Schema. To me that is where they belong.

Hello to everyone.
I can confirm that #41 works as designed for Postgres 12.
Thanks,
Marco

Rerolled the patch for 9.4.x-dev
Addressed Issues from #44

Thank you @mradcliffe for the quick review.
Updated the patch with the review from #48

Updated patch in #49 to make few corrections in core/modules/pgsql/src/Driver/Database/pgsql/Schema.php file. Attached an interdiff.

Thanks!

1. +++ b/core/modules/system/system.install
   @@ -1651,3 +1652,125 @@ function _system_check_array_table_prefixes() {
   +        module_load_include('install', $module);
   

   The function module_load_include() has been deprecated. Please replace it with \Drupal::moduleHandler()->loadInclude().

2. +++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
   @@ -0,0 +1,51 @@
   +      __DIR__ . '/../../../fixtures/update/drupal-8.8.0.bare.standard.php.gz',
   

   Let replace this with 9.0.0 version. Drupal 8 is already EOL.

3. +++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
   @@ -0,0 +1,51 @@
   +      __DIR__ . '/../../../fixtures/update/drupal-8.pgsql-orphan-sequence.php',
   

   Can we rename this from "drupal-8" to "drupal-9" and move it to the pgsql module.

4. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
   @@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
   +  public function updateSequenceOwnership(string $table, string $column) {
   

   Can we add the return type hint ": void".

5. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
   @@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
   +   * See issue https://www.drupal.org/project/drupal/issues/3028706
   

   Can we change "See issue https://www.drupal.org/project/drupal/issues/3028706" to "@see https://www.drupal.org/i/3028706".

6. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
   @@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
   +      ->query("SELECT c.relname FROM pg_class as c WHERE c.relkind = 'S' AND c.relname = :name", $args)
   

   Can we replace the variable $args with [':name' => $name].

7. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
   @@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
   +    $seq_owner = $this->connection->query("SELECT d.refobjid::regclass as table_name, a.attname as field_name
   

   Can we replace $seq_owner = with return. We do not need the variable $seq_owner. Also the last empty line in this method can be removed.

8. +++ b/core/modules/system/tests/fixtures/update/drupal-8.pgsql-orphan-sequence.php
   @@ -0,0 +1,45 @@
   +if ($db_type === 'pgsql') {
   

   As this fixture is only used by pgsql, is it not necessary to add the if statement.

9. +++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
   @@ -0,0 +1,51 @@
   +    if ($db_type === 'pgsql') {
   ...
   +      $this->assertTrue(TRUE, 'Database update ran successfully on non-pgsql driver.');
   

   We need to move this to the start of the method and change it to:

   $connection = Database::getConnection();
   if ($connection->driver() !== 'pgsql') {
     $this->markTestSkipped('This test only works with the pgsql driver');
   }
   

10. +++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
    @@ -0,0 +1,51 @@
    +        $this->assertEquals($connection->tablePrefix('users') . 'users', $seq_owner->table_name);
    +        $this->assertEquals('uid', $seq_owner->field_name, 'Sequence is owned by the table and column.');
    

    We need to a list with tables names and column name to test if they owner their sequence.

11. +++ b/core/modules/system/tests/modules/pgsql_test/pgsql_test.info.yml
    @@ -0,0 +1,7 @@
    +core: 8.x
    +core_version_requirement: ^8 || ^9
    

    These 2 lines can be removed.

12. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
    @@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
    +   * @return array|null
    +   *   Returns the sequence owner object or NULL if it does not exist..
    

    What is it? Are we returning an array or an object? Can we also describe which keys are being returned.

13. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
    @@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
    +      ':table' => $this->connection->prefixTables('{' . $table . '}'),
    

    Can we change this to:

          ':table' => $this->connection->tablePrefix() . $table,
    

14. As the database driver have been moved to their own modules See: https://www.drupal.org/node/3129492. And because this an only PostgreSQL fix, we can move all files to the pgsql module.

1. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
   @@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
   +   * @internal
   +   */
   +  public function updateSequenceOwnership(string $table, string $column) {
   ...
   +  public function getSequenceName(string $table, string $column): ?string {
   ...
   +  public function sequenceExists(string $name): bool {
   ...
   +  public function getSequenceOwner(): ?array {
   

   Are we sure that we should be adding all this public API to the Postgres schema class. We're only adding this for the update code. I think this could just as easily live in _ methods in the .install file.

2. +++ b/core/modules/system/system.install
   @@ -1651,3 +1652,125 @@ function _system_check_array_table_prefixes() {
   +/**
   + * Fix any orphan sequences created from column changes in PostgreSQL.
   + */
   +function system_update_9401(&$sandbox) {
   

   This can also move to the postgres module now...

3. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -161,6 +164,14 @@ public function testSchema() {
   +      $seq_owner = $this->schema->getSequenceOwner();
   +      if ($seq_owner) {
   +        $this->assertEquals($this->connection->tablePrefix('users') . 'users', $seq_owner->table_name);
   +        $this->assertEquals('uid', $seq_owner->field_name, 'New sequence is owned by its table.');
   +      }
   

   Shouldn't this test fail in $seq_owner is NULL?

4. I think that there is a good case here that the updates should be in a follow-up issue to the API fix.

Fixed following points of comment #52:
1, 2, 4, 5, 6, 7, 8, 11, 13.

Still needs work for other points.

Removed a random addition introduced by #55 (inside plugin.es6.js and plugin.js)

Addressed some more points from #52 and fixed some typos.
1. corrected wrong parameter being passed to \Drupal::moduleHandler()->loadInclude()
3. done
9. done
12. partially addressed (I don't know how to check which keys are being returned to indicate there.)

Now there only missing the points 10, 12, and 14 from #52, they are:

10. We need to a list with tables names and column name to test if they owner their sequence.

+++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
@@ -0,0 +1,51 @@
+        $this->assertEquals($connection->tablePrefix('users') . 'users', $seq_owner->table_name);
+        $this->assertEquals('uid', $seq_owner->field_name, 'Sequence is owned by the table and column.');

12. Can we describe which keys are being returned.

+++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
@@ -1077,6 +1077,82 @@ public function extensionExists($name): bool {
+   * @return object|null
+   *   Returns the sequence owner object or NULL if it does not exist.

14. As the database driver have been moved to their own modules See: https://www.drupal.org/node/3129492. And because this an only PostgreSQL fix, we can move all files to the pgsql module.

The points from #54 still need to be addressed too.

I'm sorry, the patch wasn't uploaded.

@mradcliffe Sorry I may have misunderstood what @daffie meant when he said on #52.3

+++ b/core/modules/system/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
@@ -0,0 +1,51 @@
+      __DIR__ . '/../../../fixtures/update/drupal-8.pgsql-orphan-sequence.php',

Can we rename this from "drupal-8" to "drupal-9" and move it to the pgsql module.

Should I have just renamed it and keept where it was at patch #50?

Updated path in pgsql.module and renamed file drupal-8.pgsql-orphan-sequence.php to drupal-9.pgsql-orphan-sequence.php still not sure on what to do about #58.

@mradcliffe, thanks for clarifying.

I moved PostgreSqlSequenceUpdateTest.php and drupal-9.pgsql-orphan-sequence.php to the pgsql module,
added back the line __DIR__ . '/../../../fixtures/update/drupal-9.0.0.bare.standard.php.gz', to UpdatePathTestBase::setDatabaseDumpFiles and updated it according to the new file location,
removed the driver checks and markTestSkipped checks.

Still need to address #56

I have rerolled the patch for 10.1.

This is blocking #3337513: Fix batch process race conditions by making ‘bid (batch id)’ auto-increment, and is a broken API - to me should qualify as Major.

Reviewed, for me it's RTBC, but I was thinking it would be good to have a test to demonstrate

The pgsql driver does not set the ownership of sequences it creates when changeField method is called. This happens if you specify a table without a serial field, then later change an integer field into one.

... then realized that DriverSpecificSchemaTestBase::testChangePrimaryKeyToSerial() is actually meant to be doing that, and should be running for every driver. So now I do not understand why that is not failing for pgsql.

EDIT: it does not seem that this is blocking #3337513: Fix batch process race conditions by making ‘bid (batch id)’ auto-increment

I agree with @mondrake in comment #67: that test is necessary.

For me just 1 other remark:

+++ b/core/modules/pgsql/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
@@ -0,0 +1,65 @@
+    if ($seq_owner) {

This if-statement can be removed. It should always be true.

Adjusted tags to reflect #67 edited comment

Address #68 and next but calling hooks inside of update hook is better to rewrite with listing all sequences.

Moreover except hook_schema() other services can provide schema (using ensureSchema() out of head) so leaving "Needs work" for upgrade path rework

+++ b/core/modules/pgsql/pgsql.install
@@ -40,3 +42,124 @@ function pgsql_requirements() {
+    $sandbox['max'] = count($sandbox['tables']);
+    $sandbox['progress'] = 0;
+
+  }
+  else {
+    // Adds ownership of orphan sequences to tables.
...
+  if ($sandbox['max'] && $sandbox['progress'] < $sandbox['max']) {
+    $sandbox['#finished'] = $sandbox['progress'] / $sandbox['max'];
+  }
+  else {
+    $sandbox['#finished'] = 1;

this else looks useless, sandbox init is executed only once and then for-loop can always iterate

1. +++ b/core/modules/pgsql/pgsql.install
   @@ -40,3 +42,124 @@ function pgsql_requirements() {
   +    $modules = $module_handler->getModuleList();
   ...
   +      if ($module_handler->moduleExists($module)) {
   +        $module_handler->loadInclude($module, 'install');
   +        $schema = $module_handler->invoke($module, 'schema');
   

   This code should not appear in update hooks, modules could have pending updates for their schema (which possibly invalid ATM)

2. +++ b/core/modules/pgsql/pgsql.install
   @@ -40,3 +42,124 @@ function pgsql_requirements() {
   +    $entity_types = \Drupal::entityTypeManager()->getDefinitions();
   ...
   +    foreach ($entity_types as $entity_type) {
   +      $storage_class = $entity_type->getStorageClass();
   +      if (is_subclass_of($storage_class, SqlContentEntityStorage::class)) {
   

   pending update hooks also can change storage

+++ b/core/modules/pgsql/pgsql.install
@@ -111,44 +112,38 @@ function pgsql_update_10100(&$sandbox) {
+          $transaction = $connection->startTransaction($sequence_name);
+          try {
+            $schema
+              ->updateSequenceOwnership($table_info['table'], $table_info['column']);

as sandbox using batch size 50 - any reason for transaction?

as sandbox using batch size 50 - any reason for transaction?

Just being on the save side. AFAIK it does no harm.

After some consultation with @daffie I've created a new patch, which fixes the custom commands and also some changes regarding the update function name.

All code changes look good to me.
Testing has been added.
I have updated the IS and I have added a CR.
For me it is RTBC.

1. +++ b/core/modules/pgsql/pgsql.install
   @@ -40,3 +42,122 @@ function pgsql_requirements() {
   +        $module_handler->loadInclude($module, 'install');
   +        $schema = $module_handler->invoke($module, 'schema');
   ...
   +    // Discovers all content entity types with integer entity keys that are most
   +    // likely serial columns.
   +    $entity_types = \Drupal::entityTypeManager()->getDefinitions();
   +    /** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */
   +    foreach ($entity_types as $entity_type) {
   

   That this will miss tables that are not part of hook_schema or an entity table. In core we have cache tables and batch storage (for example)... and there's an issue to convert batch storage to serial.

   Is there another way to do this? Like can we get all the tables - find all the possibly serial colums and check for sequences that do not have the correct ownership and then fix them. We might just have to look for sequences based on integer column names as postgres does not record the fact it is a sequence via the column type.

2. +++ b/core/phpstan-baseline.neon
   @@ -1960,6 +1960,11 @@ parameters:
   +		-
   +			message: "#^A file could not be loaded from Drupal\\\\Core\\\\Extension\\\\ModuleHandlerInterface\\:\\:loadInclude$#"
   +			count: 1
   +			path: modules/pgsql/pgsql.install
   +
   

   We need an upstream issue in the drupal phpstan project to fix this. The error is incorrect. I opened https://github.com/mglaman/phpstan-drupal/issues/516

3. #54.1 was not addressed - we're adding a load of Postgres specific API to it's schema class for use in a single update. I'm not sure that we should be doing that.

Is there another way to do this? Like can we get all the tables

If we do this than we can also change non Drupal tables. I am not sure that is something we should do.

#77 is a good point. Hmmm.... that's really really tricky. So this is a best effort situation. So let's leave the code as is but add a comment explaining the reasoning in the update function.

We also should address #54.1 / #76.1 - I'm not convinced the new API to upport the update function is worth it. We could inline the queries and be done.

btw Listing of tables and sequences looks more predictable (there's table prefix and if other tables has it they are doomed) vs calling hooks from update hook, I recall only sqlite issue with listing #2949229: SQLite findTables Returns Empty Array on External DB.

But if no prefix used then core can't list all schema service consumers for tables

I've addressed #54.1, but I've moved the code to .module instead of .install, as it wouldn't work for me when I placed it in the .install file as requested.

All code changes look good to me.

I'm not convinced the new API to upport the update function is worth it. We could inline the queries and be done.

The functionality has moved to functions in the module file and their names start with an underscore, therefor they are @internal. No addion to the public API. The code has been moved there, because to code is used in multiple instances and for better readability.

I have updated the IS.
There is testing being added.
For me it is RTBC.

For the committer: the CR must not be published.

Let's go with a slightly different approach here. Let's add a final @internal class Drupal\pgsql\Update10101 that's instantiated from the hook_update_N like this: \Drupal::classResolver(Update10101::class)->update($sandbox - see \Drupal\Core\Config\Entity\ConfigEntityUpdater for something similar. That way this class can encapsulate all the functionality and be instantiated in the test so it can access the same functionality. Adding functions like function _updateSequenceOwnership(string $table, string $column): void { don't really work. This function should be called _pgsql_update_sequence_ownership() for example. But also all these functions end up in the global function namespace and will be harder to remove once we no longer need to maintain the update.

I've created a patch based on your request @alexpott, please check if this is what you meant.

I forgot to add a comment to the update function in the Update10101 class.

All points of @alexpott have been addressed.
Back to RTBC.

1. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,235 @@
   +    return new static($container->get('entity_type.manager'));
   ...
   +    $connection = \Drupal::database();
   ...
   +    $connection = \Drupal::database();
   

   We have a mix of dependency injection and the global singleton.

   We should inject the DB I think

2. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,235 @@
   +  public function update(array &$sandbox) {
   

   We can add a return type hint here ?TranslatableMarkup

3. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,235 @@
   +                if (substr($column_info['type'], 0, 6) === 'serial') {
   

   We can use str_starts_with now

4. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,235 @@
   +      // Discovers all content entity types with integer entity keys that are most
   

   ubernit: >80

5. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,235 @@
   +          $owned = (bool) $this->pgsql_get_sequence_name($table_info['table'], $table_info['column']);
   ...
   +            $exists = $this->pgsql_sequence_exists($sequence_name);
   ...
   +                $this->pgsql_update_sequence_ownership($table_info['table'], $table_info['column']);
   

   We can use camel case for these method names now, e.g. $this->getSequenceName() (no need for the pgsql prefix either)

6. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,235 @@
   +      return \Drupal::translation()->formatPlural(
   

   we can use new \Drupal\Core\StringTranslation\PluralTranslatableMarkup instead of using the singleton here

7. +++ b/core/phpstan-baseline.neon
   @@ -1970,6 +1970,11 @@ parameters:
   +		-
   +			message: "#^A file could not be loaded from Drupal\\\\Core\\\\Extension\\\\ModuleHandlerInterface\\:\\:loadInclude$#"
   +			count: 1
   +			path: modules/pgsql/src/Update10101.php
   +
   

   Why was this needed?

8. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,235 @@
   +          $base_field_definitions = $entity_class::baseFieldDefinitions($entity_type);
   

   This doesn't take into account hook_entity_base_field_info_alter

   And I don't think we can rely on the entity-type manager during an update hook.

   I think we need to use \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface::getLastInstalledFieldStorageDefinitions instead

@larowlan Thank you for your input, I will look onto these suggestions soon and will post a new patch. Also regarding #88.7, Alexpott made a issue for this problem in phpstan stated in comment #76.2, the issue can be found through the following link: Github issue

If I'm correct I've added all requested changes from @larowlan except for #88.7 as explained in my previous comment.

I've changed the return type for the update function to ?PluralTranslatableMarkup instead of PluralTranslatableMarkup|NULL.

+++ b/core/modules/pgsql/src/Update10101.php
@@ -0,0 +1,262 @@
+      $modules = $module_handler->getModuleList();
+      foreach ($modules as $extension) {
+        $module = $extension->getName();
+        if ($module_handler->moduleExists($module)) {

there's extension.list.module service to get list of enabled modules, so moduleExists() could be removed

hi as @andypost extension.list.module is not yet stable, in D10, is it good to use it here?
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

@rckstr_rohan this service already gets used a lot in core so it is not that bad to use it in my opinion.

So as requested by @andypost, here is a updated patch with the service. I also altered some code so we get the services using the container instead of calling them in the function. Changes can be seen in the interdiff.

My bad forgot to comment the new parameters in the __construct function and had to regenerate the baseline, because that change is not necessary anymore.

Based the interdiff on patch #91 instead of #95.

The remarks of @andypost (comment #93) has been addressed.
Back to RTBC.

+++ b/core/modules/pgsql/src/Update10101.php
@@ -0,0 +1,283 @@
+
+      // Discovers all custom tables with serial columns.
+      $modules = $this->moduleExtensionList->getList();
+      foreach ($modules as $extension) {
+        $module = $extension->getName();
+        $this->moduleHandler->loadInclude($module, 'install');
+        $schema = $this->moduleHandler->invoke($module, 'schema');
+        if (!empty($schema)) {
+          foreach ($schema as $table_name => $table_info) {
+            foreach ($table_info['fields'] as $column_name => $column_info) {
+              if (str_starts_with($column_info['type'], 'serial')) {
+                $sandbox['tables'][] = [
+                  'table' => $table_name,
+                  'column' => $column_name,
+                ];
+              }
+            }
+          }
+        }
+      }
+
+      // Discovers all content entity types with integer entity keys that are
+      // most likely serial columns.
+      $entity_types = $this->entityTypeManager->getDefinitions();
+      /** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */
+      foreach ($entity_types as $entity_type) {
+        $storage_class = $entity_type->getStorageClass();

This is covering hook_schema() and entities but does it also need to run for services with lazy table creation? I'm not sure how if it does...

Discussed this in slack with @daffie.

The consequences of not updating a table are not that bad, so if we fix this for all new sites + entity and hook_schema() tables on new sites that will solve things for a lot of people.

We can then open a new issue to figure things out for on-demand tables.

+++ b/core/modules/pgsql/src/Update10101.php
@@ -0,0 +1,283 @@
+
+      // Discovers all custom tables with serial columns.
+      $modules = $this->moduleExtensionList->getList();

Let's change this to 'Discovers all tables defined with hook_schema()' and add a @todo pointing to the follow-up issue.

Created the requested followup (#3358777: New serial columns do not own sequences for on demand tables).

Here is the updated comment and added the todo.

All remarks from @catch have been addressed.
Back to RTBC.

1. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,285 @@
   +  /**
   +   * The entity type manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   +   */
   +  protected $entityTypeManager;
   +
   +  /**
   +   * The last installed schema repository.
   +   *
   +   * @var \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface
   +   */
   +  protected $entityLastInstalledSchemaRepository;
   +
   +  /**
   +   * The Drupal database connection object.
   +   *
   +   * @var \Drupal\Core\Database\Connection
   +   */
   +  protected $connection;
   +
   +  /**
   +   * The module extension list.
   +   *
   +   * @var \Drupal\Core\Extension\ModuleExtensionList
   +   */
   +  protected $moduleExtensionList;
   +
   +  /**
   +   * The module handler service.
   +   *
   +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   +   */
   +  protected $moduleHandler;
   +
   +  /**
   +   * Sequence owner update constructor.
   +   *
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   +   *   The entity type manager.
   +   * @param \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository
   +   *   The last installed schema repository service.
   +   * @param \Drupal\Core\Database\Connection $connection
   +   *   The database connection.
   +   * @param \Drupal\Core\Extension\ModuleExtensionList $module_extension_list
   +   *   The module extension list.
   +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   +   *   The module handler service.
   +   */
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository, Connection $connection, ModuleExtensionList $module_extension_list, ModuleHandlerInterface $module_handler) {
   +    $this->entityTypeManager = $entity_type_manager;
   +    $this->entityLastInstalledSchemaRepository = $entity_last_installed_schema_repository;
   +    $this->connection = $connection;
   +    $this->moduleExtensionList = $module_extension_list;
   +    $this->moduleHandler = $module_handler;
   +  }
   

   I realise this code has been raked over ad-nauseam, so apologies in advance.

   As this is 10.1+ only now, let's use constructor property promotion and do away with a fair bit of boiler plate here

2. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,285 @@
   +        ['@count' => $sandbox['fixed']]
   

   nit: we don't need this array, @count is already set by the first argument in the constructor for PluralTranslatableMarkup

3. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,285 @@
   +    $seq = $this->connection->makeSequenceName($table, $column);
   

   Any reason not to pass this in as a parameter given we already calculated it on line 181?

4. +++ b/core/modules/pgsql/src/Update10101.php
   @@ -0,0 +1,285 @@
   +  public function getSequenceOwner(string $table, string $field): object|bool {
   

   This seems to be only used in the test, any reason we can't move this method to the test?

   Will likely need to also change some cspell ignore entries on both this class and the test

Keen to get this in, so please ping me when this is RTBC again.

I think i've added all code requested in #104 by Larowlan. Here is a updated patch and interdiff, please review.

All points of @larowlan have been addressed.
Back to RTBC.

Fixed on commit

diff --git a/core/modules/pgsql/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php b/core/modules/pgsql/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
index 9cb03572f93..681b1d3b293 100644
--- a/core/modules/pgsql/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
+++ b/core/modules/pgsql/tests/src/Functional/Database/PostgreSqlSequenceUpdateTest.php
@@ -65,7 +65,7 @@ public function testPostgreSqlSequenceUpdate() {
    * @return object|bool
    *   Returns the sequence owner object or bool if it does not exist.
    */
-  public function getSequenceOwner(string $table, string $field): object|bool {
+  protected function getSequenceOwner(string $table, string $field): object|bool {
     $update_sequence = \Drupal::classResolver(Update10101::class);
     $seq_name = $update_sequence->getSequenceName($table, $field);
     return \Drupal::database()->query("SELECT d.refobjid::regclass as table_name, a.attname as field_name

Committed 167a7c8 and pushed to 11.x. Thanks!

As there's a new update hook here, this isn't backport eligible so will not be in a tagged release until 10.2

Thanks all.

It needs CR update for protected method which is not public now

Published change record