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...7/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



---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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...7/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


---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

"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...7/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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

"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...7/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


---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

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...7/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;

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

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



---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

"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


---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

>
> 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


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

"Affan Salman" <affan@enterprisedb.com> writes:
> Thanks, Tom. We're also back-patching this, right?

Yeah, working on that now.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster