Re: generic plans and "initial" pruning

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

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

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.

regards

--
Tomas Vondra