MIME-Version: 1.0
References: <CA+HiwqFpZ80UJKr4tZus4Omgg7YESzFXKSwSHRW2Ap2=XSVyUA@mail.gmail.com>
 <202406191709.jbvpf7d7hl6g@alvherre.pgsql> <CA+HiwqF+3tv=tuB9EVfOj9YcXhSq477X+1RKOpJ5JqCCj3qgww@mail.gmail.com>
 <CA+TgmobHL_vTjOdy6KVMVeW-CQQmXXz_yU6Q9d2YjnVfFxuy6A@mail.gmail.com>
 <CA+HiwqHL=aGU9Y4RYXQ5VCp4L5NVdiaQLLoXN3NCQQQMKo0ByQ@mail.gmail.com>
 <CA+TgmoabYD=pnccFLzbbREFsqkFgE4EZ+FdHoTOhgCqn4jP2Cw@mail.gmail.com>
 <CA+HiwqE_rQ9pZnkXeoHdds2kgAiT7XNNHZW8gTGicBdXv0rwnw@mail.gmail.com>
 <CA+TgmoY2drv9PmrRAC7AR77mkx09sOh-+5qJkHB_hLKeHRNqzQ@mail.gmail.com>
 <CA+HiwqHkjicWzfAjB6_SVsVmKF6omQ4EBHr+GTUgJNN7WiUDag@mail.gmail.com> <CA+TgmoaZxb4JTimK8MmbXEeCwtzyfx7uGYjq565s2pY9i1GN+Q@mail.gmail.com>
In-Reply-To: <CA+TgmoaZxb4JTimK8MmbXEeCwtzyfx7uGYjq565s2pY9i1GN+Q@mail.gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Wed, 21 Aug 2024 21:45:24 +0900
Message-ID: <CA+HiwqHzKO9FT-CjFWo6OmkiCSYmbPspKXVex96tOBKf6S_x_w@mail.gmail.com>
Subject: Re: generic plans and "initial" pruning
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Andres Freund <andres@anarazel.de>, 
	Daniel Gustafsson <daniel@yesql.se>, David Rowley <dgrowleyml@gmail.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%2BHiwqHzKO9FT-CjFWo6OmkiCSYmbPspKXVex96tOBKf6S_x_w%40mail.gmail.com>
Precedence: bulk

On Tue, Aug 20, 2024 at 11:53=E2=80=AFPM Robert Haas <robertmhaas@gmail.com=
> wrote:
> On Tue, Aug 20, 2024 at 9:00=E2=80=AFAM Amit Langote <amitlangote09@gmail=
.com> wrote:
> > I think we'd modify plancache.c to postpone the locking of only
> > prunable relations (i.e., partitions), so we're looking at only a
> > handful of concurrent modifications that are going to cause execution
> > errors.  That's because we disallow many DDL modifications of
> > partitions unless they are done via recursion from the parent, so the
> > space of errors in practice would be smaller compared to if we were to
> > postpone *all* cached plan locks to ExecInitNode() time.  DROP INDEX
> > a_partion_only_index comes to mind as something that might cause an
> > error.  I've not tested if other partition-only constraints can cause
> > unsafe behaviors.
>
> This seems like a valid point to some extent, but in other contexts
> we've had discussions about how we don't actually guarantee all that
> much uniformity between a partitioned table and its partitions, and
> it's been questioned whether we made the right decisions there. So I'm
> not entirely sure that the surface area for problems here will be as
> narrow as you're hoping -- I think we'd need to go through all of the
> ALTER TABLE variants and think it through. But maybe the problems
> aren't that bad.

Many changeable properties that are reflected in the RelationData of a
partition after getting the lock on it seem to cause no issues as long
as the executor code only looks at RelationData, which is true for
most Scan nodes.  It also seems true for ModifyTable which looks into
RelationData for relation properties relevant to insert/deletes.

The two things that don't cope are:

* Index Scan nodes with concurrent DROP INDEX of partition-only indexes.

* Concurrent DROP CONSTRAINT of partition-only CHECK and NOT NULL
constraints can lead to incorrect result as I write below.

> It does seem like constraints can change the plan. Imagine the
> partition had a CHECK(false) constraint before and now doesn't, or
> something.

Yeah, if the CHECK constraint gets dropped concurrently, any new rows
that got added after that will not be returned by executing a stale
cached plan, because the plan would have been created based on the
assumption that such rows shouldn't be there due to the CHECK
constraint.  We currently don't explicitly check that the constraints
that were used during planning still exist before executing the plan.

Overall, I'm starting to feel less enthused by the idea throwing an
error in the executor due to known and unknown hazards of trying to
execute a stale plan.  Even if we made a note in the docs of such
hazards, any users who run into these rare errors are likely to head
to -bugs or -hackers anyway.

Tom said we should perhaps look at the hazards caused by intra-session
locking, but we'd still be left with the hazards of missing index and
constraints,  AFAICS, due to DROP from other sessions.

So, the options:

* The replanning aspect of the lock-in-the-executor design would be
simpler if a CachedPlan contained the plan for a single query rather
than a list of queries, as previously mentioned. This is particularly
due to the requirements of the PORTAL_MULTI_QUERY case. However, this
option might be impractical.

* Polish the patch for the old design of doing the initial pruning
before AcquireExecutorLocks() and focus on hashing out any bugs and
issues of that design.

--=20
Thanks, Amit Langote