From: Japin Li <japinli@hotmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: holly.roberts@starlingbank.com
Cc: peter@eisentraut.org
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17409: Unable to alter data type of clustered column which is referenced by foreign key
Date: Fri, 11 Mar 2022 21:18:14 +0800
Message-ID: <MEYP282MB1669CD6C93FD1DCA85CAB2C1B60C9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <755474.1646959065@sss.pgh.pa.us>
References: <17409-52871dda8b5741cb@postgresql.org>
<MEYP282MB16691B29F4FEA0CC45CD5FFAB6369@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
<ME3P282MB1667E2B286DA22A16FD8A8FFB6369@ME3P282MB1667.AUSP282.PROD.OUTLOOK.COM>
<ME3P282MB1667F5A392BB4F26E6B0D82AB6369@ME3P282MB1667.AUSP282.PROD.OUTLOOK.COM>
<Yg8gGrJebITuPKu8@paquier.xyz>
<MEYP282MB1669405BB9B46FBED34EB309B6379@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
<755474.1646959065@sss.pgh.pa.us>
On Fri, 11 Mar 2022 at 08:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.
>
Thanks for your review. Agreed.
> 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".
My apologies! How about s/parent/atref/g and s/child/attmp/g ?
Attached v4 patch, please consider this for futher review.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Attachments:v4-fix-alter-data-type-of-clustered-column.patch (text/x-patch, 4.8K) downloaddiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 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.outindex 16e0475663..bde3f14dfa 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 atref (p1 int constraint pk_atref primary key);+create table attbl (c1 int, constraint fk_atref foreign key (c1) references atref(p1));+cluster atref using pk_atref;+alter table atref alter column p1 set data type bigint;+drop table attbl;+drop table atref;+create table atref (p1 int constraint pk_atref primary key);+alter table atref replica identity using index pk_atref;+create table attbl (c1 int, constraint fk_attbl foreign key (c1) references atref(p1));+alter table atref alter column p1 set data type bigint;+drop table attbl;+drop table atref;diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sqlindex ac894c0602..1cbcce490c 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 atref (p1 int constraint pk_atref primary key);+create table attbl (c1 int, constraint fk_atref foreign key (c1) references atref(p1));+cluster atref using pk_atref;+alter table atref alter column p1 set data type bigint;+drop table attbl;+drop table atref;++create table atref (p1 int constraint pk_atref primary key);+alter table atref replica identity using index pk_atref;+create table attbl (c1 int, constraint fk_attbl foreign key (c1) references atref(p1));+alter table atref alter column p1 set data type bigint;+drop table attbl;+drop table atref;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: japinli@hotmail.com, tgl@sss.pgh.pa.us, michael@paquier.xyz, holly.roberts@starlingbank.com, peter@eisentraut.org, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17409: Unable to alter data type of clustered column which is referenced by foreign key
In-Reply-To: <MEYP282MB1669CD6C93FD1DCA85CAB2C1B60C9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox