Horton

Re: generic plans and "initial" pruning

Tomas Vondra <tomas@vondra.me>

View thread View on postgr.es
From: Tomas Vondra <tomas@vondra.me>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Andres Freund <andres@anarazel.de>, Daniel Gustafsson <daniel@yesql.se>, David Rowley <dgrowleyml@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Thom Brown <thom@linux.com>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2024-12-05T13:53:09Z
Lists: pgsql-hackers 

On 12/5/24 07:53, Amit Langote wrote:
> On Thu, Dec 5, 2024 at 2:20 AM Tomas Vondra <tomas@vondra.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

Commits

  1. Stamp 19beta1.

  2. Revert "Don't lock partitions pruned by initial pruning"

  3. Ensure first ModifyTable rel initialized if all are pruned

  4. Fix bug in cbc127917 to handle nested Append correctly

  5. Remove unstable test suite added by 525392d57

  6. Don't lock partitions pruned by initial pruning

  7. Fix an oversight in cbc127917 to handle MERGE correctly

  8. Track unpruned relids to avoid processing pruned relations

  9. Perform runtime initial pruning outside ExecInitNode()

  10. Move PartitionPruneInfo out of plan nodes into PlannedStmt

  11. Fix setrefs.c's failure to do expression processing on prune steps.

  12. Remove obsolete executor cleanup code

  13. Revert "Move PartitionPruneInfo out of plan nodes into PlannedStmt"

  14. Move PartitioPruneInfo out of plan nodes into PlannedStmt

  15. Refactor and cleanup runtime partition prune code a little

  16. Remove some unnecessary fields from Plan trees.

  17. Remove more redundant relation locking during executor startup.

  18. Shut down Gather's children before shutting down Gather itself.