On Fri, Apr 1, 2022 at 1:08 PM David Rowley <dgrowle...@gmail.com> wrote: > On Fri, 1 Apr 2022 at 16:09, Amit Langote <amitlangot...@gmail.com> wrote: > > definition of PlannedStmt says this: > > > > /* ---------------- > > * PlannedStmt node > > * > > * The output of the planner > > > > With the ideas that you've outlined below, perhaps we can frame most > > of the things that the patch wants to do as the planner and the > > plancache changes. If we twist the above definition a bit to say what > > the plancache does in this regard is part of planning, maybe it makes > > sense to add the initial pruning related fields (nodes, outputs) into > > PlannedStmt. > > How about the PartitionPruneInfos go into PlannedStmt as a List > indexed in the way I mentioned and the cache of the results of pruning > in EState? > > I think that leaves you adding List *partpruneinfos, Bitmapset > *minimumlockrtis to PlannedStmt and the thing you have to cache the > pruning results into EState. I'm not very clear on where you should > stash the results of run-time pruning in the meantime before you can > put them in EState. You might need to invent some intermediate struct > that gets passed around that you can scribble down some details you're > going to need during execution.
Yes, the ExecLockRelsInfo node in the current patch, that first gets added to the QueryDesc and subsequently to the EState of the query, serves as that stashing place. Not sure if you've looked at ExecLockRelInfo in detail in your review of the patch so far, but it carries the initial pruning result in what are called PlanInitPruningOutput nodes, which are stored in a list in ExecLockRelsInfo and their offsets in the list are in turn stored in an adjacent array that contains an element for every plan node in the tree. If we go with a PlannedStmt.partpruneinfos list, then maybe we don't need to have that array, because the Append/MergeAppend nodes would be carrying those offsets by themselves. Maybe a different name for ExecLockRelsInfo would be better? Also, given Tom's apparent dislike for carrying that in PlannedStmt, maybe the way I have it now is fine? > > One question is whether the planner should always pay the overhead of > > initializing this bitmapset? I mean it's only worthwhile if > > AcquireExecutorLocks() is going to be involved, that is, the plan will > > be cached and reused. > > Maybe the Bitmapset for the minimal locks needs to be built with > bms_add_range(NULL, 0, list_length(rtable)); then do > bms_del_members() on the relevant RTIs you find in the listed > PartitionPruneInfos. That way it's very simple and cheap to do when > there are no PartitionPruneInfos. Ah, okay. Looking at make_partition_pruneinfo(), I think I see a way to delete the RTIs of prunable relations -- construct a all_matched_leaf_part_relids in parallel to allmatchedsubplans and delete those from the initial set. > > > 4. It's a bit disappointing to see RelOptInfo.partitioned_rels getting > > > revived here. Why don't you just add a partitioned_relids to > > > PartitionPruneInfo and just have make_partitionedrel_pruneinfo build > > > you a Relids of them. PartitionedRelPruneInfo already has an rtindex > > > field, so you just need to bms_add_member whatever that rtindex is. > > > > Hmm, not all Append/MergeAppend nodes in the plan tree may have > > make_partition_pruneinfo() called on them though. > > For Append/MergeAppends without run-time pruning you'll want to add > the RTIs to the minimal locking set of RTIs to go into PlannedStmt. > The only things you want to leave out of that are RTIs for the RTEs > that you might run-time prune away during AcquireExecutorLocks(). Yeah, I see it now. Thanks. -- Amit Langote EDB: http://www.enterprisedb.com
