Re: RFC: Logging plan of the running query

On Tue, May 20, 2025 at 9:18 AM torikoshia <torikoshia@oss.nttdata.com> wrote:
> I tackled this again and the attached patch removes ExecProcNodeOriginal
> from Planstate.
> Instead of adding a new field, this version builds the behavior into the
> existing wrapper function, ExecProcNodeFirst().
>
> Since ExecProcNodeFirst() is already handling instrumentation-related
> logic, the patch has maybe become a bit more complex to accommodate both
> that and the new behavior.
>
> While it might make sense to introduce a more general mechanism that
> allows for stacking an arbitrary number of wrappers around ExecProcNode,
> I’m not sure it's possible or worth the added complexity—such layered
> wrapping doesn't seem like something we typically need.
>
> What do you think?

Hmm, I'm not convinced that this is correct. If
GetProcessLogQueryPlanInterruptActive() is true but node->instrument
is also non-NULL, your implementation of ExecProcNodeFirst() will
handle the first but ignore the second. Plus, I don't understand why
ExecProcNodeFirst() does node->ExecProcNode = ExecProcNodeWithExplain
instead of just directly doing the necessary work. It seems to me that
this will result in the first call to ExecProcNode calling
ExecProcNodeFirst to install ExecProcNodeWithExplain; and then the
second call to ExecProcNode will call ExecProcNodeWithExplain which
will actually do the work. But this seems unnecessary to me: I think
it could just say if (GetProcessLogQueryPlanInterruptActive())
LogQueryPlan(ps).

Backing up a step, I think you've got a good idea here in thinking
that we can probably reuse ExecProcNodeFirst for this purpose. It
appears to me that it is always valid to reset a node's ExecProcNode
pointer back to ExecProcNodeFirst. It might be unnecessary and cost
some performance to do it when not required, but it is safe, because
ExecProcNodeFirst will simply reset node->ExecProcNode and then call
the appropriate function. So what we can do is just add the additional
logic to check whether we need to print the query plan after the
check_stack_depth() call and before the rest of the logic in the
function. I've attached a sample patch to show what I have in mind.

-- 
Robert Haas
EDB: http://www.enterprisedb.com