BUG #16913: GENERATED AS IDENTITY column nullability is affected by order of column properties

On 3/4/21 5:13 AM, PG Bug reporting form wrote:

The following bug has been logged on the website:

Bug reference: 16913
Logged by: Pavel Boev
Email address: pavel.boev@invitae.com
PostgreSQL version: 13.2
Operating system: Debian 8.3.0-6
Description:

I noticed that nullability of columns having GENERATED AS IDENTITY option is
affected by the order of NULL option, so the next query will create a
"my_table" table having non-nullable column "generated":

create table my_table (
generated integer null generated by default as identity,
data text not null
);

Whereas the following query will create a "my_table" table having nullable
column "generated":

create table my_table (
generated integer generated by default as identity null,
data text not null
);

I noticed the issue appeared in postgres 13.2, however folks say that they
bug affects versions 10, 11 and 12 as well.

I can confirm this bug. Attached is a patch to prevent it.
--
Vik Fearing

On 3/4/21 3:22 PM, Vik Fearing wrote:

I can confirm this bug. Attached is a patch to prevent it.

Would anybody like to take a look at this?
--
Vik Fearing

On Thu, Mar 11, 2021 at 11:37 PM Vik Fearing <vik@postgresfriends.org> wrote:

On 3/4/21 3:22 PM, Vik Fearing wrote:

I can confirm this bug. Attached is a patch to prevent it.

Would anybody like to take a look at this?

Patch looks reasonable to me.

There seems to be an empty line added at the end, which git diff --check flags:

$ git diff --check
src/test/regress/sql/identity.sql:356: new blank line at EOF.

+-- Identity columns must be NOT NULL (cf bug #16913)
+
+CREATE TABLE itest15 (id integer GENERATED ALWAYS AS IDENTITY NULL); -- fail
+CREATE TABLE itest15 (id integer NULL GENERATED ALWAYS AS IDENTITY); -- fail
+CREATE TABLE itest15 (id integer GENERATED ALWAYS AS IDENTITY NOT
NULL); DROP TABLE itest15;
+CREATE TABLE itest15 (id integer NOT NULL GENERATED ALWAYS AS
IDENTITY); DROP TABLE itest15;
+

--
Amit Langote
EDB: http://www.enterprisedb.com

Amit Langote <amitlangote09@gmail.com> writes:

On Thu, Mar 11, 2021 at 11:37 PM Vik Fearing <vik@postgresfriends.org> wrote:

On 3/4/21 3:22 PM, Vik Fearing wrote:

I can confirm this bug. Attached is a patch to prevent it.

Would anybody like to take a look at this?

Patch looks reasonable to me.

I think it's not really necessary to have two different tests; we can
just make the code for IDENTITY include exactly what the code for
CONSTR_NOTNULL does, ie it can set saw_not_null too. The point here
is that IDENTITY can be read as implicitly including NOT NULL, so we
can just act as though that was explicit.

An alternative fix approach is to add to the list of conflict checks
that are made after the loop, but I'm not sure that's any better.

Will push it in a bit.

regards, tom lane

On 3/12/21 4:30 PM, Tom Lane wrote:

Amit Langote <amitlangote09@gmail.com> writes:

On Thu, Mar 11, 2021 at 11:37 PM Vik Fearing <vik@postgresfriends.org> wrote:

On 3/4/21 3:22 PM, Vik Fearing wrote:

I can confirm this bug. Attached is a patch to prevent it.

Would anybody like to take a look at this?

Patch looks reasonable to me.

Will push it in a bit.

Thanks, Tom.
--
Vik Fearing

Hi all.

I can see the PG 13.3 change note about GENERATED ALWAYS AS IDENTITY no
longer being compatible with an explicit NULL specification. However, it
seems that GENERATED BY DEFAULT AS IDENTITY also is no longer compatible
with null:

CREATE TABLE foo
(
id INTEGER NULL GENERATED BY DEFAULT AS IDENTITY
);

Results in:

ERROR: conflicting NULL/NOT NULL declarations for column "id" of table
"foo"

Is this intended? It seems to make sense to allow NULL to be explicitly
inserted into columns whith as GENERATED BY DEFAULT AS IDENTITY. If this is
intended, maybe the release notes should be updated for this.

Shay Rojansky <roji@roji.org> writes:

I can see the PG 13.3 change note about GENERATED ALWAYS AS IDENTITY no
longer being compatible with an explicit NULL specification. However, it
seems that GENERATED BY DEFAULT AS IDENTITY also is no longer compatible
with null:

CREATE TABLE foo
(
id INTEGER NULL GENERATED BY DEFAULT AS IDENTITY
);

Results in:

ERROR: conflicting NULL/NOT NULL declarations for column "id" of table
"foo"

Is this intended?

GENERATED BY DEFAULT does create a NOT NULL constraint:

regression=# CREATE TABLE foo(id INTEGER GENERATED BY DEFAULT AS IDENTITY);
CREATE TABLE
regression=# \d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+----------------------------------
id | integer | | not null | generated by default as identity

so I think the patch is doing what it was intended to. Whether GENERATED
BY DEFAULT *should* be forcing NOT NULL is a separate question, but
AFAIK it always has.

regards, tom lane

I wrote:

GENERATED BY DEFAULT does create a NOT NULL constraint:
...
so I think the patch is doing what it was intended to. Whether GENERATED
BY DEFAULT *should* be forcing NOT NULL is a separate question, but
AFAIK it always has.

Ah, found it. SQL:2016 11.4 <column definition> syntax rule 16 saith:

If <identity column specification> ICS is specified, then:
...
d) The <column constraint definition> NOT NULL NOT DEFERRABLE is implicit.

The <identity column specification> production includes both cases:

<identity column specification> ::=
GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
[ <left paren> <common sequence generator options> <right paren> ]

so the spec does clearly say that both alternatives force NOT NULL.

So, it was my error to write the release notes as though only
GENERATED ALWAYS is affected. I'll go adjust that, though
it won't propagate to the website for another three months :-(

regards, tom lane

I'll go adjust that

Thanks Tom.

so the spec does clearly say that both alternatives force NOT NULL.

For what it's worth, it's odd to disallow nullable columns which are also
GENERATED BY DEFAULT AS IDENTITY - unless I'm missing something, it seems
like quite an artificial restriction for a legitimate scenario. After all,
we can do:

CREATE SEQUENCE foo_seq AS integer;
CREATE TABLE foo (
bar INTEGER NULL DEFAULT nextval('foo_seq')
);

... which is logically very similar, and definitely seems useful. Would it
make sense to allow nullable GENERATED BY DEFAULT AS IDENTITY as a PG
extension?

I'll go adjust that

One last pedantic note: the CREATE TABLE page[1] doesn't seem to mention
the syntactic incompatibility between GENERATED ... AS IDENTITY and NULL.