Thread: Fixing handling of constraint triggers
I want to do something about the open item discussed in this thread: http://archives.postgresql.org/message-id/20090811111446.GA25965@depesz.com The right way to handle that, IMO, is to create pg_constraint rows for triggers created via CREATE CONSTRAINT TRIGGER. Then, AfterTriggerSetState can initially search pg_constraint to identify which constraint is being targeted. Aside from allowing it to throw a more understandable error for non-deferrable index constraints, this will greatly simplify its search logic, which is a mess right now. What seems to be needed in detail is: pg_constraint.contype gains an additional possible value, CONSTRAINT_TRIGGER = 't'. We can drop pg_trigger.tgconstrname (and the index on it) and pg_trigger.tgisconstraint. Instead we'll want an index on pg_trigger.tgconstraint so that we can cheaply search pg_trigger by constraint OID. Because CREATE CONSTRAINT TRIGGER will now create a pg_trigger row with nonzero tgconstraint, it is no longer possible to use "tgconstraint is nonzero" as a proxy for "system-generated trigger". This is a problem for pg_dump in particular, which won't know which triggers it actually needs to dump. I think the best fix is to add a boolean column "tgisinternal" to flag system-generated triggers. Normally, a trigger associated with a constraint has an internal dependency on the pg_constraint entry. For CREATE CONSTRAINT TRIGGER it'll be the other way around --- pg_constraint internally depends on pg_trigger --- since you're supposed to use DROP TRIGGER not DROP CONSTRAINT to remove the assemblage. AFAICS the only user-visible change in behavior from prior versions will be that the system will complain if you try to create a constraint trigger that has the same name as an existing constraint of another type on the same table. This doesn't seem like a big problem in practice, and in any case it's appropriate since a conflict would make it unclear which constraint SET CONSTRAINTS is meant to apply to. Thoughts, objections? regards, tom lane
2010/1/17 Tom Lane <tgl@sss.pgh.pa.us>: > I want to do something about the open item discussed in this thread: > http://archives.postgresql.org/message-id/20090811111446.GA25965@depesz.com > > The right way to handle that, IMO, is to create pg_constraint rows for > triggers created via CREATE CONSTRAINT TRIGGER. Then, > AfterTriggerSetState can initially search pg_constraint to identify > which constraint is being targeted. Aside from allowing it to throw a > more understandable error for non-deferrable index constraints, this > will greatly simplify its search logic, which is a mess right now. > > [snip] > > AFAICS the only user-visible change in behavior from prior versions > will be that the system will complain if you try to create a constraint > trigger that has the same name as an existing constraint of another type > on the same table. This doesn't seem like a big problem in practice, > and in any case it's appropriate since a conflict would make it unclear > which constraint SET CONSTRAINTS is meant to apply to. Agreed. That's much neater. However, it does introduce a change in behaviour - if you have 2 constraints with the same name in different schemas, one deferrable, and one not, and the non-deferrable one is first on the search path, then you'll get an error if you try to defer the other one, referring to it with an unqualified name. This used to work, when it was searching for the trigger first. So perhaps the code should continue searching after finding a non-deferrable constraint, remembering the fact that it found one, and only report the error at the end if it didn't also find a deferrable one. Regards, Dean
Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
> Agreed. That's much neater. However, it does introduce a change in
> behaviour - if you have 2 constraints with the same name in different
> schemas, one deferrable, and one not, and the non-deferrable one is
> first on the search path, then you'll get an error if you try to defer
> the other one, referring to it with an unqualified name. This used to
> work, when it was searching for the trigger first.
> So perhaps the code should continue searching after finding a
> non-deferrable constraint, remembering the fact that it found one, and
> only report the error at the end if it didn't also find a deferrable
> one.
I went around on that a few times before committing. It's not really
true that it used to work, at least not for cases where the
nondeferrable constraint was also an FK constraint; that threw an error
just as now. What we've done is opened up the search so that it will
find nondeferrable index constraints, which is more or less the whole
point of Hubert's request.
The part that I actually think is weird is that the search stops after
the first schema in which it finds any matches. If we removed that then
we could just document the behavior as "sets the constraint mode for all
matching constraint names", full stop. Which definitely seems easier
to understand than what we have now. (The SQL spec is no big help here
btw, it just says "if a <constraint name> is specified, then it shall
identify a deferrable constraint". No clarity about what "identify"
means.)
The other point you bring up is whether to silently skip matching
constraints that aren't deferrable, rather than throwing error.
I'm not real excited about that --- it seems likely to mask problems,
and to the extent that you want to put faith in the wording of the SQL
spec here, it seems clear that an error is what they want.
These are certainly easy enough changes from a code standpoint.
The question is what behavior makes the most sense. Comments anyone?
regards, tom lane
2010/1/18 Tom Lane <tgl@sss.pgh.pa.us>: > Dean Rasheed <dean.a.rasheed@googlemail.com> writes: >> Agreed. That's much neater. However, it does introduce a change in >> behaviour - if you have 2 constraints with the same name in different >> schemas, one deferrable, and one not, and the non-deferrable one is >> first on the search path, then you'll get an error if you try to defer >> the other one, referring to it with an unqualified name. This used to >> work, when it was searching for the trigger first. > >> So perhaps the code should continue searching after finding a >> non-deferrable constraint, remembering the fact that it found one, and >> only report the error at the end if it didn't also find a deferrable >> one. > > I went around on that a few times before committing. It's not really > true that it used to work, at least not for cases where the > nondeferrable constraint was also an FK constraint; that threw an error > just as now. What we've done is opened up the search so that it will > find nondeferrable index constraints, which is more or less the whole > point of Hubert's request. > Ah OK. It was unique constraints that I was thinking of when I looked at it, and in the scenario described above it used to work by ignoring the non-deferrable one and just applying to the deferrable one. But since that's new functionality, no one's likely to notice the change. I think the new behaviour makes more sense and it's now consistent with FK constraints, in throwing an error if the first matching constraint on the search path is not deferrable. > The part that I actually think is weird is that the search stops after > the first schema in which it finds any matches. If we removed that then > we could just document the behavior as "sets the constraint mode for all > matching constraint names", full stop. Which definitely seems easier > to understand than what we have now. (The SQL spec is no big help here > btw, it just says "if a <constraint name> is specified, then it shall > identify a deferrable constraint". No clarity about what "identify" > means.) > Yeah I could go either way on that, so I guess for compatibility it's best to stick with the existing behaviour. It's not that surprising to have it apply only to the first match on the search path. Users can always use schema qualification if needed. I only really raised this because I thought the behaviour was changing, but that's not really the case for users who never had deferrable uniqueness constraints before, so FWIW I'm happy to stick with the existing code. Cheers, Dean