New serial columns do not own sequences

I have rerolled the patch for 10.1.

This is blocking 📌 Fix batch process race conditions by making ‘bid (batch id)’ auto-increment Fixed , 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.

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

The last submitted patch, 91: 3028706-91.patch, failed testing. View results →

+++ 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 ( 📌 New serial columns do not own sequences for on demand tables Postponed ).

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

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

Published change record