Thread: Deferred RI trigger for non-key UPDATEs and subxacts
With some time to spare, I thought I'd submit a quick-fix patch to the
issue I reported here:
http://archives.postgresql.org/pgsql-hackers/2007-07/msg00339.php
This should preclude optimizing away a deferred RI trigger if the
UPDATEd row (in the FK table) was inserted by "current" transaction
(i.e. defined by TransactionIdIsCurrentTransactionId()) and not
necessarily "by our own transaction" as the code currently says.
________________________________________________________________________
backend/commands/trigger.c | 17 !!!!!!!!!!!!!!!!!
test/regress/expected/foreign_key.out | 15 +++++++++++++++
test/regress/sql/foreign_key.sql | 19 +++++++++++++++++++
3 files changed, 34 insertions(+), 17 modifications(!)
________________________________________________________________________
--
Affan Salman
EnterpriseDB Corporation http://www.enterprisedb.com
Index: src/backend/commands/trigger.c
===================================================================
RCS file: /home/affan/repos/cvs/pgsql/pgsql/src/backend/commands/trigger.c,v
retrieving revision 1.215
diff -p -c -b -r1.215 trigger.c
*** src/backend/commands/trigger.c 1 Jul 2007 17:45:42 -0000 1.215
--- src/backend/commands/trigger.c 13 Jul 2007 20:13:13 -0000
*************** AfterTriggerSaveEvent(ResultRelInfo *rel
*** 3389,3403 ****
* Update on FK table
*
* There is one exception when updating FK tables: if the
! * updated row was inserted by our own transaction and the
! * FK is deferred, we still need to fire the trigger. This
! * is because our UPDATE will invalidate the INSERT so the
! * end-of-transaction INSERT RI trigger will not do
! * anything, so we have to do the check for the UPDATE
! * anyway.
*/
! if (HeapTupleHeaderGetXmin(oldtup->t_data) !=
! GetCurrentTransactionId() &&
RI_FKey_keyequal_upd_fk(trigger, rel, oldtup, newtup))
{
continue;
--- 3389,3404 ----
* Update on FK table
*
* There is one exception when updating FK tables: if the
! * updated row was inserted by "current" transaction (any
! * active or sub-committed xact from the local transaction
! * tree) and the FK is deferred, we still need to fire the
! * trigger. This is because our UPDATE will invalidate the
! * INSERT so the end-of-transaction INSERT RI trigger will
! * not do anything, so we have to do the check for the
! * UPDATE anyway.
*/
! if (!TransactionIdIsCurrentTransactionId(
! HeapTupleHeaderGetXmin(oldtup->t_data)) &&
RI_FKey_keyequal_upd_fk(trigger, rel, oldtup, newtup))
{
continue;
Index: src/test/regress/sql/foreign_key.sql
===================================================================
RCS file: /home/affan/repos/cvs/pgsql/pgsql/src/test/regress/sql/foreign_key.sql,v
retrieving revision 1.19
diff -p -c -b -r1.19 foreign_key.sql
*** src/test/regress/sql/foreign_key.sql 5 Jun 2007 21:31:09 -0000 1.19
--- src/test/regress/sql/foreign_key.sql 13 Jul 2007 21:22:56 -0000
*************** UPDATE fktable SET id = id + 1;
*** 830,832 ****
--- 830,851 ----
-- should catch error from initial INSERT
COMMIT;
+
+ -- Now test the same UPDATE from a SAVEPOINT to validate that we do
+ -- not optimize away the FK RI trigger when the transaction that
+ -- created the being-UPDATEd row isn't the same as the transaction
+ -- UPDATing the row; but is a "current" transaction.
+
+ BEGIN;
+
+ -- doesn't match PK, but no error yet
+ INSERT INTO fktable VALUES (0, 20);
+
+ -- subxact for this SAVEPOINT will be active now
+ SAVEPOINT updSavept;
+
+ -- don't change FK
+ UPDATE fktable SET id = id + 1;
+
+ -- should catch error from initial INSERT
+ COMMIT;
Index: src/test/regress/expected/foreign_key.out
===================================================================
RCS file: /home/affan/repos/cvs/pgsql/pgsql/src/test/regress/expected/foreign_key.out,v
retrieving revision 1.43
diff -p -c -b -r1.43 foreign_key.out
*** src/test/regress/expected/foreign_key.out 5 Jun 2007 21:31:09 -0000 1.43
--- src/test/regress/expected/foreign_key.out 13 Jul 2007 21:23:12 -0000
*************** UPDATE fktable SET id = id + 1;
*** 1193,1195 ****
--- 1193,1210 ----
COMMIT;
ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey"
DETAIL: Key (fk)=(20) is not present in table "pktable".
+ -- Now test the same UPDATE from a SAVEPOINT to validate that we do
+ -- not optimize away the FK RI trigger when the transaction that
+ -- created the being-UPDATEd row isn't the same as the transaction
+ -- UPDATing the row; but is a "current" transaction.
+ BEGIN;
+ -- doesn't match PK, but no error yet
+ INSERT INTO fktable VALUES (0, 20);
+ -- subxact for this SAVEPOINT will be active now
+ SAVEPOINT updSavept;
+ -- don't change FK
+ UPDATE fktable SET id = id + 1;
+ -- should catch error from initial INSERT
+ COMMIT;
+ ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey"
+ DETAIL: Key (fk)=(20) is not present in table "pktable".
On Sat, 2007-07-14 at 00:12 +0100, Affan Salman wrote: > With some time to spare, I thought I'd submit a quick-fix patch to the > issue I reported here: > > http://archives.postgresql.org/pgsql-hackers/2007-07/msg00339.php > > This should preclude optimizing away a deferred RI trigger if the > UPDATEd row (in the FK table) was inserted by "current" transaction > (i.e. defined by TransactionIdIsCurrentTransactionId()) and not > necessarily "by our own transaction" as the code currently says. Good bug fix, looks correct to me. I've re-checked all the call points of GetCurrentTransactionId() to see if there was any further abuse of it, but cannot find any. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Affan Salman" <affan@enterprisedb.com> writes: > With some time to spare, I thought I'd submit a quick-fix patch to the > issue I reported here: > http://archives.postgresql.org/pgsql-hackers/2007-07/msg00339.php I don't think this is right. If the original tuple was inserted by a subtransaction of our transaction, it will have been checked at subtransaction subcommit, no? ISTM what we need is to schedule the on-UPDATE trigger if the original tuple was inserted by either our current (sub)transaction or one of its parents, and those are not the semantics of TransactionIdIsCurrentTransactionId, unfortunately. Stephan, have you looked at this bug report? What do you think? regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > "Affan Salman" <affan@enterprisedb.com> writes: >> With some time to spare, I thought I'd submit a quick-fix patch to the >> issue I reported here: >> http://archives.postgresql.org/pgsql-hackers/2007-07/msg00339.php > > I don't think this is right. If the original tuple was inserted by a > subtransaction of our transaction, it will have been checked at > subtransaction subcommit, no? That doesn't sound right. > RELEASE SAVEPOINT destroys a savepoint previously defined in the current > transaction. > > Destroying a savepoint makes it unavailable as a rollback point, but it has > no other user visible behavior. On the other hand what happens if you have constraints not deferred, insert a record, then set constraints deferred and update it? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Sun, 15 Jul 2007, Tom Lane wrote: > "Affan Salman" <affan@enterprisedb.com> writes: > > With some time to spare, I thought I'd submit a quick-fix patch to the > > issue I reported here: > > http://archives.postgresql.org/pgsql-hackers/2007-07/msg00339.php > > I don't think this is right. If the original tuple was inserted by a > subtransaction of our transaction, it will have been checked at > subtransaction subcommit, no? ISTM what we need is to schedule the > on-UPDATE trigger if the original tuple was inserted by either our > current (sub)transaction or one of its parents, and those are not the > semantics of TransactionIdIsCurrentTransactionId, unfortunately. > > Stephan, have you looked at this bug report? What do you think? I don't think the subtransaction subcommit will do the check. Unless I'm missing something about the code, a CommitTransaction would but a CommitSubTransaction won't, which actually makes sense given that we're mapping savepoints on to it, and I don't think we are allowed to check at savepoint release time. I tried a few small ariations on the given example, all of which fail on my 8.2.4 machine, including the following, but maybe I've missed the scenario you're envisioning: begin; savepoint i1; insert ... ; release i1; savepoint u1; update ...; release u1; commit; begin; savepoint i1; insert ... ; release i1; savepoint u1; update ...; commit; begin; savepoint a1; savepoint a2; insert ...; release a2; update ...; commit; begin; savepoint a1; savepoint a2; insert ...; release a2; savepoint a3; update ...; commit;
Tom Lane wrote: > I don't think this is right. If the original tuple was inserted by a > subtransaction of our transaction, it will have been checked at > subtransaction subcommit, no? No, it will be checked at main transaction commit; the immediate_only flag is FALSE for afterTriggerMarkEvents() only through the invocation of AfterTriggerFireDeferred(), which comes from CommitTransaction() or PrepareTransaction(). -- Affan Salman EnterpriseDB Corporation http://www.enterprisedb.com
"Gregory Stark" <stark@enterprisedb.com> writes: > On the other hand what happens if you have constraints not deferred, insert a > record, then set constraints deferred and update it? After having a coffee this is obviously not a problem since if you have constraints not deferred then the constraint was checked immediately. We don't have to do the constraint in that case even if the row was inserted by us but that's an optimization that probably nobody cares about. If you go the other direction from deferred to not deferred then the constraint will be checked when you set the constraint to immediate so it's safe to skip the constraint check if the keys match subsequently regardless of whether we inserted the record. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Stephan Szabo <sszabo@megazone.bigpanda.com> writes:
> On Sun, 15 Jul 2007, Tom Lane wrote:
>> I don't think this is right. If the original tuple was inserted by a
>> subtransaction of our transaction, it will have been checked at
>> subtransaction subcommit, no?
> I don't think the subtransaction subcommit will do the check. Unless I'm
> missing something about the code, a CommitTransaction would but a
> CommitSubTransaction won't, which actually makes sense given that we're
> mapping savepoints on to it, and I don't think we are allowed to check at
> savepoint release time.
OK, that's what I get for opining before checking the code ;-). It
seems a little weird that a subcommitted subtransaction could still
cause a failure later, but that is how we're doing the triggers.
Given that, the proposed patch seems appropriate. Will apply.
regards, tom lane
> > OK, that's what I get for opining before checking the code ;-). Your *cerebral call graph visits* have a knack of being spot on, way more than often. :-) > > Will apply. Thanks, Tom. We're also back-patching this, right? -- Affan Salman EnterpriseDB Corporation http://www.enterprisedb.com
"Affan Salman" <affan@enterprisedb.com> writes:
> Thanks, Tom. We're also back-patching this, right?
Yeah, working on that now.
regards, tom lane