Re: generic plans and "initial" pruning

On Wed, Mar 22, 2023 at 9:48 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Tue, Mar 14, 2023 at 7:07 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Thu, Mar 2, 2023 at 10:52 PM Amit Langote <amitlangote09(at)gmail(dot)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.

Rebased over 3c05284d83b2 ("Invent GENERIC_PLAN option for EXPLAIN.").

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