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>
In-Reply-To: <CA+HiwqE_rQ9pZnkXeoHdds2kgAiT7XNNHZW8gTGicBdXv0rwnw@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 19 Aug 2024 12:39:02 -0400
Message-ID: <CA+TgmoY2drv9PmrRAC7AR77mkx09sOh-+5qJkHB_hLKeHRNqzQ@mail.gmail.com>
Subject: Re: generic plans and "initial" pruning
To: Amit Langote <amitlangote09@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%2BTgmoY2drv9PmrRAC7AR77mkx09sOh-%2B5qJkHB_hLKeHRNqzQ%40mail.gmail.com>
Precedence: bulk

On Fri, Aug 16, 2024 at 8:36=E2=80=AFAM Amit Langote <amitlangote09@gmail.c=
om> wrote:
> So it is possible for the executor to try to run a plan that has
> become invalid since it was created, so...

I'm not sure what the "so what" here is.

> One perhaps crazy idea [1]:
>
> What if we remove AcquireExecutorLocks() and move the responsibility
> of taking the remaining necessary locks into the executor (those on
> any inheritance children that are added during planning and thus not
> accounted for by AcquirePlannerLocks()), like the patch already does,
> but don't make it also check if the plan has become invalid, which it
> can't do anyway unless it's from a CachedPlan.  That means we instead
> let the executor throw any errors that occur when trying to either
> initialize the plan because of the changes that have occurred to the
> objects referenced in the plan, like what is happening in the above
> example.  If that case is going to be rare anway, why spend energy on
> checking the validity and replan, especially if that's not an easy
> thing to do as we're finding out.  In the above example, we could say
> that it's a user error to create a rule like that, so it should not
> happen in practice, but when it does, the executor seems to deal with
> it correctly by refusing to execute a broken plan .  Perhaps it's more
> worthwhile to make the executor behave correctly in face of plan
> invalidation than teach the rest of the system to deal with the
> executor throwing its hands up when it runs into an invalid plan?
> Again, I think this may be a crazy line of thinking but just wanted to
> get it out there.

I don't know whether this is crazy or not. I think there are two
issues. One, the set of checks that we have right now might not be
complete, and we might just not have realized that because it happens
infrequently enough that we haven't found all the bugs. If that's so,
then a change like this could be a good thing, because it might force
us to fix stuff we should be fixing anyway. I have a feeling that some
of the checks you hit there were added as bug fixes long after the
code was written originally, so my confidence that we don't have more
bugs isn't especially high.

And two, it matters a lot how frequent the errors will be in practice.
I think we normally try to replan rather than let a stale plan be used
because we want to not fail, because users don't like failure. If the
design you propose here would make failures more (or less) frequent,
then that's a problem (or awesome).

--=20
Robert Haas
EDB: http://www.enterprisedb.com