Thread: rename constraint behavior for duplicate names?

I'm starting to get into PostgreSQL development by implementing:
%Allow ALTER TABLE ... ALTER CONSTRAINT ... RENAME

from the TODO. I've been looking through the code from CommentConstraint
and ATExecDropConstraint and they error out on duplicate constraint
names for a relation. However, ADD CONSTRAINT's code checks for
duplicates and errors out, so would the stuff in comment/drop be useless
checks then? And I would not have to worry about duplicate constraint
names for my rename code?

Allan Wang

Allan Wang <allanvv@gmail.com> writes:
> I've been looking through the code from CommentConstraint
> and ATExecDropConstraint and they error out on duplicate constraint
> names for a relation. However, ADD CONSTRAINT's code checks for
> duplicates and errors out, so would the stuff in comment/drop be useless
> checks then? And I would not have to worry about duplicate constraint
> names for my rename code?

Hmm ... there seems to be a certain amount of version skew here.
Awhile back (experimentation says it was up through 7.2) we would allow
multiple foreign key constraints with the same name, and with a name
duplicating a check constraint ... but not AFAICS duplicate check
constraint names.  I think these various bits of code probably need to
be brought into agreement about what the plan is.  If we are going to
enforce constraint name uniqueness then there ought to be a unique index
guaranteeing it (which in turn would allow simplification of the lookup
code).  Note however that it's customary to check for duplication and
issue a specific error message for it --- "unique key violation" isn't
considered a friendly error message.  The index should just serve as a
backstop in case of race conditions or other unforeseen problems.

It strikes me BTW that having pg_constraint cover both table and domain
constraints was probably a dumb idea, and that normalization principles
would suggest splitting it into one table for each purpose.
        regards, tom lane

On Thu, 2005-09-01 at 17:16 -0400, Tom Lane wrote:
> Allan Wang <allanvv@gmail.com> writes:
> > I've been looking through the code from CommentConstraint
> > and ATExecDropConstraint and they error out on duplicate constraint
> > names for a relation. However, ADD CONSTRAINT's code checks for
> > duplicates and errors out, so would the stuff in comment/drop be useless
> > checks then? And I would not have to worry about duplicate constraint
> > names for my rename code?
> 
> Note however that it's customary to check for duplication and
> issue a specific error message for it --- "unique key violation" isn't
> considered a friendly error message.  The index should just serve as a
> backstop in case of race conditions or other unforeseen problems.

Alright, I see why the checks are still needed. The unique index should
be on relname, conname right? Also looking into DROP CONSTRAINT's code,
it gives a notice about "multiple constraint names dropped" when
RemoveRelConstraints(rel, conname) returns > 1. This check isn't needed
anymore right? Also RemoveRelConstraints can be simplified to assume
only one row will need removing, and be turned into a void function?

Allan Wang

Allan Wang <allanvv@gmail.com> writes:
> Alright, I see why the checks are still needed. The unique index should
> be on relname, conname right? Also looking into DROP CONSTRAINT's code,
> it gives a notice about "multiple constraint names dropped" when
> RemoveRelConstraints(rel, conname) returns > 1. This check isn't needed
> anymore right? Also RemoveRelConstraints can be simplified to assume
> only one row will need removing, and be turned into a void function?

Not unless you want to break the "quiet" option for ATExecDropConstraint.
        regards, tom lane

Allan Wang <allanvv@gmail.com> writes:
> (It would still be used for suppressing the not-exists error)

Right, which is why RemoveRelConstraints has to tell if it removed
anything.  The API could be changed, but not to "returns void".
        regards, tom lane

On Thu, 2005-09-01 at 17:55 -0400, Tom Lane wrote:
> Allan Wang <allanvv@gmail.com> writes:
> > Alright, I see why the checks are still needed. The unique index should
> > be on relname, conname right? Also looking into DROP CONSTRAINT's code,
> > it gives a notice about "multiple constraint names dropped" when
> > RemoveRelConstraints(rel, conname) returns > 1. This check isn't needed
> > anymore right? Also RemoveRelConstraints can be simplified to assume
> > only one row will need removing, and be turned into a void function?
> 
> Not unless you want to break the "quiet" option for ATExecDropConstraint.

Is the quiet option supposed to work anyway other than suppressing the
not exists error? Since there can't be multiple constraint names the
notice is never executed anyway. Otherwise I don't see how it would
break. (It would still be used for suppressing the not-exists error)

Allan Wang