Horton

Thread

Commits

  1. Fully enforce uniqueness of constraint names.

  1. Two constraints with the same name not always allowed

    André Hänsel <andre@webkr.de> — 2018-09-02T07:31:52Z

    Hi list,
calling this a bug might be pedantic, but I noticed this inconsistency:

Case 1:

CREATE TABLE t(c integer);
ALTER TABLE t ADD CONSTRAINT foo UNIQUE(c);
ALTER TABLE t ADD CONSTRAINT foo CHECK(c > 1);

-> ERROR:  constraint "foo" for relation "t" already exists

Case 2:

CREATE TABLE t(c integer);
ALTER TABLE t ADD CONSTRAINT foo CHECK(c > 1);
ALTER TABLE t ADD CONSTRAINT foo UNIQUE(c);

-> Creates two constraints, both called "foo".

Case 3:

CREATE TABLE t(c integer);
ALTER TABLE t ADD CONSTRAINT t_c_check UNIQUE(c); -- add the UNIQUE with the
same name that the following CHECK will automatically choose
ALTER TABLE t ADD CHECK(c > 1);

-> Creates the UNIQUE constraint as "t_c_check" and the CHECK as
"t_c_check1"

Case 4:

CREATE TABLE t(c integer);
ALTER TABLE t ADD CONSTRAINT t_c_key CHECK(c > 1);
ALTER TABLE t ADD UNIQUE(c);

-> Creates two constraints, both called "t_c_key".

In cases where two constraints with the same name are created, an "ALTER
TABLE t DROP CONSTRAINT ..." drops the UNIQUE first. Issuing the ALTER TABLE
a second time then drops the CHECK.

Regards,
André

  2. Re: Two constraints with the same name not always allowed

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-02T13:22:56Z

    >>>>> "André" == André Hänsel <andre@webkr.de> writes:

 André> Case 2:

 André> CREATE TABLE t(c integer);
 André> ALTER TABLE t ADD CONSTRAINT foo CHECK(c > 1);
 André> ALTER TABLE t ADD CONSTRAINT foo UNIQUE(c);

 André>  -> Creates two constraints, both called "foo".

I'd call _that_ a bug, myself - having two constraints on a table with
the same name potentially messes up a lot of automated maintenance
operations.

 André> In cases where two constraints with the same name are created,
 André> an "ALTER TABLE t DROP CONSTRAINT ..." drops the UNIQUE first.
 André> Issuing the ALTER TABLE a second time then drops the CHECK.

I think that's purely an artifact of what order an index scan on
pg_constraint_conrelid_index finds the constraints.

-- 
Andrew (irc:RhodiumToad)

  3. Re: Two constraints with the same name not always allowed

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-02T17:00:45Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "André" == André Hänsel <andre@webkr.de> writes:
>  André> Case 2:
>  André> CREATE TABLE t(c integer);
>  André> ALTER TABLE t ADD CONSTRAINT foo CHECK(c > 1);
>  André> ALTER TABLE t ADD CONSTRAINT foo UNIQUE(c);
>  André>  -> Creates two constraints, both called "foo".

> I'd call _that_ a bug, myself - having two constraints on a table with
> the same name potentially messes up a lot of automated maintenance
> operations.

Agreed.  We must have missed a check for constraint-exists someplace.

This also points up the lack of a suitable unique index on pg_constraint.
It's sort of difficult to figure out what that should look like given that
pg_constraint contains two quasi-independent collections of constraints,
but maybe UNIQUE(conrelid,contypid,conname) would serve given the
reasonable assumption that exactly one of conrelid and contypid is zero.

Potentially we could drop pg_constraint_conrelid_index and
pg_constraint_contypid_index, replacing scans on those with
scans on this new unique index.

			regards, tom lane

  4. Re: Two constraints with the same name not always allowed

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-09-02T17:05:11Z

    On 2018-Sep-02, Tom Lane wrote:

> This also points up the lack of a suitable unique index on pg_constraint.
> It's sort of difficult to figure out what that should look like given that
> pg_constraint contains two quasi-independent collections of constraints,
> but maybe UNIQUE(conrelid,contypid,conname) would serve given the
> reasonable assumption that exactly one of conrelid and contypid is zero.

Hmm ... c.f. 7eca575d1c28.  Maybe we should split them out?  Are there
reasons to have them together at all?

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

  5. Re: Two constraints with the same name not always allowed

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-02T17:35:27Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Sep-02, Tom Lane wrote:
>> This also points up the lack of a suitable unique index on pg_constraint.
>> It's sort of difficult to figure out what that should look like given that
>> pg_constraint contains two quasi-independent collections of constraints,
>> but maybe UNIQUE(conrelid,contypid,conname) would serve given the
>> reasonable assumption that exactly one of conrelid and contypid is zero.

> Hmm ... c.f. 7eca575d1c28.  Maybe we should split them out?  Are there
> reasons to have them together at all?

Yeah, I've occasionally thought about replacing pg_constraint with
two separate catalogs; we could keep pg_constraint as a union view
to avoid breaking clients that look at it.  But that'd be kind of
a large project, whereas adjusting the set of indexes for a catalog
is a pretty simple finger exercise in most cases.  (It's also unclear
how smart the planner would be about optimizing queries on such a
view.)

Thinking about the planner angle some more, it seems like probably
the most reasonable proposal is to add UNIQUE(conrelid,contypid,conname)
replacing pg_constraint_conrelid_index, but keep
pg_constraint_contypid_index.  While we could teach relevant parts
of backend/catalog how to use such a unique index to search for the
constraints of a domain, the planner would not know how to optimize
SQL queries with "WHERE contypid = xxx" unless we keep that index.
It would figure out that "WHERE conrelid = xxx" works with the
unique index, though.

			regards, tom lane

  6. Re: Two constraints with the same name not always allowed

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-02T18:15:42Z

    I wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> "André" == André Hänsel <andre@webkr.de> writes:
>> André> Case 2:
>> André> CREATE TABLE t(c integer);
>> André> ALTER TABLE t ADD CONSTRAINT foo CHECK(c > 1);
>> André> ALTER TABLE t ADD CONSTRAINT foo UNIQUE(c);
>> André>  -> Creates two constraints, both called "foo".

>> I'd call _that_ a bug, myself - having two constraints on a table with
>> the same name potentially messes up a lot of automated maintenance
>> operations.

> Agreed.  We must have missed a check for constraint-exists someplace.

Note that if you repeat that last command, what you get is

regression=# ALTER TABLE t ADD CONSTRAINT foo UNIQUE(c);
ERROR:  relation "foo" already exists

I think the code supposes that checking for duplicate relation name
is sufficient; but of course it is not if we want a table's constraints
to have distinct names, since they may not all correspond to indexes.

I do not think we can back-patch a change here --- it might break
databases that are working satisfactorily today.  But it seems like
we could tighten this up in HEAD and maybe v11.

			regards, tom lane

  7. Re: Two constraints with the same name not always allowed

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-02T22:31:04Z

    I wrote:
> I think the code supposes that checking for duplicate relation name
> is sufficient; but of course it is not if we want a table's constraints
> to have distinct names, since they may not all correspond to indexes.
> I do not think we can back-patch a change here --- it might break
> databases that are working satisfactorily today.  But it seems like
> we could tighten this up in HEAD and maybe v11.

Attached is a draft patchset for this.

0001 replaces the existing index with a unique one and makes necessary
backend code adjustments.  Said adjustments could have been as simple
as s/ConstraintRelidIndexId/ConstraintRelidTypidNameIndexId/g -- I tried
that, and it passed regression tests -- but I couldn't resist the
temptation to fix a few places that could make better use of the
redesigned index.

0002 adds user-friendliness by installing a nicer error message for
the complained-of case and by improving ChooseIndexName to avoid
autogenerating index names that will conflict with existing constraints.

I didn't look for possible documentation changes yet, but I think the
code changes are OK.

			regards, tom lane

  8. Re: Two constraints with the same name not always allowed

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-09-06T21:53:47Z

    On 02/09/2018 19:00, Tom Lane wrote:
> This also points up the lack of a suitable unique index on pg_constraint.
> It's sort of difficult to figure out what that should look like given that
> pg_constraint contains two quasi-independent collections of constraints,
> but maybe UNIQUE(conrelid,contypid,conname) would serve given the
> reasonable assumption that exactly one of conrelid and contypid is zero.

Sketches for assertions set both conrelid and contypid to zero.  I think
the unique constraint would have to include connamespace to support that
properly.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: Two constraints with the same name not always allowed

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-06T22:07:51Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 02/09/2018 19:00, Tom Lane wrote:
>> This also points up the lack of a suitable unique index on pg_constraint.
>> It's sort of difficult to figure out what that should look like given that
>> pg_constraint contains two quasi-independent collections of constraints,
>> but maybe UNIQUE(conrelid,contypid,conname) would serve given the
>> reasonable assumption that exactly one of conrelid and contypid is zero.

> Sketches for assertions set both conrelid and contypid to zero.  I think
> the unique constraint would have to include connamespace to support that
> properly.

Well, as I said in the commit message, I'm now of the opinion that
assertions should go in some new catalog.  It was a mistake to put
domain and relation constraints into the same catalog, and I don't
think we ought to double down on that mistake by confusing the
question of "what's this catalog's primary key?" still more.

But yes, *if* we bull ahead and do it the wrong way, that would be
a necessary change.  I don't like it much, because it would fuzz up
the question of whether the unique index actually guarantees only
one instance of a constraint name per relid or typid.

			regards, tom lane