Fix application of identity values in some cases

Investigation of 2d2d06b7e27e3177d5bef0061801c75946871db3 revealed that
identity values were not applied in some further cases, including
logical replication subscribers, VALUES RTEs, and ALTER TABLE ... ADD
COLUMN.  To fix all that, apply the identity column expression in
build_column_default() instead of repeating the same logic at each call
site.

For ALTER TABLE ... ADD COLUMN ... IDENTITY, the previous coding
completely ignored that existing rows for the new column should have
values filled in from the identity sequence.  The coding using
build_column_default() fails for this because the sequence ownership
isn't registered until after ALTER TABLE, and we can't do it before
because we don't have the column in the catalog yet.  So we specially
remember in ColumnDef the sequence name that we decided on and build a
custom NextValueExpr using that.

Reviewed-by: Michael Paquier <michael.paquier@gmail.com>

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 04a24c60824319c4a0e7691ed316feed61880ab9..b3933df9aff0561f971724932d9102dc80911753 100644 (file)
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -23,7 +23,6 @@
 #include "access/sysattr.h"
 #include "access/xact.h"
 #include "access/xlog.h"
-#include "catalog/dependency.h"
 #include "catalog/pg_type.h"
 #include "commands/copy.h"
 #include "commands/defrem.h"
@@ -2996,19 +2995,8 @@ BeginCopyFrom(ParseState *pstate,
                {
                        /* attribute is NOT to be copied from input */
                        /* use default value if one exists */
-                       Expr       *defexpr;
-
-                       if (att->attidentity)
-                       {
-                               NextValueExpr *nve = makeNode(NextValueExpr);
-
-                               nve->seqid = getOwnedSequence(RelationGetRelid(cstate->rel),
-                                                                                         attnum);
-                               nve->typeId = att->atttypid;
-                               defexpr = (Expr *) nve;
-                       }
-                       else
-                               defexpr = (Expr *) build_column_default(cstate->rel, attnum);
+                       Expr       *defexpr = (Expr *) build_column_default(cstate->rel,
+                                                                                                                               attnum);
 
                        if (defexpr != NULL)
                        {

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 37c7d6688190329b6bc28eb6ecc3158bc5044334..89454d8e80f1997acd0153f82957af8f09e6cf63 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5486,7 +5486,21 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
        if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE
                && relkind != RELKIND_FOREIGN_TABLE && attribute.attnum > 0)
        {
-               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);
 
                if (!defval && DomainHasConstraints(typeOid))
                {

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index fd3001c4934d44016cd616edf9257b3c499e85fa..bafe0d107159b2d4ee33f9b7acf9eecb93f98196 100644 (file)
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2819,6 +2819,7 @@ _copyColumnDef(const ColumnDef *from)
        COPY_NODE_FIELD(raw_default);
        COPY_NODE_FIELD(cooked_default);
        COPY_SCALAR_FIELD(identity);
+       COPY_NODE_FIELD(identitySequence);
        COPY_NODE_FIELD(collClause);
        COPY_SCALAR_FIELD(collOid);
        COPY_NODE_FIELD(constraints);

diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 7d2aa1a2d3a4f945ca40d45fce211a94e2c792c5..02ca7d588ce3676b48cb8a7a05ffc7521eacb8a8 100644 (file)
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2564,6 +2564,7 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
        COMPARE_NODE_FIELD(raw_default);
        COMPARE_NODE_FIELD(cooked_default);
        COMPARE_SCALAR_FIELD(identity);
+       COMPARE_NODE_FIELD(identitySequence);
        COMPARE_NODE_FIELD(collClause);
        COMPARE_SCALAR_FIELD(collOid);
        COMPARE_NODE_FIELD(constraints);

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index e0f4befd9f1f372366ffdc5afad56c274ad734f0..e6ba0962571fc7f1e753b19099865c4b4a16ead1 100644 (file)
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2814,6 +2814,7 @@ _outColumnDef(StringInfo str, const ColumnDef *node)
        WRITE_NODE_FIELD(raw_default);
        WRITE_NODE_FIELD(cooked_default);
        WRITE_CHAR_FIELD(identity);
+       WRITE_NODE_FIELD(identitySequence);
        WRITE_NODE_FIELD(collClause);
        WRITE_OID_FIELD(collOid);
        WRITE_NODE_FIELD(constraints);

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 1d35815fcfe0161d3612ca7d01eff809640dfc75..d415d7180f2ab1d5658b3e79fb8bc7d05329220f 100644 (file)
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -472,6 +472,14 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
 
        cxt->blist = lappend(cxt->blist, seqstmt);
 
+       /*
+        * Store the identity sequence name that we decided on.  ALTER TABLE
+        * ... ADD COLUMN ... IDENTITY needs this so that it can fill the new
+        * column with values from the sequence, while the association of the
+        * sequence with the table is not set until after the ALTER TABLE.
+        */
+       column->identitySequence = seqstmt->sequence;
+
        /*
         * Build an ALTER SEQUENCE ... OWNED BY command to mark the sequence as
         * owned by this column, and add it to the list of things to be done after

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 32e37989725d87f44b7e372d47f6a945cac8f088..66253fc3d3ebf2242e04ccdd25eda3d28f48dbab 100644 (file)
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -844,17 +844,7 @@ rewriteTargetListIU(List *targetList,
                {
                        Node       *new_expr;
 
-                       if (att_tup->attidentity)
-                       {
-                               NextValueExpr *nve = makeNode(NextValueExpr);
-
-                               nve->seqid = getOwnedSequence(RelationGetRelid(target_relation), attrno);
-                               nve->typeId = att_tup->atttypid;
-
-                               new_expr = (Node *) nve;
-                       }
-                       else
-                               new_expr = build_column_default(target_relation, attrno);
+                       new_expr = build_column_default(target_relation, attrno);
 
                        /*
                         * If there is no default (ie, default is effectively NULL), we
@@ -1123,6 +1113,16 @@ build_column_default(Relation rel, int attrno)
        Node       *expr = NULL;
        Oid                     exprtype;
 
+       if (att_tup->attidentity)
+       {
+               NextValueExpr *nve = makeNode(NextValueExpr);
+
+               nve->seqid = getOwnedSequence(RelationGetRelid(rel), attrno);
+               nve->typeId = att_tup->atttypid;
+
+               return (Node *) nve;
+       }
+
        /*
         * Scan to see if relation has a default for this column.
         */

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 76a73b2a377634d98de3ca05f9dfff5d17ca7d64..a16de289ba870b978ea3c0dc159aff323698cb27 100644 (file)
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -647,6 +647,8 @@ typedef struct ColumnDef
        Node       *raw_default;        /* default value (untransformed parse tree) */
        Node       *cooked_default; /* default value (transformed expr tree) */
        char            identity;               /* attidentity setting */
+       RangeVar   *identitySequence; /* to store identity sequence name for ALTER
+                                                                  * TABLE ... ADD COLUMN */
        CollateClause *collClause;      /* untransformed COLLATE spec, if any */
        Oid                     collOid;                /* collation OID (InvalidOid if not set) */
        List       *constraints;        /* other constraints on column */

diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 627389b749f441263fcd3ace8d37766921d088f0..5536044d9f4d88180b4c1748181336e20b4518b7 100644 (file)
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -104,6 +104,19 @@ SELECT * FROM itest4;
  2 | 
 (2 rows)
 
+-- VALUES RTEs
+INSERT INTO itest3 VALUES (DEFAULT, 'a');
+INSERT INTO itest3 VALUES (DEFAULT, 'b'), (DEFAULT, 'c');
+SELECT * FROM itest3;
+ a  | b 
+----+---
+  7 | 
+ 12 | 
+ 17 | a
+ 22 | b
+ 27 | c
+(5 rows)
+
 -- OVERRIDING tests
 INSERT INTO itest1 VALUES (10, 'xyz');
 INSERT INTO itest1 OVERRIDING USER VALUE VALUES (10, 'xyz');
@@ -237,6 +250,21 @@ SELECT * FROM itestv11;
  11 | xyz
 (3 rows)
 
+-- ADD COLUMN
+CREATE TABLE itest13 (a int);
+-- add column to empty table
+ALTER TABLE itest13 ADD COLUMN b int GENERATED BY DEFAULT AS IDENTITY;
+INSERT INTO itest13 VALUES (1), (2), (3);
+-- add column to populated table
+ALTER TABLE itest13 ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY;
+SELECT * FROM itest13;
+ a | b | c 
+---+---+---
+ 1 | 1 | 1
+ 2 | 2 | 2
+ 3 | 3 | 3
+(3 rows)
+
 -- various ALTER COLUMN tests
 -- fail, not allowed for identity columns
 ALTER TABLE itest1 ALTER COLUMN a SET DEFAULT 1;

diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 1b2d11cf343a382bac2d7d054984488f885fb604..8be086d7eaf00b9c2e90ece038e3b28199d476bc 100644 (file)
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -54,6 +54,14 @@ SELECT * FROM itest3;
 SELECT * FROM itest4;
 
 
+-- VALUES RTEs
+
+INSERT INTO itest3 VALUES (DEFAULT, 'a');
+INSERT INTO itest3 VALUES (DEFAULT, 'b'), (DEFAULT, 'c');
+
+SELECT * FROM itest3;
+
+
 -- OVERRIDING tests
 
 INSERT INTO itest1 VALUES (10, 'xyz');
@@ -138,6 +146,17 @@ INSERT INTO itestv11 OVERRIDING SYSTEM VALUE VALUES (11, 'xyz');
 SELECT * FROM itestv11;
 
 
+-- ADD COLUMN
+
+CREATE TABLE itest13 (a int);
+-- add column to empty table
+ALTER TABLE itest13 ADD COLUMN b int GENERATED BY DEFAULT AS IDENTITY;
+INSERT INTO itest13 VALUES (1), (2), (3);
+-- add column to populated table
+ALTER TABLE itest13 ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY;
+SELECT * FROM itest13;
+
+
 -- various ALTER COLUMN tests
 
 -- fail, not allowed for identity columns

diff --git a/src/test/subscription/t/008_diff_schema.pl b/src/test/subscription/t/008_diff_schema.pl
index ea3162540295afc5abc5afdbf7632ca156e8bad3..d4849c89a3fd13c467bb49686c13253b06b76caa 100644 (file)
--- a/src/test/subscription/t/008_diff_schema.pl
+++ b/src/test/subscription/t/008_diff_schema.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 3;
+use Test::More tests => 4;
 
 # Create publisher node
 my $node_publisher = get_new_node('publisher');
@@ -22,7 +22,7 @@ $node_publisher->safe_psql('postgres',
        "INSERT INTO test_tab VALUES (1, 'foo'), (2, 'bar')");
 
 # Setup structure on subscriber
-$node_subscriber->safe_psql('postgres', "CREATE TABLE test_tab (a int primary key, b text, c timestamptz DEFAULT now(), d bigint DEFAULT 999)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE test_tab (a int primary key, b text, c timestamptz DEFAULT now(), d bigint DEFAULT 999, e int GENERATED BY DEFAULT AS IDENTITY)");
 
 # Setup logical replication
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -52,8 +52,8 @@ $node_publisher->safe_psql('postgres', "UPDATE test_tab SET b = md5(b)");
 $node_publisher->wait_for_catchup($appname);
 
 $result =
-  $node_subscriber->safe_psql('postgres', "SELECT count(*), count(c), count(d = 999) FROM test_tab");
-is($result, qq(2|2|2), 'check extra columns contain local defaults');
+  $node_subscriber->safe_psql('postgres', "SELECT count(*), count(c), count(d = 999), count(e) FROM test_tab");
+is($result, qq(2|2|2|2), 'check extra columns contain local defaults after copy');
 
 # Change the local values of the extra columns on the subscriber,
 # update publisher, and check that subscriber retains the expected
@@ -67,5 +67,15 @@ $result =
   $node_subscriber->safe_psql('postgres', "SELECT count(*), count(extract(epoch from c) = 987654321), count(d = 999) FROM test_tab");
 is($result, qq(2|2|2), 'check extra columns contain locally changed data');
 
+# Another insert
+$node_publisher->safe_psql('postgres',
+       "INSERT INTO test_tab VALUES (3, 'baz')");
+
+$node_publisher->wait_for_catchup($appname);
+
+$result =
+  $node_subscriber->safe_psql('postgres', "SELECT count(*), count(c), count(d = 999), count(e) FROM test_tab");
+is($result, qq(3|3|3|3), 'check extra columns contain local defaults after apply');
+
 $node_subscriber->stop;
 $node_publisher->stop;