Re: wCTE: why not finish sub-updates at the end, not the beginning?

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> On 26.02.2011 07:55, Tom Lane wrote:
>> So we really need some refactoring here. I dislike adding another
>> fundamental step to the ExecutorStart/ExecutorRun/ExecutorEnd sequence,
>> but there may not be a better way.

> Could you keep the sequence unchanged for non-EXPLAIN callers with some
> refactoring? Add an exposed function like ExecutorFinishRun() that
> Explain calls explicitly in the EXPLAIN ANALYZE case, and modify
> ExecutorEnd to call it too, if it hasn't been called yet and the
> explain-only flag isn't set.

I had been toying with the same idea, but it doesn't work, as Dean
Rasheed points out nearby. The full monty for running the executor
these days is really

CreateQueryDesc(...);
AfterTriggerBeginQuery(...);
ExecutorStart(...);
ExecutorRun(...); // zero or more times
AfterTriggerEndQuery(...);
ExecutorEnd(...);
FreeQueryDesc(...);

ExecutorEnd can *not* do anything that might fire triggers, because it's
too late: AfterTriggerEndQuery has already been called.

BTW, there are various places that believe they can skip
AfterTriggerBeginQuery/AfterTriggerEndQuery if operation == CMD_SELECT,
an assumption that no longer holds if the query has wCTEs. So we have
some work to do on the callers no matter what.

I'm inclined to think that it would be best to move the responsibility
for calling AfterTriggerBeginQuery/AfterTriggerEndQuery into the
executor. That would get us down to

CreateQueryDesc(...);
ExecutorStart(...); // now includes AfterTriggerBeginQuery
ExecutorRun(...); // zero or more times
ExecutorFinish(...); // ModifyTable cleanup, AfterTriggerEndQuery
ExecutorEnd(...); // just does what it's always done
FreeQueryDesc(...);

where EXPLAIN without ANALYZE would skip ExecutorRun and ExecutorFinish.

The RI triggers have a requirement for being able to run this sequence
without the AfterTriggerBeginQuery/AfterTriggerEndQuery calls (cf
SPI_execute_snapshot's fire_triggers parameter), but we could support
that by adding an ExecutorStart flag that tells it to suppress those
trigger calls.

IMO the major disadvantage of a refactoring like this is the possibility
of sins of omission in third-party code, in particular somebody not
noticing the added requirement to call ExecutorFinish. We could help
them out by adding an Assert in ExecutorEnd to verify that
ExecutorFinish had been called (unless explain-only mode). A variant of
that problem is an auto_explain-like add-on not noticing that they
probably want to hook into ExecutorFinish if they'd previously been
hooking ExecutorRun. I don't see any simple check for that though.
The other possible failure mode is forgetting to remove calls to the two
trigger functions, but we could encourage getting that right by renaming
those two functions.

Comments?

regards, tom lane