Re: generic plans and "initial" pruning

On Tue, Mar 14, 2023 at 7:07 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Mar 2, 2023 at 10:52 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > I think I have figured out what might be going wrong on that cfbot
> > animal after building with the same CPPFLAGS as that animal locally.
> > I had forgotten to update _out/_readRangeTblEntry() to account for the
> > patch's change that a view's RTE_SUBQUERY now also preserves relkind
> > in addition to relid and rellockmode for the locking consideration.
> >
> > Also, I noticed that a multi-query Portal execution with rules was
> > failing (thanks to a regression test added in a7d71c41db) because of
> > the snapshot used for the 2nd query onward not being updated for
> > command ID change under patched model of multi-query Portal execution.
> > To wit, under the patched model, all queries in the multi-query Portal
> > case undergo ExecutorStart() before any of it is run with
> > ExecutorRun().  The patch hadn't changed things however to update the
> > snapshot's command ID for the 2nd query onwards, which caused the
> > aforementioned test case to fail.
> >
> > This new model does however mean that the 2nd query onwards must use
> > PushCopiedSnapshot() given the current requirement of
> > UpdateActiveSnapshotCommandId() that the snapshot passed to it must
> > not be referenced anywhere else.  The new model basically requires
> > that each query's QueryDesc points to its own copy of the
> > ActiveSnapshot.  That may not be a thing in favor of the patched model
> > though.  For now, I haven't been able to come up with a better
> > alternative.
>
> Here's a new version addressing the following 2 points.
>
> * Like views, I realized that non-leaf relations of partition trees
> scanned by an Append/MergeAppend would need to be locked separately,
> because ExecInitNode() traversal of the plan tree would not account
> for them.  That is, they are not opened using
> ExecGetRangeTableRelation() or ExecOpenScanRelation().  One exception
> is that some (if not all) of those non-leaf relations may be
> referenced in PartitionPruneInfo and so locked as part of initializing
> the corresponding PartitionPruneState, but I decided not to complicate
> the code to filter out such relations from the set locked separately.
> To carry the set of relations to lock, the refactoring patch 0001
> re-introduces the List of Bitmapset field named allpartrelids into
> Append/MergeAppend nodes, which we had previously removed on the
> grounds that those relations need not be locked separately (commits
> f2343653f5b, f003a7522bf).
>
> * I decided to initialize QueryDesc.planstate even in the cases where
> ExecInitNode() traversal is aborted in the middle on detecting
> CachedPlan invalidation such that it points to a partially initialized
> PlanState tree.  My earlier thinking that each PlanState node need not
> be visited for resource cleanup in such cases was naive after all.  To
> that end, I've fixed the ExecEndNode() subroutines of all Plan node
> types to account for potentially uninitialized fields.  There are a
> couple of cases where I'm a bit doubtful though.  In
> ExecEndCustomScan(), there's no indication in CustomScanState whether
> it's OK to call EndCustomScan() when BeginCustomScan() may not have
> been called.  For ForeignScanState, I've assumed that
> ForeignScanState.fdw_state being set can be used as a marker that
> BeginForeignScan would have been called, though maybe that's not a
> solid assumption.
>
> I'm also attaching a new (small) patch 0003 that eliminates the
> loop-over-rangetable in ExecCloseRangeTableRelations() in favor of
> iterating over a new List field of EState named es_opened_relations,
> which is populated by ExecGetRangeTableRelation() with only the
> relations that were opened.  This speeds up
> ExecCloseRangeTableRelations() significantly for the cases with many
> runtime-prunable partitions.

Here's another version with some cosmetic changes, like fixing some
factually incorrect / obsolete comments and typos that I found.  I
also noticed that I had missed noting near some table_open() that
locks taken with those can't possibly invalidate a plan (such as
lazily opened partition routing target partitions) and thus need the
treatment that locking during execution initialization requires.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com