Minor BEFORE DELETE trigger fix

Can I get a context diff please?

---------------------------------------------------------------------------

Gavin Sherry wrote:

Attached is a minor patch to make BEFORE DELETE triggers honour tgenabled
properly.

I know that we cannot, currently, use this feature through a DDL command
but just in case someone is updating the catalogs to do it and since it is
necessary in order to implement disabling of triggers, I thought I'd send
it in.

Gavin

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 3: 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

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Oops.

Attached in the usual format this time.

Gavin

On Fri, 6 Aug 2004, Bruce Momjian wrote:

Gavin Sherry <swm@linuxworld.com.au> writes:

Attached in the usual format this time.

AFAICS this patch makes exactly zero change in behavior. What was
the point again?

Also, if there is a point, why are we changing only one of the
several ExecFOOTriggers functions?

regards, tom lane

On Sat, 7 Aug 2004, Tom Lane wrote:

Gavin Sherry <swm@linuxworld.com.au> writes:

Attached in the usual format this time.

AFAICS this patch makes exactly zero change in behavior. What was
the point again?

With BEFORE DELETE triggers, if the trigger returns NULL, then the DELETE
will not take place. The following is the existing code:

for (i = 0; i < ntrigs; i++)
{
Trigger *trigger = &trigdesc->triggers[tgindx[i]];

if (!trigger->tgenabled)
continue;
LocTriggerData.tg_trigtuple = trigtuple;
LocTriggerData.tg_trigger = trigger;
newtuple = ExecCallTriggerFunc(&LocTriggerData,
relinfo->ri_TrigFunctions + tgindx[i],
GetPerTupleMemoryContext(estate));
if (newtuple == NULL)
break;
if (newtuple != trigtuple)
heap_freetuple(newtuple);
}
heap_freetuple(trigtuple);

return (newtuple == NULL) ? false : true;

Now, if for all the triggers on the base relation, !trigger->tgenabled is
true, then newtuple will always be NULL.

At the moment, this situation shouldn't come up. But it will when we
support DISABLE trigger. Arul, from Fujitsu, is planning to implement that
for 8.1 so I thought I'd ease the way.

Also, if there is a point, why are we changing only one of the
several ExecFOOTriggers functions?

Because only BEFORE DELETE worries about trigger procedures returning
NULL, from memory.

Thanks,

Gavin

Gavin Sherry <swm@linuxworld.com.au> writes:

Now, if for all the triggers on the base relation, !trigger->tgenabled is
true, then newtuple will always be NULL.

Hmm ... seems like the all-triggers-disabled case should act the same as
the no-triggers-at-all case, which this doesn't seem to do. It does
look broken but this doesn't seem like the correct fix.

Also, if there is a point, why are we changing only one of the
several ExecFOOTriggers functions?

Because only BEFORE DELETE worries about trigger procedures returning
NULL, from memory.

Do I have to do more than raise my eyebrow here?

regards, tom lane

Did this get resolved?

---------------------------------------------------------------------------

Gavin Sherry wrote:

Attached is a minor patch to make BEFORE DELETE triggers honour tgenabled
properly.

I know that we cannot, currently, use this feature through a DDL command
but just in case someone is updating the catalogs to do it and since it is
necessary in order to implement disabling of triggers, I thought I'd send
it in.

Gavin

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 3: 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

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Thu, 12 Aug 2004, Bruce Momjian wrote:

Did this get resolved?

---------------------------------------------------------------------------

Gavin Sherry wrote:

Attached is a minor patch to make BEFORE DELETE triggers honour tgenabled
properly.

I know that we cannot, currently, use this feature through a DDL command
but just in case someone is updating the catalogs to do it and since it is
necessary in order to implement disabling of triggers, I thought I'd send
it in.

Gavin

After taking a proper look, I agree with Tom that my patch was not the
proper solution to the problem. What we really need for the BEFORE
ROW triggers is the ability to say that there were no triggers executeed.
At the moment, if no triggers are executed (ie, they're all disabled),
then the executor thinks that a trigger returned NULL.

I'll try to find some time to fix this soon. As I noted, though, its not
critical because there's no DDL to disable a trigger.

Gavin