Heikki Linnakangas <heikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers