MIME-Version: 1.0
References: <CA+HiwqFpZ80UJKr4tZus4Omgg7YESzFXKSwSHRW2Ap2=XSVyUA@mail.gmail.com>
 <202406191709.jbvpf7d7hl6g@alvherre.pgsql>
In-Reply-To: <202406191709.jbvpf7d7hl6g@alvherre.pgsql>
From: Amit Langote <amitlangote09@gmail.com>
Date: Mon, 12 Aug 2024 21:54:16 +0900
Message-ID: <CA+HiwqF+3tv=tuB9EVfOj9YcXhSq477X+1RKOpJ5JqCCj3qgww@mail.gmail.com>
Subject: Re: generic plans and "initial" pruning
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, 
	Daniel Gustafsson <daniel@yesql.se>, David Rowley <dgrowleyml@gmail.com>, 
	Jacob Champion <jchampion@timescale.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, 
	Thom Brown <thom@linux.com>, Tom Lane <tgl@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BHiwqF%2B3tv%3DtuB9EVfOj9YcXhSq477X%2B1RKOpJ5JqCCj3qgww%40mail.gmail.com>
Precedence: bulk

On Thu, Jun 20, 2024 at 2:09=E2=80=AFAM Alvaro Herrera <alvherre@alvh.no-ip=
.org> wrote:
> I hope we can get this new executor code in 18.

Thanks for doing the benchmark, Alvaro, and sorry for the late reply.

Yes, I'm hoping to get *some* version of this into v18.  I've been
thinking how to move this forward and I'm starting to think that we
should go back to or at least consider as an option the old approach
of changing the plancache to do the initial runtime pruning instead of
changing the executor to take locks, which is the design that the
latest patch set tries to implement.

Here are the challenges facing the implementation of the current design:

1. I went through many iterations of the changes to ExecInitNode() to
return a partially initialized PlanState tree when it detects that the
CachedPlan was invalidated after locking a child table and to
ExecEndNode() to account for the PlanState tree sometimes being
partially initialized, but it still seems fragile and bug-prone to me.
It might be because this approach is fundamentally hard to get right
or I haven't invested enough effort in becoming more confident in its
robustness.

2. Refactoring needed due to the ExecutorStart() API change especially
that pertaining to portals does not seem airtight.  I'm especially
worried about moving the ExecutorStart() call for the
PORTAL_MULTI_QUERY case from where it is currently to PortalStart().
That requires additional bookkeeping in PortalData and I am not
totally sure that the snapshot handling changes after that move are
entirely correct.

3. The need to add *back* the fields to store the RT indexes of
relations that are not looked at by ExecInitNode() traversal such as
root partitioned tables and non-leaf partitions.

I'm worried about #2 the most.  One complaint about the previous
design was that the interface changes to capture and pass the result
of doing initial pruning in plancache.c to the executor did not look
great.  However, after having tried doing #2, the changes to pass the
pruning result into the executor and changes to reuse it in
ExecInit[Merge]Append() seem a tad bit simpler than the refactoring
and adjustments needed to handle failed ExecutorStart() calls, at
multiple code sites.

About #1, I tend to agree with David that adding complexity around
PlanState tree construction may not be a good idea, because we might
want to rethink Plan initialization code and data structures in the
not too distant future.  One idea I thought of is to take the
remaining locks (to wit, those on inheritance children if running a
cached plan) at the beginning of InitPlan(), that is before
ExecInitNode(), like we handle the permission checking, so that we
don't need to worry about ever returning a partially initialized
PlanState tree.  However, we're still left with the tall task to
implement #2 such that it doesn't break anything.

Another concern about the old design was the unnecessary overhead of
initializing bitmapset fields in PlannedStmt that are meant for the
locking algorithm in AcquireExecutorLocks().  Andres suggested an idea
offlist to either piggyback on cursorOptions argument of
pg_plan_queries() or adding a new boolean parameter to let the planner
know if the plan is one that might get cached and thus have
AcquireExecutorLocks() called on it.  Another idea David and I
discussed offlist is inventing a RTELockInfo (cf RTEPermissionInfo)
and only creating one for each RT entry that is un-prunable and do
away with PlannedStmt.rtable.  For partitioned tables, that entry will
point to the PartitionPruneInfo that will contain the RT indexes of
partitions (or maybe just OIDs) mapped from their subplan indexes that
are returned by the pruning code.  So AcquireExecutorLocks() will lock
all un-prunable relations by referring to their RTELockInfo entries
and for each entry that points to a PartitionPruneInfo with initial
pruning steps, will only lock the partitions that survive the pruning.

I am planning to polish that old patch set and post after playing with
those new ideas.

--
Thanks, Amit Langote