Skip to content

Commit 533c5d8

Browse files
peterepetere
committed
Fix application of identity values in some cases
Investigation of 2d2d06b 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>
1 parent 9da0cc3 commit 533c5d8

File tree

11 files changed

+102
-30
lines changed

11 files changed

+102
-30
lines changed

‎src/backend/commands/copy.c

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
#include "access/sysattr.h"
2424
#include "access/xact.h"
2525
#include "access/xlog.h"
26-
#include "catalog/dependency.h"
2726
#include "catalog/pg_type.h"
2827
#include "commands/copy.h"
2928
#include "commands/defrem.h"
@@ -2996,19 +2995,8 @@ BeginCopyFrom(ParseState *pstate,
29962995
{
29972996
/* attribute is NOT to be copied from input */
29982997
/* use default value if one exists */
2999-
Expr *defexpr;
3000-
3001-
if (att->attidentity)
3002-
{
3003-
NextValueExpr *nve = makeNode(NextValueExpr);
3004-
3005-
nve->seqid = getOwnedSequence(RelationGetRelid(cstate->rel),
3006-
attnum);
3007-
nve->typeId = att->atttypid;
3008-
defexpr = (Expr *) nve;
3009-
}
3010-
else
3011-
defexpr = (Expr *) build_column_default(cstate->rel, attnum);
2998+
Expr *defexpr = (Expr *) build_column_default(cstate->rel,
2999+
attnum);
30123000

30133001
if (defexpr != NULL)
30143002
{

‎src/backend/commands/tablecmds.c

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5486,7 +5486,21 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
54865486
if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE
54875487
&& relkind != RELKIND_FOREIGN_TABLE && attribute.attnum > 0)
54885488
{
5489-
defval = (Expr *) build_column_default(rel, attribute.attnum);
5489+
/*
5490+
* For an identity column, we can't use build_column_default(),
5491+
* because the sequence ownership isn't set yet. So do it manually.
5492+
*/
5493+
if (colDef->identity)
5494+
{
5495+
NextValueExpr *nve = makeNode(NextValueExpr);
5496+
5497+
nve->seqid = RangeVarGetRelid(colDef->identitySequence, NoLock, false);
5498+
nve->typeId = typeOid;
5499+
5500+
defval = (Expr *) nve;
5501+
}
5502+
else
5503+
defval = (Expr *) build_column_default(rel, attribute.attnum);
54905504

54915505
if (!defval && DomainHasConstraints(typeOid))
54925506
{

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2819,6 +2819,7 @@ _copyColumnDef(const ColumnDef *from)
28192819
COPY_NODE_FIELD(raw_default);
28202820
COPY_NODE_FIELD(cooked_default);
28212821
COPY_SCALAR_FIELD(identity);
2822+
COPY_NODE_FIELD(identitySequence);
28222823
COPY_NODE_FIELD(collClause);
28232824
COPY_SCALAR_FIELD(collOid);
28242825
COPY_NODE_FIELD(constraints);

‎src/backend/nodes/equalfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2564,6 +2564,7 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
25642564
COMPARE_NODE_FIELD(raw_default);
25652565
COMPARE_NODE_FIELD(cooked_default);
25662566
COMPARE_SCALAR_FIELD(identity);
2567+
COMPARE_NODE_FIELD(identitySequence);
25672568
COMPARE_NODE_FIELD(collClause);
25682569
COMPARE_SCALAR_FIELD(collOid);
25692570
COMPARE_NODE_FIELD(constraints);

‎src/backend/nodes/outfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2814,6 +2814,7 @@ _outColumnDef(StringInfo str, const ColumnDef *node)
28142814
WRITE_NODE_FIELD(raw_default);
28152815
WRITE_NODE_FIELD(cooked_default);
28162816
WRITE_CHAR_FIELD(identity);
2817+
WRITE_NODE_FIELD(identitySequence);
28172818
WRITE_NODE_FIELD(collClause);
28182819
WRITE_OID_FIELD(collOid);
28192820
WRITE_NODE_FIELD(constraints);

‎src/backend/parser/parse_utilcmd.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,14 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
472472

473473
cxt->blist = lappend(cxt->blist, seqstmt);
474474

475+
/*
476+
* Store the identity sequence name that we decided on. ALTER TABLE
477+
* ... ADD COLUMN ... IDENTITY needs this so that it can fill the new
478+
* column with values from the sequence, while the association of the
479+
* sequence with the table is not set until after the ALTER TABLE.
480+
*/
481+
column->identitySequence = seqstmt->sequence;
482+
475483
/*
476484
* Build an ALTER SEQUENCE ... OWNED BY command to mark the sequence as
477485
* owned by this column, and add it to the list of things to be done after

‎src/backend/rewrite/rewriteHandler.c

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -844,17 +844,7 @@ rewriteTargetListIU(List *targetList,
844844
{
845845
Node *new_expr;
846846

847-
if (att_tup->attidentity)
848-
{
849-
NextValueExpr *nve = makeNode(NextValueExpr);
850-
851-
nve->seqid = getOwnedSequence(RelationGetRelid(target_relation), attrno);
852-
nve->typeId = att_tup->atttypid;
853-
854-
new_expr = (Node *) nve;
855-
}
856-
else
857-
new_expr = build_column_default(target_relation, attrno);
847+
new_expr = build_column_default(target_relation, attrno);
858848

859849
/*
860850
* If there is no default (ie, default is effectively NULL), we
@@ -1123,6 +1113,16 @@ build_column_default(Relation rel, int attrno)
11231113
Node *expr = NULL;
11241114
Oid exprtype;
11251115

1116+
if (att_tup->attidentity)
1117+
{
1118+
NextValueExpr *nve = makeNode(NextValueExpr);
1119+
1120+
nve->seqid = getOwnedSequence(RelationGetRelid(rel), attrno);
1121+
nve->typeId = att_tup->atttypid;
1122+
1123+
return (Node *) nve;
1124+
}
1125+
11261126
/*
11271127
* Scan to see if relation has a default for this column.
11281128
*/

‎src/include/nodes/parsenodes.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -647,6 +647,8 @@ typedef struct ColumnDef
647647
Node *raw_default; /* default value (untransformed parse tree) */
648648
Node *cooked_default; /* default value (transformed expr tree) */
649649
char identity; /* attidentity setting */
650+
RangeVar *identitySequence; /* to store identity sequence name for ALTER
651+
* TABLE ... ADD COLUMN */
650652
CollateClause *collClause; /* untransformed COLLATE spec, if any */
651653
Oid collOid; /* collation OID (InvalidOid if not set) */
652654
List *constraints; /* other constraints on column */

‎src/test/regress/expected/identity.out

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,19 @@ SELECT * FROM itest4;
104104
2 |
105105
(2 rows)
106106

107+
-- VALUES RTEs
108+
INSERT INTO itest3 VALUES (DEFAULT, 'a');
109+
INSERT INTO itest3 VALUES (DEFAULT, 'b'), (DEFAULT, 'c');
110+
SELECT * FROM itest3;
111+
a | b
112+
----+---
113+
7 |
114+
12 |
115+
17 | a
116+
22 | b
117+
27 | c
118+
(5 rows)
119+
107120
-- OVERRIDING tests
108121
INSERT INTO itest1 VALUES (10, 'xyz');
109122
INSERT INTO itest1 OVERRIDING USER VALUE VALUES (10, 'xyz');
@@ -237,6 +250,21 @@ SELECT * FROM itestv11;
237250
11 | xyz
238251
(3 rows)
239252

253+
-- ADD COLUMN
254+
CREATE TABLE itest13 (a int);
255+
-- add column to empty table
256+
ALTER TABLE itest13 ADD COLUMN b int GENERATED BY DEFAULT AS IDENTITY;
257+
INSERT INTO itest13 VALUES (1), (2), (3);
258+
-- add column to populated table
259+
ALTER TABLE itest13 ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY;
260+
SELECT * FROM itest13;
261+
a | b | c
262+
---+---+---
263+
1 | 1 | 1
264+
2 | 2 | 2
265+
3 | 3 | 3
266+
(3 rows)
267+
240268
-- various ALTER COLUMN tests
241269
-- fail, not allowed for identity columns
242270
ALTER TABLE itest1 ALTER COLUMN a SET DEFAULT 1;

‎src/test/regress/sql/identity.sql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,14 @@ SELECT * FROM itest3;
5454
SELECT * FROM itest4;
5555

5656

57+
-- VALUES RTEs
58+
59+
INSERT INTO itest3 VALUES (DEFAULT, 'a');
60+
INSERT INTO itest3 VALUES (DEFAULT, 'b'), (DEFAULT, 'c');
61+
62+
SELECT * FROM itest3;
63+
64+
5765
-- OVERRIDING tests
5866

5967
INSERT INTO itest1 VALUES (10, 'xyz');
@@ -138,6 +146,17 @@ INSERT INTO itestv11 OVERRIDING SYSTEM VALUE VALUES (11, 'xyz');
138146
SELECT * FROM itestv11;
139147

140148

149+
-- ADD COLUMN
150+
151+
CREATE TABLE itest13 (a int);
152+
-- add column to empty table
153+
ALTER TABLE itest13 ADD COLUMN b int GENERATED BY DEFAULT AS IDENTITY;
154+
INSERT INTO itest13 VALUES (1), (2), (3);
155+
-- add column to populated table
156+
ALTER TABLE itest13 ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY;
157+
SELECT * FROM itest13;
158+
159+
141160
-- various ALTER COLUMN tests
142161

143162
-- fail, not allowed for identity columns

‎src/test/subscription/t/008_diff_schema.pl

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
use warnings;
44
use PostgresNode;
55
use TestLib;
6-
use Test::More tests => 3;
6+
use Test::More tests => 4;
77

88
# Create publisher node
99
my $node_publisher = get_new_node('publisher');
@@ -22,7 +22,7 @@
2222
"INSERT INTO test_tab VALUES (1, 'foo'), (2, 'bar')");
2323

2424
# Setup structure on subscriber
25-
$node_subscriber->safe_psql('postgres', "CREATE TABLE test_tab (a int primary key, b text, c timestamptz DEFAULT now(), d bigint DEFAULT 999)");
25+
$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)");
2626

2727
# Setup logical replication
2828
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -52,8 +52,8 @@
5252
$node_publisher->wait_for_catchup($appname);
5353

5454
$result =
55-
$node_subscriber->safe_psql('postgres', "SELECT count(*), count(c), count(d = 999) FROM test_tab");
56-
is($result, qq(2|2|2), 'check extra columns contain local defaults');
55+
$node_subscriber->safe_psql('postgres', "SELECT count(*), count(c), count(d = 999), count(e) FROM test_tab");
56+
is($result, qq(2|2|2|2), 'check extra columns contain local defaults after copy');
5757

5858
# Change the local values of the extra columns on the subscriber,
5959
# update publisher, and check that subscriber retains the expected
@@ -67,5 +67,15 @@
6767
$node_subscriber->safe_psql('postgres', "SELECT count(*), count(extract(epoch from c) = 987654321), count(d = 999) FROM test_tab");
6868
is($result, qq(2|2|2), 'check extra columns contain locally changed data');
6969

70+
# Another insert
71+
$node_publisher->safe_psql('postgres',
72+
"INSERT INTO test_tab VALUES (3, 'baz')");
73+
74+
$node_publisher->wait_for_catchup($appname);
75+
76+
$result =
77+
$node_subscriber->safe_psql('postgres', "SELECT count(*), count(c), count(d = 999), count(e) FROM test_tab");
78+
is($result, qq(3|3|3|3), 'check extra columns contain local defaults after apply');
79+
7080
$node_subscriber->stop;
7181
$node_publisher->stop;

0 commit comments

Comments
 (0)