Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hfUHa-0000g4-I7 for pgsql-bugs@arkaria.postgresql.org; Mon, 24 Jun 2019 19:05:38 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hfUHZ-00037c-Bw for pgsql-bugs@arkaria.postgresql.org; Mon, 24 Jun 2019 19:05:37 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hfUHZ-00036l-4D for pgsql-bugs@lists.postgresql.org; Mon, 24 Jun 2019 19:05:37 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hfUHW-0005Yr-RG for pgsql-bugs@lists.postgresql.org; Mon, 24 Jun 2019 19:05:36 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id x5OJ5UEM014788; Mon, 24 Jun 2019 15:05:30 -0400 From: Tom Lane To: Keith Fiske cc: Alvaro Herrera , pgsql-bugs@lists.postgresql.org Subject: Re: BUG #15865: ALTER TABLE statements causing "relation already exists" errors when some indexes exist In-reply-to: <19496.1561133520@sss.pgh.pa.us> References: <20190620204505.GA22934@alvherre.pgsql> <25354.1561064909@sss.pgh.pa.us> <905.1561076455@sss.pgh.pa.us> <28827.1561082086@sss.pgh.pa.us> <19496.1561133520@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Fri, 21 Jun 2019 12:12:00 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <14744.1561403059.0@sss.pgh.pa.us> Date: Mon, 24 Jun 2019 15:05:30 -0400 Message-ID: <14787.1561403130@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <14744.1561403059.1@sss.pgh.pa.us> I wrote: > As before, I attach a patch against HEAD, plus one that assumes e76de8861 > has been reverted first, which is likely easier to review. > Unlike yesterday, I'm feeling pretty good about this patch now, but it > still wouldn't hurt for somebody else to go over it. I started to back-patch this, and soon noticed that the content of the OCLASS_CONSTRAINT case branch in ATExecAlterColumnType has varied across versions, which makes copy-and-pasting it seem pretty hazardous. Hence it seems prudent to do slightly more work and split that code out into a subroutine rather than having two copies. As attached, which is a hopefully-final patch for HEAD. As before, it presumes reversion of e76de8861, because it's a lot easier to see what's going on that way. BTW ... while working on this, I got annoyed by the fact that ATExecAlterColumnGenericOptions was inserted, no doubt with the aid of a dartboard, into the middle of a large group of AlterColumnType-related functions. Would anyone mind a separate patch to relocate it down past those, probably just before ATExecChangeOwner? regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="alter-type-final-fix-HEAD.patch"; charset="us-ascii" Content-ID: <14744.1561403059.2@sss.pgh.pa.us> Content-Description: alter-type-final-fix-HEAD.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/table= cmds.c index 98519ef..7d62a0f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -456,6 +456,8 @@ static void ATPrepAlterColumnType(List **wqueue, static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno= ); static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relatio= n rel, AlterTableCmd *cmd, LOCKMODE lockmode); +static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo = *tab); +static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)= ; static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const = char *colName, List *options, LOCKMODE lockmode); static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, @@ -10600,11 +10602,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Rela= tion rel, * performed all the individual ALTER TYPE operations. We have to save * the info before executing ALTER TYPE, though, else the deparser will * get confused. - * - * There could be multiple entries for the same object, so we must check - * to ensure we process each one only once. Note: we assume that an ind= ex - * that implements a constraint will not show a direct dependency on the - * column. */ depRel =3D table_open(DependRelationId, RowExclusiveLock); = @@ -10647,13 +10644,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Rela= tion rel, relKind =3D=3D RELKIND_PARTITIONED_INDEX) { Assert(foundObject.objectSubId =3D=3D 0); - if (!list_member_oid(tab->changedIndexOids, foundObject.objectId)) - { - tab->changedIndexOids =3D lappend_oid(tab->changedIndexOids, - foundObject.objectId); - tab->changedIndexDefs =3D lappend(tab->changedIndexDefs, - pg_get_indexdef_string(foundObject.objectId)); - } + RememberIndexForRebuilding(foundObject.objectId, tab); } else if (relKind =3D=3D RELKIND_SEQUENCE) { @@ -10689,18 +10680,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Rela= tion rel, = case OCLASS_CONSTRAINT: Assert(foundObject.objectSubId =3D=3D 0); - if (!list_member_oid(tab->changedConstraintOids, - foundObject.objectId)) - { - char *defstring =3D pg_get_constraintdef_command(foundObject.obje= ctId); - - tab->changedConstraintOids =3D - lappend_oid(tab->changedConstraintOids, - foundObject.objectId); - tab->changedConstraintDefs =3D - lappend(tab->changedConstraintDefs, - defstring); - } + RememberConstraintForRebuilding(foundObject.objectId, tab); break; = case OCLASS_REWRITE: @@ -11000,6 +10980,75 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Rela= tion rel, } = /* + * Subroutine for ATExecAlterColumnType: remember that a constraint needs + * to be rebuilt (which we might already know). + */ +static void +RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab) +{ + /* + * This de-duplication check is critical for two independent reasons: we + * mustn't try to recreate the same constraint twice, and if a constrain= t + * depends on more than one column whose type is to be altered, we must + * capture its definition string before applying any of the column type + * changes. ruleutils.c will get confused if we ask again later. + */ + if (!list_member_oid(tab->changedConstraintOids, conoid)) + { + /* OK, capture the constraint's existing definition string */ + char *defstring =3D pg_get_constraintdef_command(conoid); + + tab->changedConstraintOids =3D lappend_oid(tab->changedConstraintOids, + conoid); + tab->changedConstraintDefs =3D lappend(tab->changedConstraintDefs, + defstring); + } +} + +/* + * Subroutine for ATExecAlterColumnType: remember that an index needs + * to be rebuilt (which we might already know). + */ +static void +RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab) +{ + /* + * This de-duplication check is critical for two independent reasons: we + * mustn't try to recreate the same index twice, and if an index depends + * on more than one column whose type is to be altered, we must capture + * its definition string before applying any of the column type changes. + * ruleutils.c will get confused if we ask again later. + */ + if (!list_member_oid(tab->changedIndexOids, indoid)) + { + /* + * Before adding it as an index-to-rebuild, we'd better see if it + * belongs to a constraint, and if so rebuild the constraint instead. + * Typically this check fails, because constraint indexes normally + * have only dependencies on their constraint. But it's possible for + * such an index to also have direct dependencies on table columns, + * for example with a partial exclusion constraint. + */ + Oid conoid =3D get_index_constraint(indoid); + + if (OidIsValid(conoid)) + { + RememberConstraintForRebuilding(conoid, tab); + } + else + { + /* OK, capture the index's existing definition string */ + char *defstring =3D pg_get_indexdef_string(indoid); + + tab->changedIndexOids =3D lappend_oid(tab->changedIndexOids, + indoid); + tab->changedIndexDefs =3D lappend(tab->changedIndexDefs, + defstring); + } + } +} + +/* * Returns the address of the modified column */ static ObjectAddress diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/= expected/alter_table.out index cedc7c6..a639601 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1900,6 +1900,64 @@ select * from anothertab; (3 rows) = drop table anothertab; +-- Test index handling in alter table column type (cf. bugs #15835, #1586= 5) +create table anothertab(f1 int primary key, f2 int unique, + f3 int, f4 int, f5 int); +alter table anothertab + add exclude using btree (f3 with =3D); +alter table anothertab + add exclude using btree (f4 with =3D) where (f4 is not null); +alter table anothertab + add exclude using btree (f4 with =3D) where (f5 > 0); +alter table anothertab + add unique(f1,f4); +create index on anothertab(f2,f3); +create unique index on anothertab(f4); +\d anothertab + Table "public.anothertab" + Column | Type | Collation | Nullable | Default = +--------+---------+-----------+----------+--------- + f1 | integer | | not null | = + f2 | integer | | | = + f3 | integer | | | = + f4 | integer | | | = + f5 | integer | | | = +Indexes: + "anothertab_pkey" PRIMARY KEY, btree (f1) + "anothertab_f1_f4_key" UNIQUE CONSTRAINT, btree (f1, f4) + "anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2) + "anothertab_f4_idx" UNIQUE, btree (f4) + "anothertab_f2_f3_idx" btree (f2, f3) + "anothertab_f3_excl" EXCLUDE USING btree (f3 WITH =3D) + "anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =3D) WHERE (f4 IS N= OT NULL) + "anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =3D) WHERE (f5 > 0= ) + +alter table anothertab alter column f1 type bigint; +alter table anothertab + alter column f2 type bigint, + alter column f3 type bigint, + alter column f4 type bigint; +alter table anothertab alter column f5 type bigint; +\d anothertab + Table "public.anothertab" + Column | Type | Collation | Nullable | Default = +--------+--------+-----------+----------+--------- + f1 | bigint | | not null | = + f2 | bigint | | | = + f3 | bigint | | | = + f4 | bigint | | | = + f5 | bigint | | | = +Indexes: + "anothertab_pkey" PRIMARY KEY, btree (f1) + "anothertab_f1_f4_key" UNIQUE CONSTRAINT, btree (f1, f4) + "anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2) + "anothertab_f4_idx" UNIQUE, btree (f4) + "anothertab_f2_f3_idx" btree (f2, f3) + "anothertab_f3_excl" EXCLUDE USING btree (f3 WITH =3D) + "anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =3D) WHERE (f4 IS N= OT NULL) + "anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =3D) WHERE (f5 > 0= ) + +drop table anothertab; create table another (f1 int, f2 text); insert into another values(1, 'one'); insert into another values(2, 'two'); diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/a= lter_table.sql index e8e094a..0369909 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1317,6 +1317,31 @@ select * from anothertab; = drop table anothertab; = +-- Test index handling in alter table column type (cf. bugs #15835, #1586= 5) +create table anothertab(f1 int primary key, f2 int unique, + f3 int, f4 int, f5 int); +alter table anothertab + add exclude using btree (f3 with =3D); +alter table anothertab + add exclude using btree (f4 with =3D) where (f4 is not null); +alter table anothertab + add exclude using btree (f4 with =3D) where (f5 > 0); +alter table anothertab + add unique(f1,f4); +create index on anothertab(f2,f3); +create unique index on anothertab(f4); + +\d anothertab +alter table anothertab alter column f1 type bigint; +alter table anothertab + alter column f2 type bigint, + alter column f3 type bigint, + alter column f4 type bigint; +alter table anothertab alter column f5 type bigint; +\d anothertab + +drop table anothertab; + create table another (f1 int, f2 text); = insert into another values(1, 'one'); ------- =_aaaaaaaaaa0--