Re: generic plans and "initial" pruning

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.

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