Re: BUG #14952: COPY fails to fill in IDENTITY column default value - Mailing list pgsql-bugs

On Sat, Dec 16, 2017 at 2:37 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 12/13/17 22:39, Tom Lane wrote:
>> For that matter, should build_column_default() know about it explicitly
>> either?  Why aren't we implementing IDENTITY by storing an appropriate
>> default expression in the catalogs?
>
> Initial versions were coded that way, but it was pretty messy and buggy
> because you have to touch a lot of places to teach them the difference
> between a real default expression and a synthetic one.  Another problem
> is that the NextValueExpr has special permission behavior that is not
> really representable by a normal expression, thus introducing more
> special cases, and possibly permission or security issues.

I have no major objections about 0001. Here are some comments.

@@ -1123,6 +1113,16 @@ build_column_default(Relation rel, int attrno)
    Node       *expr = NULL;
    Oid         exprtype;

+   if (att_tup->attidentity)
+   {
I would add a comment explaining why this should come first.

+INSERT INTO itest3 VALUES (DEFAULT, 'a');
+INSERT INTO itest3 VALUES (DEFAULT, 'b'), (DEFAULT, 'c');
+SELECT * FROM itest3;
Thanks for adding a test with INSERT RTEs.

+-- ADD COLUMN
+CREATE TABLE itest13 (a int);
+-- add column to empty table
+ALTER TABLE itest13 ADD COLUMN b int GENERATED BY DEFAULT AS IDENTITY;
+ERROR:  no owned sequence found
Those are hitting an elog(), so I think that it would be better to
issue a proper ereport() instead with ERRCODE_FEATURE_NOT_SUPPORTED as
errcode. It is easier to understand the point you are going to
though...

Now coming to 0002...

-       defval = (Expr *) build_column_default(rel, attribute.attnum);
+       /*
+        * For an identity column, we can't use build_column_default(),
+        * because the sequence ownership isn't set yet.  So do it manually.
+        */
+       if (colDef->identity)
+       {
+           NextValueExpr *nve = makeNode(NextValueExpr);
+
+           nve->seqid = RangeVarGetRelid(colDef->identitySequence,
NoLock, false);
+           nve->typeId = typeOid;
+
+           defval = (Expr *) nve;
+       }
+       else
+           defval = (Expr *) build_column_default(rel, attribute.attnum);
I have a problem with this approach which is basically something
similar to what I complained in the thread about typed and partition
tables for identity columns
(https://www.postgresql.org/message-id/20171023074458.1473.25799@wrigleys.postgresql.org).
In my opinion, this ALTER TABLE handling should be done by treating
identity columns a way similar to default expressions in
transformColumnDefinition(), by storing the FuncExpr node at parsing
time instead of storing the information needed to rebuild it when
executing the query. In short the mapping should get closer to what
default does with nextval or serial.
-- 
Michael