Re: generic plans and "initial" pruning

On Thu, Dec 12, 2024 at 4:58 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> To summarize how extensions can be affected:
>
> 1. Plan invalidation during standard_ExecutorStart(): A plan tree
> originating from a CachedPlan can become invalid during
> standard_ExecutorStart() due to locks taken on leaf partitions that
> survive initial pruning. Extensions should be updated to handle this
> scenario by checking ExecPlanStillValid(estate) immediately after
> calling standard_ExecutorStart() in their ExecutorStart_hook. If it
> returns false, the extensions should avoid further processing.
>
> 2. Validation of RT indexes: If the plan tree remains valid, any
> direct manipulation of relations using RT indexes must first verify
> that the RT index is present in the EState.es_unpruned_relids
> bitmapset. This bitmapset includes: a) RT indexes of relations that
> are originally unprunable (and locked during GetCachedPlan()), and
> b) RT indexes of leaf partitions that survive initial partition
> pruning. This step is crucial because pruned relations are not locked.
> Additionally, with the update in 0004, attempting to open pruned
> relations using ExecGetRangeTableRelation() will result in an error.
>
> I’d love to hear from anyone maintaining executor hooks, such as those
> from Timescale, Citus, or other extension developers. Please give this
> patch set (0001-0004) a try and let me know if you run into any issues
> or have feedback. 0005 is a sketch of an approach that eliminates the
> need for extensions to check ExecPlanStillValid() in their
> ExecutorStart_hook.

I’ve rebased over recent changes to setrefs.c (commit bf826ea0629).
During the rebase, I realized that the patch
0002-Initialize-PartitionPruneContexts-lazily wasn’t a good idea after
all.

The test case added by bf826ea0629 highlighted an issue: initializing
pruning expressions lazily during execution could leave the
Append/MergeAppend node’s PlanState.subPlan uninitialized at
ExecInitNode() time. Initially, I thought this would have only
cosmetic consequences -- such as changes in test case output where
SubPlans referenced in "exec" pruning expressions wouldn’t appear --
but I may have underestimated the problem. As a result, I’ve abandoned
that approach and the patch in favor of initializing all pruning
expressions during plan initialization.

Additionally, I revisited the impact of the main patch on
ExecutorStart_hooks. It seems better to change the return type from
void to bool, returning the result of
ExecPlanStillValid(queryDesc->estate). This change has the added
benefit of breaking extensions that use ExecutorStart_hook at compile
time, encouraging authors to update their code. The updated commit
message includes details on additional checks extensions must
implement, particularly for cases where they might access pruned and
thus unlocked relations.

I've stared at the refactoring patches 0001 and 0002 for long enough
at this point that I'd like to commit them early next week, barring
further comments or objections. I'll keep staring at 0003.

--
Thanks, Amit Langote