Re: generic plans and "initial" pruning - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: generic plans and "initial" pruning |
Date | |
Msg-id | CA+HiwqHQ1PM+HXoEdvutj0huhu2cfmuPa8wtctor0NNADzZVvA@mail.gmail.com Whole thread Raw |
In response to | Re: generic plans and "initial" pruning (Amit Langote <amitlangote09@gmail.com>) |
Responses |
Re: generic plans and "initial" pruning
(Amit Langote <amitlangote09@gmail.com>)
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: