PostgreSQL constraints are still not renamed properly on table renames

Anything blocking a critical is critical in my book :)

Do we need a change notice here?

Thanks, for clearing that up. The patch is green and #2984782-56: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data is also green. The changes look good and are well documented so setting it to RTBC.

Wow, impressive detective work :)

I have a few doubts, but I'm no PgSql expert so they might be moot. Anyway, better safe than sorry:

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -561,12 +575,21 @@ public function renameTable($table, $new_name) {
   -        $old_sequence = $this->prefixNonTable($table, $field, 'seq');
   ...
   +        // The initial name of the sequence is generated automatically by
   +        // PostgreSQL when the table is created, so we need to use
   +        // pg_get_serial_sequence() to retrieve it.
   +        $old_sequence = $this->connection->query("SELECT pg_get_serial_sequence('" . $old_full_name . "', '" . $field . "')")->fetchField();
   

   Does this mean that $this->prefixNonTable($table, $field, 'seq'); generates a sequence name that matches PostgreSQL's default behavior, unless its length is greater than 63, in which case the actual name is unpredictable? If this is not the case, then I don't understand how this code was working before the change :)

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -561,12 +575,21 @@ public function renameTable($table, $new_name) {
   -        $new_sequence = $this->prefixNonTable($new_name, $field, 'seq');
   ...
   +        // The new sequence name generated with ::ensureIdentifiersLength() does
   +        // not match the pattern that is automatically applied by PostgreSQL on
   +        // table creation, but that's ok because pg_get_serial_sequence() will
   +        // return our non-standard name on subsequent table renames.
   +        $new_sequence = $this->ensureIdentifiersLength($new_name, $field, 'seq');
   

   Assuming a pg_ prefix, $this->prefixNonTable('new_name', 'field', 'seq') returns pg_new_name_field_seq, however the new code would return pg_new_name__field__seq. To minimize the risk of introducing a BC break, I think we should change this line to:

   $new_sequence = $this->ensureIdentifiersLength($new_name . '_' . $field . '_seq');
   

3. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -214,7 +214,13 @@ public function testSchema() {
   +    // PostgreSQL has a max identifier length of 63 characters, MySQL has 64 and
   +    // SQLite does not have any limit. Use the lowest common value and create a
   +    // table name as long as possible in order to cover edge cases around
   +    // identifier names for the table's primary or unique key constraints.
   +    $table_name = strtolower($this->getRandomGenerator()->name(63 - strlen($this->getDatabasePrefix())));
   +    $this->schema->createTable($table_name, $table_specification);
   

Since we are at this, what about using 64 for MySql? That way we actually test limits for all db types we support. We could introduce an helper method to avoid repeating this logic all over the place.

There's no risk of introducing a BC break :)

I agree that the current code does not break renaming tables for existing sites and is BC with the previous names, but what if some custom code is querying those values for any reason? I know it's far fetched, but given that the solution for that is just tweaking one line, I think it would be worth it, unless the tweak I suggested has some drawbacks I'm missing.

Mmmh, I tested it both manually and automatically and it appeared to work. However, upon a closer look, I guess it might fail if a per-table prefix has been set, so scratch that :)

I added the additional test runs a few days ago to compare against any performance regressions on the test bots, but I never got back to looking at the results. These look fine.

The internals of pg_get_serial_sequence look good and I don't see anything costly like getting from information_schema, which is slow.

For that aspect, +1 to the patch.

I agree that the current code does not break renaming tables for existing sites and is BC with the previous names, but what if some custom code is querying those values for any reason?

In the past, not related to Drupal, I've used currval, nextval and setval functions, which allow me to specify the sequence name. I think in a custom application that's not a CMS that would be fine, but I think that if we're working with a dynamic system, then using pg_get_serial_sequence inside one of those functions would be the best.

The Database API handbook doesn't reference anything here as a guide for developers other than ("You should almost never be making database calls directly unless you are developing core APIs."). However since our DBAL isn't fully-featured, if someone were to be concatenating values to query a sequence dynamically for any reason, then they would be caught off-guard and their code would break.

Maybe a change record that demonstrates usage change would help?

If you're doing something like this,

  $node->save();
  $seq = $connection->select("SELECT currval('node_nid_seq'::regclass)")->execute();

Then do something like this instead

  $node->save();
  $seq = $connection->select("SELECT currval(pg_get_serial_sequence('{node}', 'nid'))")->execute();

That might not be accurate usage of table prefixes, and I'm having a hard time coming up with a good Drupal example. The above doesn't really make sense in actual usage because I'd just get nid from the new Node entity? I think I'd need to be doing something without entities, directly querying database on a Drupal web application that was querying and inserting data into a shared database?

I was reviewing the CR and how table prefixes work when I came across

makeSequenceName method in Connection. This is used by pgsql driver only (in core, but maybe a contrib driver also uses it?).

I think that may need to change as well..?

I'm setting to Needs review to answer that.

Wow, that method is fairly old. nextId() is now only called by user module in core from what I can tell. There's a mention in the entity storage API. The change to nextId() would then be an actual example for the change record.

We might remove the makeSequenceName usage in nextId() and replace with getting via pg_get_serial_sequence(), and then should we deprecate makeSequenceName in favor of Schema::prefixNonTable() (or the aforementioned change record) since it would then be unused?

If makeSequenceName is in the API, then are there more BC concerns?

More questions, sorry. :|

Edit: grammar

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -584,11 +593,12 @@ public function renameTable($table, $new_name) {
   +        $new_sequence = $this->ensureIdentifiersLength($new_name, $field, 'seq', '_');
   
   @@ -952,7 +962,7 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = []) {
   -      $seq = "{" . $table . "}_" . $field_new . "_seq";
   +      $seq = $this->connection->makeSequenceName($table, $field_new);
   

   ::makeSequenceName() does not handle the PG identifier limit, shouldn't we override it and call ::ensureIdentifiersLength() internally? This way both places would be correctly dealing with long identifiers.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -74,7 +83,7 @@ protected function ensureIdentifiersLength($identifier) {
   -      $saveIdentifier = '"drupal_' . $this->hashBase64($identifierName) . '_' . $args[2] . '"';
   +      $saveIdentifier = '"drupal_' . $this->hashBase64($identifierName) . '_' . $tag . '"';
   

   I just noticed that the identifier name is enclosed in double quotes, so those will end up in the sequence name (see the attached screenshots). Shouldn't we trim() them?

(posting the same screenshot twice doesn't help)

@#20:
1: 👍
2: Ha, tricky :) Confirmed this behavior by creating a Test table (see attached screen).

It seems indeed that if we want to keep uppercase characters we need to leave double quotes around, however those seem to be actually part of the sequence name, or at least there's no way to refer to the sequence name without them:

drupal_8x_dev=> SELECT pg_get_serial_sequence('Test', 'id');
ERROR:  relation "test" does not exist

drupal_8x_dev=> SELECT pg_get_serial_sequence('"Test"', 'id');
 pg_get_serial_sequence 
------------------------
 public."Test_id_seq"
(1 row)

And its usage in \Drupal\Core\Database\Driver\pgsql\Connection::nextId() is legitimate because we know that the resulting name will not exceed 63 chars.

Ah, thanks, @amateescu. This seems odd. The two usages of nextId are UserStorage and batches in form.inc? And in the pgsql driver it's always going to use the "sequences_value_seq" sequence for.. both? So user ids are tied to batch ids? Uh, hrm, maybe we should change that in a follow-up. That's probably going to require some extensive changes to either oul DBAL or UserStorage (and drupal 7).

@@ -929,7 +962,7 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = []) {
       // Type "serial" is known to PostgreSQL, but *only* during table creation,
       // not when altering. Because of that, the sequence needs to be created
       // and initialized by hand.
-      $seq = "{" . $table . "}_" . $field_new . "_seq";
+      $seq = $this->connection->makeSequenceName($table, $field_new);
       $this->connection->query("CREATE SEQUENCE " . $seq);
       // Set sequence to maximal field value to not conflict with existing
       // entries.

I found another Major issue with changeField, but it's not related here. I don't see where we're relating a new sequence to the table field. It should use the OWNED BY statement, but it's not. This means anyone who's changed a field to be serial has floating sequences. This probably doesn't matter, but pg_get_serial_sequence will not find it.

That seems like database update territory especially if we publish the CR about using pg_get_serial_sequence.

I also found that the sequences automagically created with CREATE TABLE and sequences created with CREATE SEQUENCE are not truncated in the same way. The former will truncate the table name adding the trailing _, but the latter will truncate entire thing. But that shouldn't matter if you always (and can) use pg_get_serial_sequence.

So I think that the change record should reflect a better example on manually created sequences or for non-entity tables.

I feel like the issue summary could be updated after the discussion in the last week to provide some background information about driver things and answers to some of the questions.

Thank you, @amateescu.

I created follow-up issues for pgsql driver. I think it's safe to postpone the CREATE SEQUENCE one after this because there are no core updates that change fields into serial columns.

- #3028706: New serial columns do not own sequences
- #3028709: Change behavior of PostgreSQL to not share sequence for batch and users

Let's flip it back to RTBC for #17.

Thanks for the very thorough reviews, @mradcliffe!

Committed 1b46f2f and pushed to 8.7.x. Thanks!

Since this was only marked critical because it is blocking a critical issue and it might require to follow the change record to be fully compliant with the new logic, I think it's ok to just commit this to 8.7.x. I will ping a release manager to confirm this.

I think the title of the CR is sufficient information for the release note here, so I've used that. This is potentially disruptive so tagging for the release notes directly.