From c67215912e973f97f91b3fa19b111a4a0696e7ea Mon Sep 17 00:00:00 2001 From: Fujii Masao Date: Thu, 17 Sep 2026 21:35:15 +0900 Subject: [PATCH v1] Fix ADD PRIMARY KEY after dropping and re-adding a column Previously, dropping a NOT NULL column, adding a new column with the same name, and making it a primary key in a single ALTER TABLE command could fail with "primary key column ... is not marked NOT NULL". This happened because ALTER TABLE checked for an existing NOT NULL constraint before dropping the old column. Finding the constraint caused it to skip adding one for the new column, even though the old constraint would be removed with the old column. Defer this check until after column drops and additions have been executed, so that it examines the columns that will actually form the primary key. Add any required NOT NULL constraints before creating the primary-key index. Backpatch to v18, where this issue was introduced by commit a379061a22a. --- src/backend/commands/tablecmds.c | 21 +++++++++++---------- src/test/regress/expected/alter_table.out | 4 ++++ src/test/regress/sql/alter_table.sql | 5 +++++ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 2f073ddb84a..99384951864 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -555,9 +555,8 @@ static ObjectAddress ATExecDropColumn(List **wqueue, Relation rel, const char *c bool recurse, bool recursing, bool missing_ok, LOCKMODE lockmode, ObjectAddresses *addrs); -static void ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, - bool recurse, LOCKMODE lockmode, - AlterTableUtilityContext *context); +static void ATPrepAddPrimaryKey(AlteredTableInfo *tab, Relation rel, + AlterTableCmd *cmd, LOCKMODE lockmode); static void verifyNotNullPKCompatible(HeapTuple tuple, const char *colname); static ObjectAddress ATExecAddIndex(AlteredTableInfo *tab, Relation rel, IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode); @@ -5146,7 +5145,6 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_AddConstraint: /* ADD CONSTRAINT */ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE); - ATPrepAddPrimaryKey(wqueue, rel, cmd, recurse, lockmode, context); if (recurse) { /* recurses at exec time; lock descendants and set flag */ @@ -5518,9 +5516,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, case AT_AddConstraint: /* ADD CONSTRAINT */ /* Transform the command only during initial examination */ if (cur_pass == AT_PASS_ADD_CONSTR) + { + ATPrepAddPrimaryKey(tab, rel, cmd, lockmode); cmd = ATParseTransformCmd(wqueue, tab, rel, cmd, cmd->recurse, lockmode, cur_pass, context); + } /* Depending on constraint type, might be no more work to do now */ if (cmd != NULL) address = @@ -9552,7 +9553,7 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, * hierarchy (failing to ensure that leads to funny corner cases). For the * normal case where we're asked to recurse, this routine checks if the * not-null constraints exist already, and if not queues a requirement for - * them to be created by phase 2. + * them to be created in the AT_PASS_COL_ATTRS pass. * * For the case where we're asked not to recurse, we verify that a not-null * constraint exists on each column of each (direct) child table, throwing an @@ -9567,9 +9568,8 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, * deadlocks during parallel pg_restore of PKs on partitioned tables. */ static void -ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, - bool recurse, LOCKMODE lockmode, - AlterTableUtilityContext *context) +ATPrepAddPrimaryKey(AlteredTableInfo *tab, Relation rel, AlterTableCmd *cmd, + LOCKMODE lockmode) { Constraint *pkconstr; List *children = NIL; @@ -9600,7 +9600,7 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, heap_freetuple(tuple); continue; } - else if (!recurse) + else if (!cmd->recurse) { /* * No constraint on this column. Asked not to recurse, we won't @@ -9637,7 +9637,8 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, newcmd->recurse = true; newcmd->def = (Node *) nnconstr; - ATPrepCmd(wqueue, rel, newcmd, true, false, lockmode, context); + tab->subcmds[AT_PASS_COL_ATTRS] = + lappend(tab->subcmds[AT_PASS_COL_ATTRS], newcmd); } } diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index e167a41ce79..5528749b953 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1046,6 +1046,10 @@ ERROR: column "test2" of relation "atacc1" contains null values -- now add a primary key column with a default (succeeds). alter table atacc1 add column test2 int default 0 primary key; drop table atacc1; +-- Replacing a NOT NULL column must create a new constraint for the primary key. +create table atacc1 (id int not null); +alter table atacc1 drop column id, add column id int, add primary key (id); +drop table atacc1; -- this combination used to have order-of-execution problems (bug #15580) create table atacc1 (a int); insert into atacc1 values(1); diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 9f6c2a4bb08..6e28b5539eb 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -782,6 +782,11 @@ alter table atacc1 add column test2 int primary key; alter table atacc1 add column test2 int default 0 primary key; drop table atacc1; +-- Replacing a NOT NULL column must create a new constraint for the primary key. +create table atacc1 (id int not null); +alter table atacc1 drop column id, add column id int, add primary key (id); +drop table atacc1; + -- this combination used to have order-of-execution problems (bug #15580) create table atacc1 (a int); insert into atacc1 values(1); -- 2.55.0