MIME-Version: 1.0
References: <CA+HiwqFGkMSge6TgC9KQzde0ohpAycLQuV7ooitEEpbKB0O_mg@mail.gmail.com>
 <CA+TgmoZ=92LxPeVJ20vf4vJ4o8dr7Q2seDQR=xLY04puLjDs_A@mail.gmail.com>
 <CA+HiwqEAdv+1t+v6pkcH9pgsSXnBeHLchDuGRwXP-oEB4BCPmg@mail.gmail.com>
 <CA+TgmobN=+AoDE4JZioVpaRXJVCQ2Fa9Uw3TjC-OqnXWQ0uq1w@mail.gmail.com>
 <CA+HiwqFOsJ21nmvoKtPkzxKfYzACajj-mAOoJ+-y_O6g+-v-aA@mail.gmail.com>
 <CA+TgmoanT5K44bHZqMXxjAKwyF7nhajKBrw-p-+HSyEzOVqX1w@mail.gmail.com>
 <CA+HiwqGbnNGttKr8fK2pAYcC1JAapo=1nXO7OQs2M7+HmoVVpw@mail.gmail.com>
 <CA+HiwqEYCpEqh2LMDOp9mT+4-QoVe8HgFMKBjntEMCTZLpcCCA@mail.gmail.com>
 <CA+TgmoYbpAC4fDWLBSg6MZ9SymyKrt3NSc42whin9Qbgm71nCQ@mail.gmail.com>
 <215356.1647286703@sss.pgh.pa.us> <CA+Tgmoakey5yO+TTp0Gntz5MdnMLhd+Ei-+HWuCszHJeVU_YVA@mail.gmail.com>
 <CA+HiwqEDuz6OaM0Rv2D6kXgK0Cusr7HtquZTPjan+U76AJm0pg@mail.gmail.com>
 <CA+HiwqH9-fAvpG-w9qYCcDWzK3vGPCMyw4f9nHzqkxXVuD1pxw@mail.gmail.com>
 <CA+HiwqFq9UQiSJbGpJkmd0MkxqQLZWGk1+__tyk0QM+hmYHrrA@mail.gmail.com>
 <CA+HiwqERQkaKkE8TwNmFP2SDBykttAqBkwh_Niif9FqST7F4Kg@mail.gmail.com>
 <CA+HiwqGoORM+yfm8PhGWfHNUSBbSUgTQC=s5ieV8b4FX1yDBrg@mail.gmail.com>
 <CAApHDvp_DjVVkgSV24+UF7p_yKWeepgoo+W2SWLLhNmjwHTVYQ@mail.gmail.com>
 <CA+HiwqH7pYXfPZhHCKTmb_=C0z538wP8Mog+_j+vTi7JCVqnhQ@mail.gmail.com> <CAApHDvrpWWtSRZXFO31H1KcC9csKuE31v2f7k6rVS9Yp5ibc7Q@mail.gmail.com>
In-Reply-To: <CAApHDvrpWWtSRZXFO31H1KcC9csKuE31v2f7k6rVS9Yp5ibc7Q@mail.gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Fri, 1 Apr 2022 15:58:13 +0900
Message-ID: <CA+HiwqEhn35nnFqv-0-T8mO1=ug7VcU2_3LnnToT2PzCNw9+4w@mail.gmail.com>
Subject: Re: generic plans and "initial" pruning
To: David Rowley <dgrowleyml@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CA%2BHiwqEhn35nnFqv-0-T8mO1%3Dug7VcU2_3LnnToT2PzCNw9%2B4w%40mail.gmail.com>
Precedence: bulk

On Fri, Apr 1, 2022 at 1:08 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Fri, 1 Apr 2022 at 16:09, Amit Langote <amitlangote09@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