Deferred RI trigger for non-key UPDATEs and subxacts

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