Re: identity columns

Hello Peter,

I have reviewed the patch.
Currently it does not applies at the top of master, the last commit
without a conflict is 975768f

It compiles and passes "make check" tests, but fails with "make check-world" at:
test foreign_data ... FAILED

It tries to implement SQL:2011 feature T174 ("Identity columns"):
* column definition;
* column altering;
* inserting clauses "OVERRIDING {SYSTEM|USER} VALUE".

It has documentation changes.

===
The implementation has several distinctions from the standard:

1. The standard requires "... ALTER COLUMN ... SET GENERATED { ALWAYS
| BY DEFAULT }" (9075-2:2011 subcl 11.20), but the patch implements it
as "... ALTER COLUMN ... ADD GENERATED { ALWAYS | BY DEFAULT } AS
IDENTITY"

2. The standard requires not more than one identity column, the patch
does not follow that requirement, but it does not mentioned in the
doc.

3. Changes in the table "information_schema.columns" is not full. Some
required columns still empty for identity columns:
* identity_start
* identity_increment
* identity_maximum
* identity_minimum
* identity_cycle

4. "<alter identity column specification>" is not fully implemented
because "<set identity column generation clause>" is implemented
whereas "<alter identity column option>" is not.

5. According to 9075-2:2011 subcl 14.11 Syntax Rule 11)c) for a column
with an indication that values are generated by default the only
possible "<override clause>" is "OVERRIDING USER VALUE".
Implementation allows to use "OVERRIDING SYSTEM VALUE" (as "do
nothing"), but it should be mentioned in "Compatibility" part in the
doc.

postgres=# CREATE TABLE itest10 (a int generated BY DEFAULT as
identity PRIMARY KEY, b text);
CREATE TABLE
postgres=# INSERT INTO itest10 overriding SYSTEM value SELECT 10, 'a';
INSERT 0 1
postgres=# SELECT * FROM itest10;
a | b
---+---
10 | a
(1 row)

===

6. "CREATE TABLE ... (LIKE ... INCLUDING ALL)" fails Assertion at
src/backend/commands/tablecmds.c:631
postgres=# CREATE TABLE itest1 (a int generated by default as identity, b text);
CREATE TABLE
postgres=# CREATE TABLE x(LIKE itest1 INCLUDING ALL);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

===

Also the implementation has several flaws in corner cases:

7. Changing default is allowed but a column is still "identity":

postgres=# CREATE TABLE itest4 (a int GENERATED ALWAYS AS IDENTITY, b text);
CREATE TABLE
postgres=# ALTER TABLE itest4 ALTER COLUMN a set DEFAULT 1;
ALTER TABLE
postgres=# \d itest4
Table "public.itest4"
Column | Type | Modifiers
--------+---------+--------------------------------------------------
a | integer | not null default 1 generated always as identity
b | text |

---
8. Changing a column to be "identity" raises "duplicate key" exception:

postgres=# CREATE TABLE itest4 (a serial, b text);
CREATE TABLE
postgres=# ALTER TABLE itest4 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
ERROR: duplicate key value violates unique constraint
"pg_attrdef_adrelid_adnum_index"

---
9. Changing type of a column deletes linked sequence but leaves
"default" and "identity" marks:

postgres=# CREATE TABLE itest4 (a int GENERATED ALWAYS AS IDENTITY, b int);
CREATE TABLE
postgres=# ALTER TABLE itest4 ALTER COLUMN a TYPE text;
ALTER TABLE
postgres=# \d itest4;
Table "public.itest4"
Column | Type | Modifiers
--------+---------+------------------------------------------------------------------
a | text | default nextval('16445'::regclass) generated
always as identity
b | integer |

postgres=# insert into itest4(b) values(1);
ERROR: could not open relation with OID 16445
postgres=# select * from itest4;
a | b
---+---
(0 rows)

---
10. "identity" modifier is lost when the table inherits another one:

postgres=# CREATE TABLE itest_err_1 (a int);
CREATE TABLE
postgres=# CREATE TABLE x (a int GENERATED ALWAYS AS
IDENTITY)inherits(itest_err_1);
NOTICE: merging column "a" with inherited definition
CREATE TABLE
postgres=# \d itest_err_1; \d x;
Table "public.itest_err_1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Number of child tables: 1 (Use \d+ to list them.)

Table "public.x"
Column | Type | Modifiers
--------+---------+-----------------------------------------------
a | integer | not null default nextval('x_a_seq'::regclass)
Inherits: itest_err_1

---
11. The documentation says "OVERRIDING ... VALUE" can be placed even
before "DEFAULT VALUES", but it is against SQL spec and the
implementation:

postgres=# CREATE TABLE itest10 (a int GENERATED BY DEFAULT AS
IDENTITY, b text);
CREATE TABLE
postgres=# INSERT INTO itest10 DEFAULT VALUES;
INSERT 0 1
postgres=# INSERT INTO itest10 OVERRIDING USER VALUE VALUES(1,2);
INSERT 0 1
postgres=# INSERT INTO itest10 OVERRIDING USER VALUE DEFAULT VALUES;
ERROR: syntax error at or near "DEFAULT" at character 43

---
12. Dump/restore is broken for some cases:

postgres=# CREATE SEQUENCE itest1_a_seq;
CREATE SEQUENCE
postgres=# CREATE TABLE itest1 (a int generated by default as identity, b text);
CREATE TABLE
postgres=# DROP SEQUENCE itest1_a_seq;
DROP SEQUENCE
postgres=# CREATE DATABASE a;
CREATE DATABASE
postgres=# \q

comp ~ $ pg_dump postgres | psql a
SET
SET
SET
SET
SET
SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
SET
SET
SET
CREATE TABLE
ALTER TABLE
ALTER TABLE
COPY 0
ERROR: relation "itest1_a_seq1" does not exist
LINE 1: SELECT pg_catalog.setval('itest1_a_seq1', 2, true);

---
13. doc/src/sgml/ref/create_table.sgml (5th chunk) has "TODO". Why?

---
14. It would be fine if psql has support of new clauses.

===
Also several notes:

15. Initializing attidentity in most places is ' ' but makefuncs.c has
"n->identity = 0;". Is it correct?

---
16. I think it is a good idea to not raise exceptions for "SET
GENERATED/DROP IDENTITY" if a column has the same type of identity/not
an identity. To be consistent with "SET/DROP NOT NULL".

--
Best regards,
Vitaly Burovoy