tg_trigtuple/tg_newtuple settings in AFTER triggers
Set tg_trigtuple/tg_newtuple in AFTER triggers according to whether
old and new tuples were supplied rather than blindly setting them
according to the event type. Per discussion in pgsql-hackers.
http://archives.postgresql.org/pgsql-hackers/2006-07/msg01601.php
If the patch is logically or stylistically flawed then please advise
and I'll rework it. Thanks.
--
Michael Fuhr
Attachments:
trigger.c.difftext/plain; charset=us-asciiDownload
Index: src/backend/commands/trigger.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/trigger.c,v
retrieving revision 1.205
diff -c -r1.205 trigger.c
*** src/backend/commands/trigger.c 31 Jul 2006 20:09:00 -0000 1.205
--- src/backend/commands/trigger.c 2 Aug 2006 02:02:13 -0000
***************
*** 2090,2100 ****
--- 2090,2107 ----
/*
* Fetch the required OLD and NEW tuples.
*/
+ LocTriggerData.tg_trigtuple = NULL;
+ LocTriggerData.tg_newtuple = NULL;
+ LocTriggerData.tg_trigtuplebuf = InvalidBuffer;
+ LocTriggerData.tg_newtuplebuf = InvalidBuffer;
+
if (ItemPointerIsValid(&(event->ate_oldctid)))
{
ItemPointerCopy(&(event->ate_oldctid), &(oldtuple.t_self));
if (!heap_fetch(rel, SnapshotAny, &oldtuple, &oldbuffer, false, NULL))
elog(ERROR, "failed to fetch old tuple for AFTER trigger");
+ LocTriggerData.tg_trigtuple = &oldtuple;
+ LocTriggerData.tg_trigtuplebuf = oldbuffer;
}
if (ItemPointerIsValid(&(event->ate_newctid)))
***************
*** 2102,2107 ****
--- 2109,2124 ----
ItemPointerCopy(&(event->ate_newctid), &(newtuple.t_self));
if (!heap_fetch(rel, SnapshotAny, &newtuple, &newbuffer, false, NULL))
elog(ERROR, "failed to fetch new tuple for AFTER trigger");
+ if (LocTriggerData.tg_trigtuple)
+ {
+ LocTriggerData.tg_newtuple = &newtuple;
+ LocTriggerData.tg_newtuplebuf = newbuffer;
+ }
+ else
+ {
+ LocTriggerData.tg_trigtuple = &newtuple;
+ LocTriggerData.tg_trigtuplebuf = newbuffer;
+ }
}
/*
***************
*** 2112,2141 ****
event->ate_event & (TRIGGER_EVENT_OPMASK | TRIGGER_EVENT_ROW);
LocTriggerData.tg_relation = rel;
- switch (event->ate_event & TRIGGER_EVENT_OPMASK)
- {
- case TRIGGER_EVENT_INSERT:
- LocTriggerData.tg_trigtuple = &newtuple;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigtuplebuf = newbuffer;
- LocTriggerData.tg_newtuplebuf = InvalidBuffer;
- break;
-
- case TRIGGER_EVENT_UPDATE:
- LocTriggerData.tg_trigtuple = &oldtuple;
- LocTriggerData.tg_newtuple = &newtuple;
- LocTriggerData.tg_trigtuplebuf = oldbuffer;
- LocTriggerData.tg_newtuplebuf = newbuffer;
- break;
-
- case TRIGGER_EVENT_DELETE:
- LocTriggerData.tg_trigtuple = &oldtuple;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigtuplebuf = oldbuffer;
- LocTriggerData.tg_newtuplebuf = InvalidBuffer;
- break;
- }
-
MemoryContextReset(per_tuple_context);
/*
--- 2129,2134 ----
Michael Fuhr <mike@fuhr.org> writes:
Set tg_trigtuple/tg_newtuple in AFTER triggers according to whether
old and new tuples were supplied rather than blindly setting them
according to the event type. Per discussion in pgsql-hackers.
Looks good, applied.
regards, tom lane
On Thu, Aug 03, 2006 at 12:05:23PM -0400, Tom Lane wrote:
Michael Fuhr <mike@fuhr.org> writes:
Set tg_trigtuple/tg_newtuple in AFTER triggers according to whether
old and new tuples were supplied rather than blindly setting them
according to the event type. Per discussion in pgsql-hackers.Looks good, applied.
Thanks. Alvaro made the following suggestion but didn't copy the
list -- shall I do what he suggested and submit the changes as
another patch?
Alvaro Herrera wrote:
I'd add an Assert() on the second hunk to make sure newtuple is only set
in UPDATE. And also a comment on top of the "if" to explain why.
--
Michael Fuhr
Michael Fuhr <mike@fuhr.org> writes:
Thanks. Alvaro made the following suggestion but didn't copy the
list -- shall I do what he suggested and submit the changes as
another patch?
Alvaro Herrera wrote:
I'd add an Assert() on the second hunk to make sure newtuple is only set
in UPDATE. And also a comment on top of the "if" to explain why.
Can't get excited about that. Will you also have asserts to complain
if the wrong combinations of tuples are supplied for the other cases?
Is this really likely to catch anything? It's not like this function
is called from a variety of places.
While I was applying the patch I considered changing the
if (LocTriggerData.tg_trigtuple != NULL)
to
if ((event->ate_event & TRIGGER_EVENT_OPMASK) == TRIGGER_EVENT_UPDATE)
but this didn't seem to be an improvement on the whole, as it effectively
provides two ways to get it wrong (wrong tuple args OR wrong event)
instead of only one. I think driving the setup of the tuple fields
entirely off the provided tuple args is logically cleaner.
regards, tom lane