Re: generic plans and "initial" pruning

On Thu, Dec 5, 2024 at 10:53 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> On 12/5/24 07:53, Amit Langote wrote:
> > On Thu, Dec 5, 2024 at 2:20 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> >> ...
> >>
> >>>> What if an
> >>>> extension doesn't do that? What weirdness will happen?
> >>>
> >>> The QueryDesc.planstate won't contain a PlanState tree for starters
> >>> and other state information that InitPlan() populates in EState based
> >>> on the PlannedStmt.
> >>
> >> OK, and the consequence is that the query will fail, right?
> >
> > No, the core executor will retry the execution with a new updated
> > plan. In the absence of the early return, the extension might even
> > crash when accessing such incomplete QueryDesc.
> >
> > What the patch makes the ExecutorStart_hook do is similar to how
> > InitPlan() will return early when locks taken on partitions that
> > survive initial pruning invalidate the plan.
>
> Isn't that what I said? My question was what happens if the extension
> does not add the new ExecPlanStillValid() call - sorry if that wasn't
> clear. If it can crash, that's what I meant by "fail".

Ok, I see. So, I suppose you meant to confirm if the invalid plan
won't silently be executed returning wrong results. Yes, I don't
think that would happen given the kinds of invalidations that are
possible. The various checks in the ExecInitNode() path, such as the
one that catches a missing index, will prevent the plan from running.
I may not have searched exhaustively enough though.

> >>>> Maybe it'd be
> >>>> possible to at least check this in some other executor hook? Or at least
> >>>> we could ensure the check was done in assert-enabled builds? Or
> >>>> something to make extension authors aware of this?
> >>>
> >>> I've added a note in the commit message, but if that's not enough, one
> >>> idea might be to change the return type of ExecutorStart_hook so that
> >>> the extensions that implement it are forced to be adjusted. Say, from
> >>> void to bool to indicate whether standard_ExecutorStart() succeeded
> >>> and thus created a "valid" plan. I had that in the previous versions
> >>> of the patch. Thoughts?
> >>
> >> Maybe. My concern is that this case (plan getting invalidated) is fairly
> >> rare, so it's entirely plausible the extension will seem to work just
> >> fine without the code update for a long time.
> >
> > You might see the errors like the one below when the core executor or
> > a hook tries to initialize or process in some other way a known
> > invalid plan, for example, because an unpruned partition's index got
> > concurrently dropped before the executor got the lock:
> >
> > ERROR: could not open relation with OID xxx
>
> Yeah, but how likely is that? How often get plans invalidated in regular
> application workload. People don't create or drop indexes very often,
> for example ...

Yeah, that's a valid point. Andres once mentioned that ANALYZE can
invalidate plans and that can occur frequently in busy systems.

> Again, I'm not saying requiring the call would be unacceptable, I'm sure
> we made similar changes in the past. But if it wasn't needed without too
> much contortion, that would be nice.

I tend to agree.

Another change introduced by the patch that extensions might need to
mind (noted in the commit message of v58-0004) is the addition of the
es_unpruned_relids field to EState. This field tracks the RT indexes
of relations that are locked and therefore safe to access during
execution. Importantly, it does not include the RT indexes of leaf
partitions that are pruned during "initial" pruning and thus remain
unlocked.

This change means that executor extensions can no longer assume that
all relations in the range table are locked and safe to access.
Instead, extensions must account for the possibility that some
relations, specifically pruned partitions, are not locked. Normally,
executor code accesses relations using ExecGetRangeTableRelation(),
which does not take a lock before returning the Relation pointer,
assuming that locks are already managed upstream.

--
Thanks, Amit Langote