From: Tom Lane <tgl@sss.pgh.pa.us>
To: Keith Fiske <keith.fiske@crunchydata.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15865: ALTER TABLE statements causing "relation already exists" errors when some indexes exist
Date: Mon, 24 Jun 2019 15:05:30 -0400
Message-ID: <14787.1561403130@sss.pgh.pa.us> (raw)
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>
<CAODZiv70xfnh75hNmY2hn7+ySxd1=QuQW5gfz6vFm4JvVfihwg@mail.gmail.com>
<19496.1561133520@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
Attachments:
[text/x-diff] alter-type-final-fix-HEAD.patch (9.3K, ../14787.1561403130@sss.pgh.pa.us/2-alter-type-final-fix-HEAD.patch)
download | inline diff:diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 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, Relation 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, Relation 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 index- * that implements a constraint will not show a direct dependency on the- * column.
*/
depRel = table_open(DependRelationId, RowExclusiveLock);
@@ -10647,13 +10644,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
relKind == RELKIND_PARTITIONED_INDEX)
{
Assert(foundObject.objectSubId == 0);
- if (!list_member_oid(tab->changedIndexOids, foundObject.objectId))- {- tab->changedIndexOids = lappend_oid(tab->changedIndexOids,- foundObject.objectId);- tab->changedIndexDefs = lappend(tab->changedIndexDefs,- pg_get_indexdef_string(foundObject.objectId));- }+ RememberIndexForRebuilding(foundObject.objectId, tab);
}
else if (relKind == RELKIND_SEQUENCE)
{
@@ -10689,18 +10680,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
case OCLASS_CONSTRAINT:
Assert(foundObject.objectSubId == 0);
- if (!list_member_oid(tab->changedConstraintOids,- foundObject.objectId))- {- char *defstring = pg_get_constraintdef_command(foundObject.objectId);-- tab->changedConstraintOids =- lappend_oid(tab->changedConstraintOids,- foundObject.objectId);- tab->changedConstraintDefs =- lappend(tab->changedConstraintDefs,- defstring);- }+ RememberConstraintForRebuilding(foundObject.objectId, tab);
break;
case OCLASS_REWRITE:
@@ -11000,6 +10980,75 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation 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 constraint+ * 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 = pg_get_constraintdef_command(conoid);++ tab->changedConstraintOids = lappend_oid(tab->changedConstraintOids,+ conoid);+ tab->changedConstraintDefs = 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 = get_index_constraint(indoid);++ if (OidIsValid(conoid))+ {+ RememberConstraintForRebuilding(conoid, tab);+ }+ else+ {+ /* OK, capture the index's existing definition string */+ char *defstring = pg_get_indexdef_string(indoid);++ tab->changedIndexOids = lappend_oid(tab->changedIndexOids,+ indoid);+ tab->changedIndexDefs = 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.outindex 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, #15865)+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 =);+alter table anothertab+ add exclude using btree (f4 with =) where (f4 is not null);+alter table anothertab+ add exclude using btree (f4 with =) 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 =)+ "anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =) WHERE (f4 IS NOT NULL)+ "anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =) 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 =)+ "anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =) WHERE (f4 IS NOT NULL)+ "anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =) 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/alter_table.sqlindex 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, #15865)+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 =);+alter table anothertab+ add exclude using btree (f4 with =) where (f4 is not null);+alter table anothertab+ add exclude using btree (f4 with =) 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');
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: tgl@sss.pgh.pa.us, keith.fiske@crunchydata.com, alvherre@2ndquadrant.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15865: ALTER TABLE statements causing "relation already exists" errors when some indexes exist
In-Reply-To: <14787.1561403130@sss.pgh.pa.us>
* 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