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

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