Firing order of RI triggers

I've looked into the cause of bug #6268,
http://archives.postgresql.org/pgsql-bugs/2011-10/msg00223.php

It's fairly simple: we're firing RI triggers in the wrong order.

What's happening is that we update the tuple and queue RI_FKey_check_upd
and RI_FKey_cascade_upd events for the update action, *in that order*.
When RI_FKey_check_upd runs, it checks things and quite properly
complains that there's no matching PK, since the row that used to have
the matching PK is now obsolete.

Had RI_FKey_cascade_upd fired first, all would have been well,
because when RI_FKey_check_upd fired for this particular update, it
would've seen the new tuple is already obsolete and done nothing.

The reason they fire in the wrong order is that triggers for a single
event are fired in name order, and the names being used are things like
RI_ConstraintTrigger_53569. Most of the time, the trigger with higher
OID is going to sort last ... and createForeignKeyTriggers creates the
check triggers before the action triggers.

You might wonder why this doesn't mean that all self-referential foreign
key situations are broken all the time. Well, the answer is that the
problem is usually masked by the optimization that avoids firing a check
trigger at all if the referencing field didn't change --- see
AfterTriggerSaveEvent. In the test case given in the bug, the first
UPDATE within the transaction doesn't see the problem because of this.
But in the second UPDATE of the same row, that optimization is disabled,
so the check trigger fires and fails.

As far as I can see, the only practical way to fix this is to change the
names given to RI triggers so that cascade actions will fire before
check triggers. Just changing the order of creation would fix it 99.99%
of the time, but fail on the times when the first trigger had OID 99999
and the second OID 1000000, for example. And I definitely don't think
we want to mess with the general rule that triggers fire in name order.

I'm thinking we could do "RI_ConstraintTrigger_a_NNNN" for "action"
triggers and "RI_ConstraintTrigger_c_NNNN" for "checking" triggers,
and then the names would be guaranteed to sort correctly.

I'm not sure if this is something we can back-patch --- I don't see any
dependencies in our own code on what names RI triggers have, but I'm
afraid there is client-side code out there that knows it. In any case,
changing the name assignments would not fix things for existing
triggers; but if we did back-patch then any affected users could just
drop and re-create the problematic FK constraint. Or maybe we could
back-patch a change in creation order and rely on that usually working.
Given the lack of prior complaints that might be good enough.

Comments?

regards, tom lane

Excerpts from Tom Lane's message of mar oct 25 14:57:43 -0300 2011:

> I'm not sure if this is something we can back-patch --- I don't see any
> dependencies in our own code on what names RI triggers have, but I'm
> afraid there is client-side code out there that knows it.

Yeah, sounds possible.

> In any case,
> changing the name assignments would not fix things for existing
> triggers; but if we did back-patch then any affected users could just
> drop and re-create the problematic FK constraint. Or maybe we could
> back-patch a change in creation order and rely on that usually working.
> Given the lack of prior complaints that might be good enough.

The latter looks reasonable ... particularly if the symptoms of a
botched order would be immediately visible -- the user could just drop
and reload the constraints to fix the order in the very unlikely case
that they are reversed.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Tom Lane's message of mar oct 25 14:57:43 -0300 2011:
>> ... Or maybe we could
>> back-patch a change in creation order and rely on that usually working.
>> Given the lack of prior complaints that might be good enough.

> The latter looks reasonable ... particularly if the symptoms of a
> botched order would be immediately visible -- the user could just drop
> and reload the constraints to fix the order in the very unlikely case
> that they are reversed.

Well, the symptoms would probably be just like in the bug report: you'd
get unexpected failures from double updates of a self-referential row in
a single transaction. That's a sufficiently weird corner case that most
people probably wouldn't exercise it right away. But given that this
problem has been there from day one and nobody noticed before, I'm not
too concerned about the intersection of people who have an issue and
people who are unlucky enough to get an end-of-decade trigger OID.
I think 100% solution in HEAD and 99.99% solution in back branches
should be good enough.

regards, tom lane