Re: enable/disable trigger (Re: Fwd: [HACKERS] Open items)

On Fri, 1 Jul 2005, Satoshi Nagayasu wrote:

> Hi all,
>
> Here is a first patch to allow these commands.
>
> > ALTER TABLE <table> ENABLE TRIGGER <trigname>
> > ALTER TABLE <table> DISABLE TRIGGER <trigname>

There are three other areas which are worth looking at:

a) We may defer the execution of some triggers to the end of the
transaction. Do we execute those if a they were later disabled?

b) There is a bug in how we execute triggers. For example, in
ExecDelete():

bool dodelete;

dodelete = ExecBRDeleteTriggers(estate, resultRelInfo, tupleid,
estate->es_snapshot->curcid);

if (!dodelete) /* "do nothing" */
return;

This means that if a before trigger returned NULL, we short circuit and do
not delete the tuple. Consider the following in ExecBRDeleteTriggers()

HeapTuple newtuple = NULL;

...

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

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

...

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

This means that if all triggers on a table are disabled, we tell the
caller that a trigger returned NULL and that we should short circuit. This
does not seem to be the case for the other DML statements.

c) There has been a push over previous releases to make dumps generated by
pg_dump look like ANSI SQL. Now, ALTER TABLE ... DISABLE trigger is useful
for pg_dump but not compliant. Others have suggested something like:

SET enable_triggers = off

This would turn all triggers off in the current session. It has the added
benefit that it does not affect other sessions. It does introduce some
issues with permissions -- I wouldn't want users turning off data
validation before triggers in my database, but that's me. I'm not
enamoured of the idea but it is worth discussing, I believe.

Also, a final patch will also need documentation and regression tests :-)

Thanks,

Gavin