On Fri, May 23, 2025 at 12:08 AM Robert Haas <robertmh...@gmail.com> wrote: Thanks for your review!
> On Tue, May 20, 2025 at 9:18 AM torikoshia <torikos...@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. 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
