The PostgreSQL backend does not handle NULL values for serial fields gracefully

For those who aren't familiar with PostgreSQL, some quick explanation is probably in order.

In MySQL, you can use 0 or NULL or DEFAULT as the value of an field with AUTO_INCREMENT to select the next value in the sequence. In PostgreSQL, you have the concept of a "serial" data type, which behaves similarly to an AUTO_INCREMENT field, but you cannot use 0 or NULL to select the next value. You must use DEFAULT.

Here's a quick example on both MySQL and PostgreSQL:

MySQL

[test]> CREATE TABLE blah (
    -> id int PRIMARY KEY NOT NULL AUTO_INCREMENT,
    -> description text);
Query OK, 0 rows affected (0.02 sec)
[test]> INSERT INTO blah VALUES (0,'Alpha');
Query OK, 1 row affected (0.01 sec)
[test]> INSERT INTO blah VALUES (NULL,'Bravo');
Query OK, 1 row affected (0.01 sec)
[test]> INSERT INTO blah VALUES (DEFAULT,'Charlie');
Query OK, 1 row affected (0.01 sec)
[test]> SELECT * FROM blah;
+----+-------------+
| id | description |
+----+-------------+
|  1 | Alpha       |
|  2 | Bravo       |
|  3 | Charlie     |
+----+-------------+
3 rows in set (0.00 sec)

PostgreSQL

test=# CREATE TABLE blah (
test(# id serial,
test(# description text
test(# );
CREATE TABLE
test=# INSERT INTO blah VALUES (0,'Alpha');
INSERT 0 1
test=# INSERT INTO blah VALUES (NULL, 'Bravo');
ERROR:  null value in column "id" violates not-null constraint
DETAIL:  Failing row contains (null, Bravo).
test=# INSERT INTO blah VALUES (DEFAULT, 'Charlie');
INSERT 0 1
test=# SELECT * FROM blah;
 id | description 
----+-------------
  0 | Alpha
  1 | Charlie
(2 rows)

Analysis

If we want Drupal to work well across both MySQL/MariaDB and PostgreSQL, we should:

• use DEFAULT to insert the next ID when inserting values into a table
• not use 0, as PostgreSQL treats it literally
• not use NULL, as PostgreSQL treats it literally, and complains that it violates the NOT NULL constraint on the primary key

This breaks all the things.

Although postgresql does not support SQL NULL in this manner the PDO::pgsql (or libpg) code should support the correct behavior. The issue is that PHP NULL is not the same as unset(). In drupal_write_record(), the value for serial fields is unset and thus not passed in the fields method.

After I added behavior to unset the value for any entity field with the "serial" type, all entities are saved correctly. I don't know if a "serial Field" (Field as in Field UI field) might define multiple columns where one column is serial type and another is of a different type. That could theoretically be possible and this patch would break that, but fix currently broken HEAD.

Edit: This patch requires #2279363: file_managed.uri should not specify a prefixed unique key to manually test, and is blocked by it.

Fixed conditions to find only serial columns that have no data. Works for revisions now too.

Let's see how this does on all the tests.

8: drupal-2279395-revert-serial-value-behavior-8.patch queued for re-testing.

Any chance to move forward here? With the patch applied Drupal installs again on PostgreSQL and I'm really interested how many of the current exceptions will be fixed in PostgreSQL TestBot results.

Patched lastest 8.x branch with no issues. Install completed successfully.

PostgreSQL bugs do not block D8's release, so reducing priority.

I'd love to commit this so we can unblock PostgreSQL bugs preventing it from working, but I'm a little concerned that:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -727,7 +727,7 @@ protected function doSave($id, EntityInterface $entity) {
       // Even if this is a new entity the ID key might have been set, in which
       // case we should not override the provided ID. An empty value for the
       // ID is interpreted as NULL and thus overridden.
-      if (empty($record->{$this->idKey})) {
+      if (!isset($record->{$this->idKey})) {

Unless I'm misreading, this hunk is no longer doing what the comment say it's doing. And presumably it was explicitly empty() before for a reason. Git blame shows it happened in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities, and was first called out in #48 (out of #436).

Tests are still passing without it, so I assume it doesn't break anything (though not sure we test creating a new entity with an ID explicitly set anywhere?), but can we both a) check that that use case is still working correctly and b) update that comment accordingly?

Thank you for the review. The comment indeed needs to be updated. I hope this makes sense in this patch. I did not want to go into too many details about PHP.

I did some digging and did find a test: NodeSaveTest::testImport. :-)

Cool, looks good to me. Thanks for checking. :)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -851,6 +851,14 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
         $record->$schema_name = drupal_schema_get_field_value($definition->getSchema()['columns'][$column_name], $value);
+
+        // Serial field columns must be unset and set as a NULL value to
+        // support all database drivers. Serial fields only have one column.
+        if (count($columns) == 1 &&
+            $this->getSchema()[$table_name]['fields'][$field_name]['type'] == 'serial' &&
+            empty($record->$schema_name)) {
+          unset($record->$schema_name);
+        }

This comment seems strange unsetting and we're not setting the value to NULL.

Plus we've just set the value above - why bother?

Is the why bother comment to why it should be unset() or why it is in the place that it is?

The why bother refers to the fact that we've just set this above. So, I think, something like this is more elegant.

I'll try to manually test this in the next day or so, but still in the middle of deadlines (or run it in my puphpet drupal8.dev environment from github which now supports sqlite and postgresql).

Just tried patch from #19 but PostgreSQL install now fails with:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: http://xxx.xxx.xxx.xxx/core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do
StatusText: OK
ResponseText: Drupal\Core\Entity\EntityStorageException: SQLSTATE[23502]: Not null violation: 7 ERROR:  null value in column "id" violates not-null constraint
DETAIL:  Failing row contains (null, default, cf14db7c-7052-4777-9f00-efdd96908ce9, und). in Drupal\Core\Entity\ContentEntityDatabaseStorage->save()

In other words the Numeric value out of range exception is gone but we fail due NULL inserts.

@bzrudi71 does #15 pass?

Oh - here is a logical difference between 15 and 19.

That may not work either. The difference is that PHP NULL is interpreted as a SQL NULL value, and variable that is unset is interpreted as PostgreSQL DEFAULT. That's why I had a unset().

@alexpott patch from #15 work fine, will give #23 a go...

mradcliffe is right, #23 doesn't work and gives the same error as with #19

The documentation for drupal_schema_get_field_value mentions that it does the typecasting. But I don't think that's possible because we still be setting it to NULL instead of not set at all. Casting via (unset) is not the same as unset() unfortunately.

This works... tested locally. The problem is at which point do we decide that an int field is actually a serial and (in my opinion) we decide this way too late.

Unset does nothing special it just removes the object property. If we never set it then this is better and more elegant.

Minor; don't want to hold off this patch, but found the following worth to comment on:

It took me a good amount of time to understand the following, complex, and ultimately negated condition:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -850,7 +850,14 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
-        $record->$schema_name = drupal_schema_get_field_value($definition->getSchema()['columns'][$column_name], $value);
+
+        // Do not set serial fields if we do not have a value. This supports all
+        // SQL database drivers.
+        // @see https://www.drupal.org/node/2279395
+        $value = drupal_schema_get_field_value($definition->getSchema()['columns'][$column_name], $value);
+        if (!(empty($value) && $this->getSchema()[$table_name]['fields'][$field_name]['type'] == 'serial')) {
+          $record->$schema_name = $value;
+        }

There are various ways to express it differently, but I've the impression you're shooting for the most simple AND:

+        if (!empty($value) && $this->getSchema()[$table_name]['fields'][$field_name]['type'] !== 'serial') {

In turn, I wondered why we're checking empty() instead of isset() for non-serial fields, but it's possible that (A) I didn't fully understand the other changes of this patch, or (B) I still did not understand the intended condition.

Alternatively, (C) the condition might be a bug. ;)

EDIT: Still unsure. Dissecting verbosely:

+        if (!(empty($value) && $this->getSchema()[$table_name]['fields'][$field_name]['type'] == 'serial')) {

↓

+        $is_serial = $this->getSchema()[$table_name]['fields'][$field_name]['type'] == 'serial';
+        if (!(empty($value) && $is_serial)) {

↓

+        $is_serial = $this->getSchema()[$table_name]['fields'][$field_name]['type'] == 'serial';
+        if (!($is_serial && empty($value))) {

↓

+        $is_serial = $this->getSchema()[$table_name]['fields'][$field_name]['type'] == 'serial';
+        if (!$is_serial || !empty($value)) {

↓

+        // Only set a value for serial fields if a value was passed.
+        if ($this->getSchema()[$table_name]['fields'][$field_name]['type'] !== 'serial' || !empty($value)) {

→ If it's not a serial, always set a value. But if it's a serial, only set a value if it's not empty.

(The currently proposed condition in the patch is correct. My initial simplification above is wrong. Definitely worth to simplify.)

All test failures appear to be caused by a missing 'description' schema definition for taxonomy term (or vocabulary?) entities.

I ran into that in an earlier patch that I uploaded, which is why I had the approach that I did in #15.

Edit: see interdiff in #15.

Let's see if we can get rid of some exceptions by adding a description to forum_install() as a first step...

@bzrudi71 not those descriptions - it's the description field - which is a multi-column field - I think the correct fix is in the patch attached.

@alexpott I see... Thought the notices where caused because entity_create() itself expects a description and at least it fixes 34 exceptions ;-) So let's wait for your approach being testet.

Thanks @alextpott! Will try fresh PG install now and later on run all tests on PG.

Patch in #35 seems the absolutely right approach, as

1. Drupal installs fine again on PG
2. All SQLSTATE[22003]: Numeric value out of range errors fixed
3. Hundreds of PG related broken tests fixed (while bot still running)!

RTBC?
Thanks @alexpott

I was not able to get a successful install with the Standard install profile with latest HEAD. This is nothing to do with the patch as I was able to complete the installation after the exception, create content types, taxonomy, and create and update nodes.

I'll file a follow-up issue.

Drupal\Core\Database\SchemaObjectExistsException: Table node__field_image already exists. in Drupal\Core\Database\Schema->createTable() (line 678 of /var/www/drupal8.dev/core/lib/Drupal/Core/Database/Schema.php).

@bzrudi71, did you try the Standard install profile?

Minimal install works, but I didn't get any menus. I was able to still create content types, taxonomy terms, and modify those entities successfully.

Edit: I don't think minimal install profile installs menu items as it's provided by menu links or menu ui now.

@mradcliffe Sorry, but install went fine for me with patch from #35 and latest HEAD.

git reset --hard origin/8.x
HEAD is now at ef4e932 Issue #2301313 by pwolanin, dawehner, Wim Leers, effulgentsia, kgoel, YesCT, glide: MenuLinkNG part3 (no conversions): MenuLinkContent UI.

git apply 2279395.35.patch

git status
On branch 8.x
Your branch is up-to-date with 'origin/8.x'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
	modified:   core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php

Now install with standard profile works for me...

Committed to 8.x. Thanks.

This patch reintroduced the instantiation of database schema during write operations, removing which was one of the goals of #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities. We should explore an alternative solution to fix this. Probably, as @alexpott suggested me in another issue (I don't remember which one, sorry), we should have a serial/sequence field type or a field storage definition setting, so we can perform the same check without needing to recalculate the whole entity schema for just one field.

What about something like this? The whole point of the new approach is that WE know how the entity storage logic works. This should be the default behavior, entity types having more complex layouts can override that method.
This is blocking #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions, btw.

Sorry, $schema_name should be used instead of $column_name.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1017,7 +1017,7 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
   -        if (!(empty($value) && $this->getSchema()[$table_name]['fields'][$schema_name]['type'] == 'serial')) {
   +        if (!empty($value) || !$this->isColumnSerial($table_name, $column_name)) {
   

   This is not quite the same logic. This will now not set serial fields - ever :).

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1027,6 +1027,33 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
      /**
   +   * Checks whether a field column should be treated as serial.
   +   *
   +   * @param $table_name
   +   *   The name of the table the field column belongs to.
   +   * @param $column_name
   +   *   The name of the field column.
   +   *
   +   * @return bool
   +   *   TRUE if the the column is serial, FALSE otherwise.
   +   */
   +  protected function isColumnSerial($table_name, $column_name) {
   +    $result = FALSE;
   +
   +    switch ($table_name) {
   +      case $this->baseTable:
   +        $result = $column_name == $this->idKey;
   +        break;
   +
   +      case $this->revisionTable:
   +        $result = $column_name == $this->revisionKey;
   +        break;
   +    }
   +
   +    return $result;
   +  }
   

   As for changing how we identify if it is a serial... fine by me but we need to add an @sees to ContentEntitySchemaHandler:: processBaseTable() and ContentEntitySchemaHandler:: processRevisionTable()

This is not quite the same logic.

Wikipedia disagrees ;) but since it seems people preferred the previous form, I restored it. Personally I find it less readable.

Users do not use serial ids...

FWIW, that's exactly why I pointed out the complexity of that condition in #30 already.

Even though I'm not affected by this issue in any way, I posted #30, because I found the current condition construct to be interesting enough to spend ~20 minutes to double-check all possible code paths (+ re[verse]-engineer a simpler condition that achieves the same). ;-)

FWIW I totally agree with you :)

Edit: also because the wrapping parentheses can be easily missed. I initially did...

Let's get this fix in to unblock #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions

If we can improve of this later great - and if we can add a test in a followup that'd be great too.

Thanks for the follow-up, I raised eyebrows at the schema getting when I first saw it but failed to post.

Committed/pushed to 8.x, thanks!

I'm not sure if this should go into a new issue or can be sort of an extension of this one.

By the use of "mapToStorageRecord"

we are sending to the database Update statements that include PRIMARY KEYS as fields that need to be updated, such as:

UPDATE xxx SET nid = 1, property0 = :value0 WHERE nid = 1

Although you are updating the Primary Key with the previous value (no update actually) the statement is telling the database engine that you wish to perform an update.

This totally crashes SQL Server, Primary Keys cannot be updated.

I think the issue you mentioned @david_garcia is another follow-up to #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities. That is the issue that got rid of use of drupal_write_record(), which exposed the bug in this issue for pgsql.

I created #2342699: SqlContentEntityStorage tries to update identity/serial values by default in your sqlsrv queue, but it could be moved to the core queue?