From: Peter Eisentraut Date: Fri, 2 Feb 2018 19:20:50 +0000 (-0500) Subject: Fix application of identity values in some cases X-Git-Tag: REL_10_2~11 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=1597948c962a1407c01fc492c44917c097efa92e;p=postgresql.git 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 --- diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 156359476c8..ef301b6328a 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -25,7 +25,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" @@ -3064,19 +3063,8 @@ BeginCopyFrom(ParseState *pstate, { /* attribute is NOT to be copied from input */ /* use default value if one exists */ - Expr *defexpr; - - if (attr[attnum - 1]->attidentity) - { - NextValueExpr *nve = makeNode(NextValueExpr); - - nve->seqid = getOwnedSequence(RelationGetRelid(cstate->rel), - attnum); - nve->typeId = attr[attnum - 1]->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 d22a9d1e1f7..7ea5c4983cd 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5342,7 +5342,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 4d67070b028..8c8384cd6b0 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2812,6 +2812,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 8d92c03633f..68d38fcba1a 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -2544,6 +2544,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 1ca253b41e3..d8cf6601540 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2801,6 +2801,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 f70976ffeff..722637b771f 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -475,6 +475,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 12bd14b531f..f8a2c912a47 100644 --- 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 ef6753e31ad..ecb6cd02498 100644 --- 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 627389b749f..5536044d9f4 100644 --- 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 1b2d11cf343..8be086d7eaf 100644 --- 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 b71be6e4870..cca4480f52e 100644 --- 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; sub wait_for_caught_up { @@ -31,7 +31,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'; @@ -61,8 +61,8 @@ $node_publisher->safe_psql('postgres', "UPDATE test_tab SET b = md5(b)"); wait_for_caught_up($node_publisher, $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 @@ -76,5 +76,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')"); + +wait_for_caught_up($node_publisher, $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;