Date: Mon, 26 Feb 2024 21:54:41 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: James Coleman <jtc331@gmail.com>, Andres Freund <andres@anarazel.de>,
	torikoshia <torikoshia@oss.nttdata.com>,
	pgsql-hackers@lists.postgresql.org,
	=?iso-8859-1?Q?=C9tienne?= BERSAC <etienne.bersac@dalibo.com>,
	ashutosh.bapat.oss@gmail.com, rafaelthca@gmail.com,
	jian.universality@gmail.com
Subject: Re: RFC: Logging plan of the running query
Message-ID: <ZdyYIXZDEsAuKiw9@jrouhaud>
References: <CA+TgmobH+Uto9MCD+vWc71bVbOnd7d8zeYjRT8nXaeLe5hsNJQ@mail.gmail.com>
 <20240215185911.v4o6fo444md6a3w7@awork3.anarazel.de>
 <CA+TgmoZ2Hsb39zzofasjOHXnjsNfJy84YjFF=-KNOO7FsvAi9Q@mail.gmail.com>
 <CAAaqYe-gMkdL=M4v47=H0F3+-zi2qL9zFqAv3QsizkRjFiQR0w@mail.gmail.com>
 <CA+TgmoYkztFSryutQMNv9DnQ__NDwGE8XhhgnbdCG8e_o4T7yQ@mail.gmail.com>
 <hzicr3kidnmwbcqwmwortldzq4fijrfca7c3prncoepgt6rpwf@uydrxvi7qhpk>
 <CA+Tgmoa6QniTueRJ1rKcp_-TpVc0c8FZxOtu4ehDop+R=pEcpA@mail.gmail.com>
 <CAAaqYe9sS+FXZBugNEEjb89XLny8KGpfhrVHdsOug2yOeC3now@mail.gmail.com>
 <ryv7bxrkuycet2rm6yh2igcfdm6cpza3bmu6zlpdx4x5mvgd2r@yjys5ktjz7hv>
 <CA+TgmoZ-YjXougke=jej09RW2ZWJ8+hfVuBwzQvmjcD9OnObLg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+TgmoZ-YjXougke=jej09RW2ZWJ8+hfVuBwzQvmjcD9OnObLg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZdyYIXZDEsAuKiw9%40jrouhaud>
Precedence: bulk

On Mon, Feb 26, 2024 at 01:56:44PM +0530, Robert Haas wrote:
> On Sun, Feb 25, 2024 at 5:00 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > Yeah, trying to find a generalized solution seems like worth investing some
> > time to avoid having a bunch of CHECK_FOR_XXX() calls scattered in the code a
> > few years down the road.
>
> I just don't really see how to do it. I suspect that every task that
> wants to run at some CFIs but not others is going to have slightly
> different requirements, and we probably can't foresee what all of
> those requirements are.
>
> Said another way, if in the future we want to call
> DoSomethingOrOther() from the CFI handler, well then we need to know
> that we're not already in the middle of using any subsystem that
> DoSomethingOrOther() might also try to use ... and we also need to
> know that we're not in the middle of doing anything that's more
> critical than DoSomethingOrOther(). But both of these are likely to
> vary in each case.
>
> EXPLAIN might be one member of a general class of things that require
> catalog access (and thus might take locks or lwlocks, access the
> catalogs, trigger invalidations, etc.) but it's not clear how general
> that class really is. Also, I think if we try to do too many different
> kinds of things at CFIs, the whole thing is going to fall apart.
> You'll end up failing to foresee some interactions, or the stack will
> get too deep, or ... something.

I still fail to understand your point.  So you say we might want to check for
safe condition to run explain or DoSomethingOrOther or even DoMeh, with all
different requirements.  IIUC what you're saying is that we should have
CHECK_FOR_EXPLAIN(), CHECK_FOR_DOSOMETHINGOROTHER() and CHECK_FOR_MEH()?

And so in some code path A we could have

CHECK_FOR_INTERRUPTS();
CHECK_FOR_EXPLAIN();

In another

CHECK_FOR_INTERRUPTS();
CHECK_FOR_DOSOMETHINGOROTHER();

and in one happy path

CHECK_FOR_INTERRUPTS();
CHECK_FOR_EXPLAIN();
CHECK_FOR_DOSOMETHINGOROTHER();
CHECK_FOR_MEH();

Or that we should still have all of those but they shouldn't be anywhere close
to a CHECK_FOR_INTERRUPTS(), or something totally different?

In the first case, I'm not sure why having CHECK_FOR_INTERRUPTS(EXPLAIN|...),
with a combination of flags and with the flag handling being done after
ProcessIterrupts(), would be any different apart from having less boilerplate
lines.  Similarly for the 2nd case, but relying on a single more general
CHECK_FOR_CONDITION(EXPLAIN | ...) rather than N CHECK_FOR_XXX?

If you just want something totally different then sure.