Thread: Hm, table constraints aren't so unique as all that
Over in the thread about enhanced error fields, I claimed that
"constraints are uniquely named among those associated with a table,
or with a domain". But it turns out that that ain't necessarily so,
because the code path for index constraints doesn't pay any attention
to pre-existing check constraints:
d1=# create table t1 (f1 int);
CREATE TABLE
d1=# alter table t1 add constraint c1 check (f1 > 0);
ALTER TABLE
d1=# alter table t1 add constraint c1 unique (f1);
ALTER TABLE
d1=# \d t1 Table "public.t1"Column | Type | Modifiers
--------+---------+-----------f1 | integer |
Indexes: "c1" UNIQUE CONSTRAINT, btree (f1)
Check constraints: "c1" CHECK (f1 > 0)
If you do this in the other order it does get rejected:
d1=# create table t2 (f1 int);
CREATE TABLE
d1=# alter table t2 add constraint c2 unique (f1);
ALTER TABLE
d1=# alter table t2 add constraint c2 check (f1 > 0);
ERROR: constraint "c2" for relation "t2" already exists
Aside from being plain inconsistent, this seems to me to create a
dump/reload hazard: pg_dump has no idea that it would have to dump
these two constraints in a particular order to make them reloadable.
In practice there's not such a big risk because pg_dump prefers to stick
CHECK constraints directly into the CREATE TABLE rather than add them
after-the-fact. But if it had to split off the CHECK constraint to
avoid a circularity problem, I don't believe there's anything preventing
a reload failure.
I think we need to tighten this down by having index-constraint creation
check for conflicts with other constraint types. It also seems like it
might be a good idea to put in a unique index to enforce the intended
lack of conflicts --- note that the existing index on (conname,
connamespace) isn't unique. It's a bit problematic that pg_constraint
contains both table-related constraints and domain-related constraints,
but it strikes me that we could get close enough by changing
pg_constraint_conname_nsp_index to be a unique index on
(conname, connamespace, conrelid, contypid). That would fix the problem
as long as no pg_constraint entry ever has both conrelid and contypid
nonzero; the unique index couldn't catch such an error. But it doesn't
seem to me that such a coding error would escape detection anyway.
Of course this wouldn't be material for back-patching, but it seems to
me there's still time to fix this for 9.3, and we should do so if we
want to claim that the enhanced-errors patch uniquely identifies
constraints.
Thoughts?
regards, tom lane
On 29 January 2013 00:25, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Of course this wouldn't be material for back-patching, but it seems to > me there's still time to fix this for 9.3, and we should do so if we > want to claim that the enhanced-errors patch uniquely identifies > constraints. I can see the case for fixing this, but I don't feel that it's particularly important that constraints be uniquely identifiable from the proposed new errdata fields. -- Regards, Peter Geoghegan
Peter Geoghegan <peter.geoghegan86@gmail.com> writes:
> On 29 January 2013 00:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Of course this wouldn't be material for back-patching, but it seems to
>> me there's still time to fix this for 9.3, and we should do so if we
>> want to claim that the enhanced-errors patch uniquely identifies
>> constraints.
> I can see the case for fixing this, but I don't feel that it's
> particularly important that constraints be uniquely identifiable from
> the proposed new errdata fields.
I think that we'll soon be buried in gripes if they're not. Pretty much
the whole point of this patch is to allow applications to get rid of
ad-hoc, it-usually-works coding techniques. I'd argue that not checking
the entire constraint identity is about as fragile as trying to "sed"
the constraint name out of a potentially-localized error message.
In both cases, it often works fine, until the application's context
changes.
regards, tom lane
On Mon, Jan 28, 2013 at 10:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think that we'll soon be buried in gripes if they're not. Pretty much > the whole point of this patch is to allow applications to get rid of > ad-hoc, it-usually-works coding techniques. I'd argue that not checking > the entire constraint identity is about as fragile as trying to "sed" > the constraint name out of a potentially-localized error message. > In both cases, it often works fine, until the application's context > changes. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> Tom Lane Wrote: > Peter Geoghegan <peter.geoghegan86@gmail.com> writes: > > On 29 January 2013 00:25, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I can see the case for fixing this, but I don't feel that it's > > particularly important that constraints be uniquely identifiable from > > the proposed new errdata fields. > > I think that we'll soon be buried in gripes if they're not. Pretty much the > whole point of this patch is to allow applications to get rid of ad-hoc, it- > usually-works coding techniques. I'd argue that not checking the entire > constraint identity is about as fragile as trying to "sed" > the constraint name out of a potentially-localized error message. > In both cases, it often works fine, until the application's context changes. +1 here too. I'm feel I'm quite close to the front of the queue of application developers waiting on enhances error fields. I'd personally rather I noticed my application was broken during an testing an upgrade to 9.3 than somewhere down the line. I can't imagine renaming a constraint to upgrade to 9.3 is going to be a showstopper for anyone. Perhaps the release notes can contain a query to allow users to check this pre-upgrade. Regards David Rowley > > regards, tom lane > > > --
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Peter Geoghegan <peter.geoghegan86@gmail.com> writes:
> > I can see the case for fixing this, but I don't feel that it's
> > particularly important that constraints be uniquely identifiable from
> > the proposed new errdata fields.
>
> I think that we'll soon be buried in gripes if they're not. Pretty much
> the whole point of this patch is to allow applications to get rid of
> ad-hoc, it-usually-works coding techniques. I'd argue that not checking
> the entire constraint identity is about as fragile as trying to "sed"
> the constraint name out of a potentially-localized error message.
> In both cases, it often works fine, until the application's context
> changes.
Perhaps I wasn't clear previously, but this is precisely what I had been
argueing for upthread..
Thanks,
Stephen
I wrote:
> Over in the thread about enhanced error fields, I claimed that
> "constraints are uniquely named among those associated with a table,
> or with a domain". But it turns out that that ain't necessarily so,
> because the code path for index constraints doesn't pay any attention
> to pre-existing check constraints: ...
> I think we need to tighten this down by having index-constraint creation
> check for conflicts with other constraint types. It also seems like it
> might be a good idea to put in a unique index to enforce the intended
> lack of conflicts --- note that the existing index on (conname,
> connamespace) isn't unique. It's a bit problematic that pg_constraint
> contains both table-related constraints and domain-related constraints,
> but it strikes me that we could get close enough by changing
> pg_constraint_conname_nsp_index to be a unique index on
> (conname, connamespace, conrelid, contypid).
I experimented with changing pg_constraint's index that way. It doesn't
seem to break anything, but it turns out not to fix the problem
completely either, because if you use CREATE INDEX syntax to create an
index then no pg_constraint entry is made at all. So it's still
possible to have an index with the same name as some non-index
constraint on the same table.
If we wanted to pursue this, we could think about decreeing that every
index must have a pg_constraint entry. That would have some attraction
from the standpoint of catalog-entry uniformity, but there are
considerable practical problems in the way as well. Notably, what would
we do for the conkey field in pg_constraint for an expression index?
(Failing to set that up as expected might well break client-side code.)
Also, I think we'd end up with the pg_depend entry between the index and
the constraint pointing in opposite directions depending on whether the
index was made using CONSTRAINT syntax or CREATE INDEX syntax. There's
some precedent for that with the linkage between pg_class entries and
their pg_type rowtype entries, but that's a mess that I'd rather not
replicate.
Or we could leave the catalogs alone and just add more pre-creation
checking for conflicts. That doesn't seem very bulletproof though
because of possible race conditions. I think that right now it'd
be safe enough because of the table-level locks taken by ALTER TABLE
and CREATE INDEX --- but if the project to reduce ALTER TABLE's locking
level ever gets resurrected, we'd be at serious risk of introducing
a problem there.
Or on the third hand, we could just say it's okay if there are conflicts
between index names and check-constraint names. Any given SQLSTATE
would only be mentioning one of these types of constraints, so it's
arguable that there's not going to be any real ambiguity in practice.
At the moment I'm inclined to leave well enough alone. Thoughts?
regards, tom lane
On 1/28/13 6:25 PM, Tom Lane wrote: > I think we need to tighten this down by having index-constraint creation > check for conflicts with other constraint types. It also seems like it > might be a good idea to put in a unique index to enforce the intended > lack of conflicts --- note that the existing index on (conname, > connamespace) isn't unique. It's a bit problematic that pg_constraint > contains both table-related constraints and domain-related constraints, > but it strikes me that we could get close enough by changing > pg_constraint_conname_nsp_index to be a unique index on > (conname, connamespace, conrelid, contypid). That would fix the problem > as long as no pg_constraint entry ever has both conrelid and contypid > nonzero; the unique index couldn't catch such an error. But it doesn't > seem to me that such a coding error would escape detection anyway. My belt-and-suspenders mind tells me that there should be a check constraint enforcing that either conrelid IS NOT NULL XOR contypid IS NOT NULL. We routinely do this at work. Dunno if putting check constraints on catalog tables is possible/sane though...
On 1/29/13 6:40 PM, Tom Lane wrote: > I wrote: >> >Over in the thread about enhanced error fields, I claimed that >> >"constraints are uniquely named among those associated with a table, >> >or with a domain". But it turns out that that ain't necessarily so, >> >because the code path for index constraints doesn't pay any attention >> >to pre-existing check constraints: ... >> >I think we need to tighten this down by having index-constraint creation >> >check for conflicts with other constraint types. It also seems like it >> >might be a good idea to put in a unique index to enforce the intended >> >lack of conflicts --- note that the existing index on (conname, >> >connamespace) isn't unique. It's a bit problematic that pg_constraint >> >contains both table-related constraints and domain-related constraints, >> >but it strikes me that we could get close enough by changing >> >pg_constraint_conname_nsp_index to be a unique index on >> >(conname, connamespace, conrelid, contypid). > I experimented with changing pg_constraint's index that way. It doesn't > seem to break anything, but it turns out not to fix the problem > completely either, because if you use CREATE INDEX syntax to create an > index then no pg_constraint entry is made at all. So it's still > possible to have an index with the same name as some non-index > constraint on the same table. > > If we wanted to pursue this, we could think about decreeing that every > index must have a pg_constraint entry. That would have some attraction > from the standpoint of catalog-entry uniformity, but there are > considerable practical problems in the way as well. Notably, what would > we do for the conkey field in pg_constraint for an expression index? > (Failing to set that up as expected might well break client-side code.) > Also, I think we'd end up with the pg_depend entry between the index and > the constraint pointing in opposite directions depending on whether the > index was made using CONSTRAINT syntax or CREATE INDEX syntax. There's > some precedent for that with the linkage between pg_class entries and > their pg_type rowtype entries, but that's a mess that I'd rather not > replicate. > > Or we could leave the catalogs alone and just add more pre-creation > checking for conflicts. That doesn't seem very bulletproof though > because of possible race conditions. I think that right now it'd > be safe enough because of the table-level locks taken by ALTER TABLE > and CREATE INDEX --- but if the project to reduce ALTER TABLE's locking > level ever gets resurrected, we'd be at serious risk of introducing > a problem there. > > Or on the third hand, we could just say it's okay if there are conflicts > between index names and check-constraint names. Any given SQLSTATE > would only be mentioning one of these types of constraints, so it's > arguable that there's not going to be any real ambiguity in practice. > > At the moment I'm inclined to leave well enough alone. Thoughts? ISTM that we shouldn't blindly equate indexes and constraints. I'd actually argue they should in no way be related, except that I've found it to be extremely useful to create unique indexes that cover scenarios that you can't handle with an actual unique constraint (ie: UNIQUE(field_a, field_B) WHERE field_c IS NULL). Perhaps a good compromise would be that only unique indexes get entries in pg_constraint. Something else worth mentioning is that hopefully we'll eventually have indexes (including unique) that can span multiple tables.