Hi folks,
apologies for not replying earlier. Thanks torikoshia for having
reviewed the related patch I sent that includes instrumentation.
It makes total sense to focus on this one first.
Been thinking about the current strategy of having to iterate through
the execution tree to add the custom ExecProcNode to avoid logging
the plan in CHECK_FOR_INTERRUPTS(). I see you folks are still
discussing all the nuances in the recursive tree traversal function.
Taking a step back and proposing a different approach, have we thought
about logging the query plan in the regions of the code related to the
query executor, NEXT to CHECK_FOR_INTERRUPTS calls instead of IN
that function?
For example, in this part of executor/nodeHashjoin.c:
for (;;)
{
/*
* It's possible to iterate this loop many times before returning a
* tuple, in some pathological cases such as needing to move much of
* the current batch to a later batch. So let's check for interrupts
* each time through.
*/
CHECK_FOR_INTERRUPTS();
We replace CHECK_FOR_INTERRUPTS() for a new function that does
query plan logging logic + CHECK_FOR_INTERRUPTS().
Would that be considered a safe operation given that we would always be
in the query execution context? The current ExecProcNode wrapper strategy
logs the query plan in ExecProcNodeFirst(). That function will then call
the real ExecProcNode, which points to one of the functions that contain
the CHECK_FOR_INTERRUPTS in the first place. I don't see much difference
in terms of logging safety between the two approaches, but maybe there
is :)
Rafael.
