On Thu, Dec 5, 2024 at 3:53 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Dec 5, 2024 at 2:20 AM Tomas Vondra <tomas@vondra.me> wrote:
> > Sure, changing the APIs is allowed, I'm just wondering if maybe there
> > might be a way to not have this issue, or at least notice the missing
> > call early.
> >
> > I haven't tried, wouldn't it be better to modify ExecutorStart() to do
> > the retries internally? I mean, the extensions wouldn't need to check if
> > the plan is still valid, ExecutorStart() would take care of that. Yeah,
> > it might need some new arguments, but that's more obvious.
>
> One approach could be to move some code from standard_ExecutorStart()
> into ExecutorStart(). Specifically, the code responsible for setting
> up enough state in the EState to perform ExecDoInitialPruning(), which
> takes locks that might invalidate the plan. If the plan does become
> invalid, the hook and standard_ExecutorStart() are not called.
> Instead, the caller, ExecutorStartExt() in this case, creates a new
> plan.
>
> This avoids the need to add ExecPlanStillValid() checks anywhere,
> whether in core or extension code. However, it does mean accessing the
> PlannedStmt earlier than InitPlan(), but the current placement of the
> code is not exactly set in stone.
I tried this approach and found that it essentially disables testing
of this patch using the delay_execution module, which relies on the
ExecutorStart_hook(). The way the testing works is that the hook in
delay_execution.c pauses the execution of a cached plan to allow a
concurrent session to drop an index referenced in the plan. When
unpaused, execution initialization resumes by calling
standard_ExecutorStart(). At this point, obtaining the lock on the
partition whose index has been dropped invalidates the plan, which the
hook detects and reports. It then also reports the successful
re-execution of an updated plan that no longer references the dropped
index. Hmm.
--
Thanks, Amit Langote
