Re: RFC: Logging plan of the running query

On Fri, May 23, 2025 at 12:08 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
Thanks for your review!

> On Tue, May 20, 2025 at 9:18 AM torikoshia <torikoshia(at)oss(dot)nttdata(dot)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.
In that case, the patch performs instrumentation during unwrapping --
specifically when executing UnwrapExecProcNodeWithExplain() -- so that
the query plan can still be logged for statements like "EXPLAIN
ANALYZE SELECT ..".
However, I admit this isn’t a good implementation: it’s hard to follow.

> Plus, I don't understand why
> ExecProcNodeFirst() does node->ExecProcNode = ExecProcNodeWithExplain
> instead of just directly doing the necessary work.
Indeed!

> 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.

Thanks for the idea and the sample patch!
Agreed. I’ll go ahead and implement a new patch based on this approach.

--
Atsushi Torikoshi