Horton

Thread

Commits

  1. Simplify psql \d's rule for ordering the indexes of a table.

  2. Purely-cosmetic adjustments in tablecmds.c.

  3. Further fix ALTER COLUMN TYPE's handling of indexes and index constraints.

  4. Fix ALTER COLUMN TYPE failure with a partial exclusion constraint.

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

    The Post Office <noreply@postgresql.org> — 2019-06-20T20:14:29Z

    The following bug has been logged on the website:

Bug reference:      15865
Logged by:          Keith Fiske
Email address:      keith.fiske@crunchydata.com
PostgreSQL version: 11.4
Operating system:   CentOS7
Description:        

When testing the setup of our monitoring platform, we started running into
an error when using PostgreSQL as a backend for Grafana. We narrowed down
the issue to only occurring with the latest point release of PG,
specifically 11.4, 10.9 and 9.6.14 (previous major versions were not tested
at this time). The issue can be recreated by following the setup steps for
Grafana with a PG backend and it will occur when the Grafana service is
started for the first time and it tries to set up its schema in PG. We do
not see the error occurring with the previous minor versions (11.3, 10.8,
9.6.13).

A standalone, reproducible use-case is as follows. The final, ALTER TABLE
statement (which is generated by Grafana) will cause the error:

-----------------------------
ERROR:  relation "UQE_user_login" already exists
-----------------------------

However if each ALTER COLUMN statement is run independently, it seems to
work fine.

-----------------------------
CREATE TABLE public."user" (
    id integer NOT NULL,
    version integer NOT NULL,
    login character varying(190) NOT NULL,
    email character varying(190) NOT NULL,
    name character varying(255),
    password character varying(255),
    salt character varying(50),
    rands character varying(50),
    company character varying(255),
    org_id bigint NOT NULL,
    is_admin boolean NOT NULL,
    email_verified boolean,
    theme character varying(255),
    created timestamp without time zone NOT NULL,
    updated timestamp without time zone NOT NULL,
    help_flags1 bigint DEFAULT 0 NOT NULL
);

CREATE SEQUENCE public.user_id_seq1
    AS integer
    START WITH 1
    INCREMENT BY 1
    NO MINVALUE
    NO MAXVALUE
    CACHE 1;

ALTER TABLE ONLY public."user" ALTER COLUMN id SET DEFAULT
nextval('public.user_id_seq1'::regclass);

SELECT pg_catalog.setval('public.user_id_seq1', 1, false);

ALTER TABLE ONLY public."user" ADD CONSTRAINT user_pkey1 PRIMARY KEY (id);

CREATE UNIQUE INDEX "UQE_user_email" ON public."user" USING btree (email);

CREATE UNIQUE INDEX "UQE_user_login" ON public."user" USING btree (login);

ALTER TABLE "user" ALTER "login" TYPE VARCHAR(190), ALTER "email" TYPE
VARCHAR(190), ALTER "name" TYPE VARCHAR(255), ALTER "password" TYPE
VARCHAR(255), ALTER "salt" TYPE VARCHAR(50), ALTER "rands" TYPE VARCHAR(50),
ALTER "company" TYPE VARCHAR(255), ALTER "theme" TYPE VARCHAR(255);
-----------------------------

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-20T20:45:05Z

    On 2019-Jun-20, PG Bug reporting form wrote:

> When testing the setup of our monitoring platform, we started running into
> an error when using PostgreSQL as a backend for Grafana. We narrowed down
> the issue to only occurring with the latest point release of PG,
> specifically 11.4, 10.9 and 9.6.14 (previous major versions were not tested
> at this time). The issue can be recreated by following the setup steps for
> Grafana with a PG backend and it will occur when the Grafana service is
> started for the first time and it tries to set up its schema in PG. We do
> not see the error occurring with the previous minor versions (11.3, 10.8,
> 9.6.13).

Confirmed.  Bisection says that

commit e76de886157b7f974d4d247908b242607cfbf043
Author:     Tom Lane <tgl@sss.pgh.pa.us>
AuthorDate: Wed Jun 12 12:29:24 2019 -0400
CommitDate: Wed Jun 12 12:29:39 2019 -0400

    Fix ALTER COLUMN TYPE failure with a partial exclusion constraint.

is the culprit.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-20T21:08:29Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jun-20, PG Bug reporting form wrote:
>> When testing the setup of our monitoring platform, we started running into
>> an error when using PostgreSQL as a backend for Grafana.

> commit e76de886157b7f974d4d247908b242607cfbf043
> Author:     Tom Lane <tgl@sss.pgh.pa.us>
> AuthorDate: Wed Jun 12 12:29:24 2019 -0400
> CommitDate: Wed Jun 12 12:29:39 2019 -0400
>     Fix ALTER COLUMN TYPE failure with a partial exclusion constraint.

Yeah, obviously I fat-fingered something there.  Looking ...

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-21T00:20:55Z

    I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> commit e76de886157b7f974d4d247908b242607cfbf043
>> Author:     Tom Lane <tgl@sss.pgh.pa.us>
>> AuthorDate: Wed Jun 12 12:29:24 2019 -0400
>> CommitDate: Wed Jun 12 12:29:39 2019 -0400
>> Fix ALTER COLUMN TYPE failure with a partial exclusion constraint.

> Yeah, obviously I fat-fingered something there.  Looking ...

Sigh ... so the answer is that I added the cleanup code (lines
10831..10864 in HEAD) in the wrong place.  Putting it in
ATExecAlterColumnType is wrong because that gets executed potentially
multiple times per ALTER command, but I'd coded the cleanup assuming
that it would run only once.  So we can end up with duplicate entries
in the changedIndexDefs list.

The right place to put it is in ATPostAlterTypeCleanup, of course.
(I think we could eliminate the changedIndexDefs list altogether and
just build the index-defining commands in the loop that uses them.)

This is a pretty embarrassing bug, reinforcing my hindsight view
that I was firing on very few cylinders last week.  It basically
means that any ALTER TABLE that tries to alter the type of more than
one column is going to fail, if any but the last such column has a
dependent plain (non-constraint) index.  The test cases added by
e76de8861 were oh so close to noticing that, but not close enough.

I'll go fix it, but do we need to consider a near-term re-release?

			regards, tom lane

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

    Michael Paquier <michael@paquier.xyz> — 2019-06-21T00:45:54Z

    On Thu, Jun 20, 2019 at 08:20:55PM -0400, Tom Lane wrote:
> This is a pretty embarrassing bug, reinforcing my hindsight view
> that I was firing on very few cylinders last week.  It basically
> means that any ALTER TABLE that tries to alter the type of more than
> one column is going to fail, if any but the last such column has a
> dependent plain (non-constraint) index.  The test cases added by
> e76de8861 were oh so close to noticing that, but not close enough.
> 
> I'll go fix it, but do we need to consider a near-term re-release?

Ugh.  That's a possibility.  Changing each ALTER TABLE to be run
individually can be a pain, and we really ought to push for the fix of
the most recent CVE as soon as possible :(
--
Michael

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-21T01:54:46Z

    I wrote:
>> Yeah, obviously I fat-fingered something there.  Looking ...

After further review it seems like I was led into this error by a siren
singing something about how we could skip collecting the index definition
string for an index we were going to ignore later.  (Cue standard lecture
about premature optimization...)  That absolutely *does not* work, because
we might not find out till we're considering some later ALTER TYPE
subcommand that the index depends on a relevant constraint.  And we have
to capture the index definition before we alter the type of any column it
depends on, or pg_get_indexdef_string will get very confused.  That little
dependency wasn't documented anywhere.  I also found a pre-existing
comment that contradicted the new reality but I'd missed removing in
e76de8861.

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.
It might be easier to review the code changes by just ignoring e76de8861
and diffing against tablecmds.c from before that, as I've done in the
second attachment.

BTW, has anyone got an explanation for the order in which psql is
listing the indexes of "anothertab" in this test case?

			regards, tom lane

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

    Keith Fiske <keith.fiske@crunchydata.com> — 2019-06-21T14:10:27Z

    On Thu, Jun 20, 2019 at 9:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> >> Yeah, obviously I fat-fingered something there.  Looking ...
>
> After further review it seems like I was led into this error by a siren
> singing something about how we could skip collecting the index definition
> string for an index we were going to ignore later.  (Cue standard lecture
> about premature optimization...)  That absolutely *does not* work, because
> we might not find out till we're considering some later ALTER TYPE
> subcommand that the index depends on a relevant constraint.  And we have
> to capture the index definition before we alter the type of any column it
> depends on, or pg_get_indexdef_string will get very confused.  That little
> dependency wasn't documented anywhere.  I also found a pre-existing
> comment that contradicted the new reality but I'd missed removing in
> e76de8861.
>
> 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.
> It might be easier to review the code changes by just ignoring e76de8861
> and diffing against tablecmds.c from before that, as I've done in the
> second attachment.
>
> BTW, has anyone got an explanation for the order in which psql is
> listing the indexes of "anothertab" in this test case?
>
>                         regards, tom lane
>
>

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!

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-21T16:12:00Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-21T16:47:55Z

    I wrote:
> BTW, has anyone got an explanation for the order in which psql is
> listing the indexes of "anothertab" in this test case?

Ah, here's the explanation: describe.c is sorting the indexes
with this:

	"ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname;"

I can see the point of putting the pkey first, I guess, but the preference
for uniques second seems pretty bizarre, especially since
(a) it doesn't distinguish unique constraints from plain unique indexes and
(b) there's no similar preference for exclusion constraints, even though
those might be morally equivalent to a unique constraint.

What do people think of dropping the indisunique sort column here?
Obviously not back-patch material, but it might be more sensible
behavior going forward.

			regards, tom lane

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

    Keith Fiske <keith.fiske@crunchydata.com> — 2019-06-21T17:06:19Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-21T17:27:47Z

    Keith Fiske <keith.fiske@crunchydata.com> writes:
> On Fri, Jun 21, 2019 at 12:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As before, I attach a patch against HEAD, plus one that assumes e76de8861
>> has been reverted first, which is likely easier to review.

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

Again, thanks for testing!

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-24T19:05:30Z

    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

  13. Weird index ordering in psql's \d (was Re: BUG #15865: ALTER TABLE statements causing "relation already exists" errors when some indexes exist)

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-25T15:02:09Z

    I wrote:
>> BTW, has anyone got an explanation for the order in which psql is
>> listing the indexes of "anothertab" in this test case?

> Ah, here's the explanation: describe.c is sorting the indexes
> with this:
> 	"ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname;"
> I can see the point of putting the pkey first, I guess, but the preference
> for uniques second seems pretty bizarre, especially since
> (a) it doesn't distinguish unique constraints from plain unique indexes and
> (b) there's no similar preference for exclusion constraints, even though
> those might be morally equivalent to a unique constraint.
> What do people think of dropping the indisunique sort column here?
> Obviously not back-patch material, but it might be more sensible
> behavior going forward.

Here's a proposed patch that does this.  The changes it causes in the
existing regression test results seem to be sufficient illustration,
so I didn't add new tests.  There is of course no documentation
touching on this point ...

With the patch, psql's rule for listing indexes is "pkey first, then
everything else in name order".  The traditional rule is basically
crazytown IMO when you consider mixes of unique constraints and plain
(non-constraint-syntax) indexes and exclusion constraints.

A different idea that might make it slightly less crazytown is to
include exclusion constraints in the secondary preference group, along
the lines of
"ORDER BY i.indisprimary DESC, i.indisunique|i.indisexclusion DESC, c2.relname;"
This'd restore what I think was the original design intention, that
the secondary preference group includes all indexes that impose
constraints on what the table can hold.  But this'd be doubling down
on what I think is fundamentally not a very good idea, so I didn't
pursue it.

Alternatively we could go further and drop the pkey preference too,
making it pure index name order, but I don't feel a need to do that.

Thoughts?

			regards, tom lane