MIME-Version: 1.0
References: <d68c3ae31672664876b22d2dcbb526d2@postgrespro.ru>
 <2aedcb5e8120bfdee994775e660f8718@oss.nttdata.com> <174144854199.294105.9417032645816235284.pgcf@coridan.postgresql.org>
 <ffdfab063ac6f3d3d97763c9d744b70b@oss.nttdata.com> <784224198089cbe3cc3825332f195475@oss.nttdata.com>
 <CA+TgmoYcnAj2WTiNP_wtLTf6DabhF384nR8wvDZ_o8C5AG-K3w@mail.gmail.com> <067675dd73b5485ecf0159d9a986846d@oss.nttdata.com>
In-Reply-To: <067675dd73b5485ecf0159d9a986846d@oss.nttdata.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Tue, 1 Apr 2025 08:32:04 -0400
Message-ID: <CA+TgmoZ8dm2-19icz+Lta2+e_DRNb5ZkoOz_8pNDGm49ofEyGg@mail.gmail.com>
Subject: Re: RFC: Logging plan of the running query
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: rafaelthca@gmail.com, pgsql-hackers@lists.postgresql.org, 
	destrex271@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BTgmoZ8dm2-19icz%2BLta2%2Be_DRNb5ZkoOz_8pNDGm49ofEyGg%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 31, 2025 at 9:43=E2=80=AFPM torikoshia <torikoshia@oss.nttdata.=
com> wrote:
> Previously, Rafael proposed a patch in this thread that added execution
> progress tracking. However, I felt that expanding the scope could make
> it harder to get the patch reviewed or committed. So, I suggested first
> completing a feature that only retrieves the execution plan of a running
> query, and then developing execution progress tracking afterward[3].

That's reasonable. Comparing the two patches, I think that you have
correctly solved a couple of key problems that Rafael did not handle
correctly. Specifically, I believe you have correctly implemented what
Andres described i.e. use CFI to get control to do the wrapping and
then from the ExecProcNode wrapper do the actual EXPLAIN, whereas I
believe Rafael was doing the wrapping directly from the signal
handler, which did not seem safe to me:

http://postgr.es/m/CA+TgmobrzeDep+Z1BPQqGNsCqTQ8M58wNNKJB_8Lwpwbqbz3GQ@mail=
.gmail.com

I also like your version of (sub)transaction abort handling much
better than the memory-context cleanup that Rafael initially chose.
He's since revised that.

That said, I think the view that Rafael proposes, with a
periodically-updating version of the EXPLAIN ANALYZE output up to that
point, could be extremely useful in some situations. To make that
work, he mostly just hacked InstrStopNode(), although as I'm thinking
about it, that probably doesn't handle all of the parallel query cases
correctly. I'm somewhat inclined to hope that we eventually end up
with both interfaces.

> I was considering whether to introduce a GUC in this patch to allow for
> prior setup before outputting the plan or to switch to Rafael's patch
> after reviewing its details. However, since there isn=E2=80=99t much time=
 left
> before the feature freeze, if you have already reviewed Rafael's patch
> and there is a chance it could be committed, it would be better to focus
> on that.

I wasn't feeling very confident about my ability to get that patch
committed before feature freeze. I don't want to rush into something
that we might later regret. I'm going to spend a bit more time
studying your patch next.

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