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. 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
example-execprocnodefirst-patch.diff
Description: Binary data
