Skip to content

Commit 1597948

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 b94988f commit 1597948

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
@@ -25,7 +25,6 @@
2525
#include "access/sysattr.h"
2626
#include "access/xact.h"
2727
#include "access/xlog.h"
28-
#include "catalog/dependency.h"
2928
#include "catalog/pg_type.h"
3029
#include "commands/copy.h"
3130
#include "commands/defrem.h"
@@ -3064,19 +3063,8 @@ BeginCopyFrom(ParseState *pstate,
30643063
{
30653064
/* attribute is NOT to be copied from input */
30663065
/* use default value if one exists */
3067-
Expr *defexpr;
3068-
3069-
if (attr[attnum - 1]->attidentity)
3070-
{
3071-
NextValueExpr *nve = makeNode(NextValueExpr);
3072-
3073-
nve->seqid = getOwnedSequence(RelationGetRelid(cstate->rel),
3074-
attnum);
3075-
nve->typeId = attr[attnum - 1]->atttypid;
3076-
defexpr = (Expr *) nve;
3077-
}
3078-
else
3079-
defexpr = (Expr *) build_column_default(cstate->rel, attnum);
3066+
Expr *defexpr = (Expr *) build_column_default(cstate->rel,
3067+
attnum);
30803068

30813069
if (defexpr != NULL)
30823070
{

‎src/backend/commands/tablecmds.c

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5342,7 +5342,21 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
53425342
if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE
53435343
&& relkind != RELKIND_FOREIGN_TABLE && attribute.attnum > 0)
53445344
{
5345-
defval = (Expr *) build_column_default(rel, attribute.attnum);
5345+
/*
5346+
* For an identity column, we can't use build_column_default(),
5347+
* because the sequence ownership isn't set yet. So do it manually.
5348+
*/
5349+
if (colDef->identity)
5350+
{
5351+
NextValueExpr *nve = makeNode(NextValueExpr);
5352+
5353+
nve->seqid = RangeVarGetRelid(colDef->identitySequence, NoLock, false);
5354+
nve->typeId = typeOid;
5355+
5356+
defval = (Expr *) nve;
5357+
}
5358+
else
5359+
defval = (Expr *) build_column_default(rel, attribute.attnum);
53465360

53475361
if (!defval && DomainHasConstraints(typeOid))
53485362
{

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2812,6 +2812,7 @@ _copyColumnDef(const ColumnDef *from)
28122812
COPY_NODE_FIELD(raw_default);
28132813
COPY_NODE_FIELD(cooked_default);
28142814
COPY_SCALAR_FIELD(identity);
2815+
COPY_NODE_FIELD(identitySequence);
28152816
COPY_NODE_FIELD(collClause);
28162817
COPY_SCALAR_FIELD(collOid);
28172818
COPY_NODE_FIELD(constraints);

‎src/backend/nodes/equalfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2544,6 +2544,7 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
25442544
COMPARE_NODE_FIELD(raw_default);
25452545
COMPARE_NODE_FIELD(cooked_default);
25462546
COMPARE_SCALAR_FIELD(identity);
2547+
COMPARE_NODE_FIELD(identitySequence);
25472548
COMPARE_NODE_FIELD(collClause);
25482549
COMPARE_SCALAR_FIELD(collOid);
25492550
COMPARE_NODE_FIELD(constraints);

‎src/backend/nodes/outfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2801,6 +2801,7 @@ _outColumnDef(StringInfo str, const ColumnDef *node)
28012801
WRITE_NODE_FIELD(raw_default);
28022802
WRITE_NODE_FIELD(cooked_default);
28032803
WRITE_CHAR_FIELD(identity);
2804+
WRITE_NODE_FIELD(identitySequence);
28042805
WRITE_NODE_FIELD(collClause);
28052806
WRITE_OID_FIELD(collOid);
28062807
WRITE_NODE_FIELD(constraints);

‎src/backend/parser/parse_utilcmd.c

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

476476
cxt->blist = lappend(cxt->blist, seqstmt);
477477

478+
/*
479+
* Store the identity sequence name that we decided on. ALTER TABLE
480+
* ... ADD COLUMN ... IDENTITY needs this so that it can fill the new
481+
* column with values from the sequence, while the association of the
482+
* sequence with the table is not set until after the ALTER TABLE.
483+
*/
484+
column->identitySequence = seqstmt->sequence;
485+
478486
/*
479487
* Build an ALTER SEQUENCE ... OWNED BY command to mark the sequence as
480488
* 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

0 commit comments

Comments
 (0)