Re: BUG #15865: ALTER TABLE statements causing "relation already exists" errors when some indexes exist

On Fri, Jun 21, 2019 at 12:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Keith Fiske <keith.fiske@crunchydata.com> writes:
> > On Thu, Jun 20, 2019 at 9:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Here's a patch against HEAD --- since I'm feeling more mortal than usual
> >> right now, I'll put this out for review rather than just pushing it.
>
> > Can't really provide a thorough code review, but I did apply the patch to
> > the base 11.4 code (not HEAD from github) and the compound ALTER table
> > statement that was failing before now works without error. Thank you for
> > the quick fix!
>
> Thanks for testing!  However, I had a nagging feeling that I was still
> missing something, and this morning I realized what.  The proposed
> patch basically changes ATExecAlterColumnType's assumptions from
> "no constraint index will have any direct dependencies on table columns"
> to "if a constraint index has a direct dependency on a table column,
> so will its constraint".  This is easily shown to not be the case:
>
> regression=# create table foo (f1 int, f2 int);
> CREATE TABLE
> regression=# alter table foo add exclude using btree (f1 with =) where (f2
> > 0);
> ALTER TABLE
> regression=# select pg_describe_object(classid,objid,objsubid) as obj,
> pg_describe_object(refclassid,refobjid,refobjsubid) as ref, deptype from
> pg_depend where objid >= 'foo'::regclass or refobjid >= 'foo'::regclass;
>                  obj                 |                 ref
>  | deptype
>
> -------------------------------------+-------------------------------------+---------
>  type foo                            | table foo
>  | i
>  type foo[]                          | type foo
> | i
>  table foo                           | schema public
>  | n
>  constraint foo_f1_excl on table foo | column f1 of table foo
> | a
>  index foo_f1_excl                   | constraint foo_f1_excl on table foo
> | i
>  index foo_f1_excl                   | column f2 of table foo
> | a
> (6 rows)
>
> Notice that the index has a dependency on column f2 but the constraint
> doesn't.  So if we change (just) f2, ATExecAlterColumnType never notices
> the constraint at all, and kaboom:
>
> regression=# alter table foo alter column f2 type bigint;
> ERROR:  cannot drop index foo_f1_excl because constraint foo_f1_excl on
> table foo requires it
> HINT:  You can drop constraint foo_f1_excl on table foo instead.
>
> This is the same with or without yesterday's patch, and while I didn't
> trouble to verify it, I'm quite sure pre-e76de8861 would fail the same.
>
> I'm not exactly convinced that this dependency structure is a Good Thing,
> but in any case we don't get to rethink it in released branches.  So
> we need to make ATExecAlterColumnType cope, and the way to do that seems
> to be to do the get_index_constraint check in that function not later on.
>
> In principle this might lead to a few more duplicative
> get_index_constraint calls than before, because if a constraint index has
> multiple column dependencies we'll have to repeat get_index_constraint for
> each one.  But I hardly think that case is worth stressing about the
> performance of, given it never worked at all before this month.
>
> 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.
>
>                         regards, tom lane
>
>

Tested applying the patch against HEAD this time. Combined ALTER TABLE
again works without issue.

-- 
Keith Fiske
Senior Database Engineer
Crunchy Data - http://crunchydata.com