Re: BUG #17409: Unable to alter data type of clustered column which is referenced by foreign key - Mailing list pgsql-bugs

Japin Li <japinli@hotmail.com> writes:
> Attach a new patch to fix the replica identify case, also add test cases.

Now that we realize we need to de-duplicate, it seems to me we should
postpone the get_rel_name() calls so that we don't have to do that
work repeatedly; as attached.

Also, while I've not done anything about it here, the proposed test
cases seem remarkably cavalier about their choices of test table
names.  If you want to use names as generic as "parent" and "child",
they'd better be temp tables to avoid risk of conflict against other
concurrent regression tests.  But most of alter_table.sql prefers
to use names starting with "at".

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dc5872f988..19c924b7df 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -189,8 +189,8 @@ typedef struct AlteredTableInfo
     List       *changedConstraintDefs;    /* string definitions of same */
     List       *changedIndexOids;    /* OIDs of indexes to rebuild */
     List       *changedIndexDefs;    /* string definitions of same */
-    char       *replicaIdentityIndex;    /* index to reset as REPLICA IDENTITY */
-    char       *clusterOnIndex; /* index to use for CLUSTER */
+    Oid            replicaIdentityIndex;    /* index to reset as REPLICA IDENTITY */
+    Oid            clusterOnIndex; /* index to use for CLUSTER */
     List       *changedStatisticsOids;    /* OIDs of statistics to rebuild */
     List       *changedStatisticsDefs;    /* string definitions of same */
 } AlteredTableInfo;
@@ -12856,10 +12856,12 @@ RememberReplicaIdentityForRebuilding(Oid indoid, AlteredTableInfo *tab)
     if (!get_index_isreplident(indoid))
         return;

-    if (tab->replicaIdentityIndex)
+    /* We must de-duplicate multiple requests */
+    if (OidIsValid(tab->replicaIdentityIndex) &&
+        tab->replicaIdentityIndex != indoid)
         elog(ERROR, "relation %u has multiple indexes marked as replica identity", tab->relid);

-    tab->replicaIdentityIndex = get_rel_name(indoid);
+    tab->replicaIdentityIndex = indoid;
 }

 /*
@@ -12871,10 +12873,12 @@ RememberClusterOnForRebuilding(Oid indoid, AlteredTableInfo *tab)
     if (!get_index_isclustered(indoid))
         return;

-    if (tab->clusterOnIndex)
+    /* We must de-duplicate multiple requests */
+    if (OidIsValid(tab->clusterOnIndex) &&
+        tab->clusterOnIndex != indoid)
         elog(ERROR, "relation %u has multiple clustered indexes", tab->relid);

-    tab->clusterOnIndex = get_rel_name(indoid);
+    tab->clusterOnIndex = indoid;
 }

 /*
@@ -13117,13 +13121,15 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
     /*
      * Queue up command to restore replica identity index marking
      */
-    if (tab->replicaIdentityIndex)
+    if (OidIsValid(tab->replicaIdentityIndex))
     {
         AlterTableCmd *cmd = makeNode(AlterTableCmd);
         ReplicaIdentityStmt *subcmd = makeNode(ReplicaIdentityStmt);

         subcmd->identity_type = REPLICA_IDENTITY_INDEX;
-        subcmd->name = tab->replicaIdentityIndex;
+        subcmd->name = get_rel_name(tab->replicaIdentityIndex);
+        Assert(subcmd->name);
+
         cmd->subtype = AT_ReplicaIdentity;
         cmd->def = (Node *) subcmd;

@@ -13135,12 +13141,13 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
     /*
      * Queue up command to restore marking of index used for cluster.
      */
-    if (tab->clusterOnIndex)
+    if (OidIsValid(tab->clusterOnIndex))
     {
         AlterTableCmd *cmd = makeNode(AlterTableCmd);

         cmd->subtype = AT_ClusterOn;
-        cmd->name = tab->clusterOnIndex;
+        cmd->name = get_rel_name(tab->clusterOnIndex);
+        Assert(cmd->name);

         /* do it after indexes and constraints */
         tab->subcmds[AT_PASS_OLD_CONSTR] =
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 16e0475663..53a19e38c9 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4584,3 +4584,16 @@ drop publication pub1;
 drop schema alter1 cascade;
 NOTICE:  drop cascades to table alter1.t1
 drop schema alter2 cascade;
+/* Test case for bug #17409 */
+create table parent (p1 int constraint pk_parent primary key);
+create table child (c1 int, constraint fk_child foreign key (c1) references parent(p1));
+cluster parent using pk_parent;
+alter table parent alter column p1 set data type bigint;
+drop table child;
+drop table parent;
+create table parent (p1 int constraint pk_parent primary key);
+alter table parent replica identity using index pk_parent;
+create table child (c1 int, constraint fk_child foreign key (c1) references parent(p1));
+alter table parent alter column p1 set data type bigint;
+drop table child;
+drop table parent;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index ac894c0602..38ed8a7134 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -3013,3 +3013,19 @@ alter table alter1.t1 set schema alter2; -- should fail
 drop publication pub1;
 drop schema alter1 cascade;
 drop schema alter2 cascade;
+
+/* Test case for bug #17409 */
+
+create table parent (p1 int constraint pk_parent primary key);
+create table child (c1 int, constraint fk_child foreign key (c1) references parent(p1));
+cluster parent using pk_parent;
+alter table parent alter column p1 set data type bigint;
+drop table child;
+drop table parent;
+
+create table parent (p1 int constraint pk_parent primary key);
+alter table parent replica identity using index pk_parent;
+create table child (c1 int, constraint fk_child foreign key (c1) references parent(p1));
+alter table parent alter column p1 set data type bigint;
+drop table child;
+drop table parent;