public inbox for [email protected]
help / color / mirror / Atom feedFrom: torikoshia <[email protected]>
To: Robert Haas <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: RFC: Logging plan of the running query
Date: Tue, 01 Apr 2025 22:57:44 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoZ8dm2-19icz+Lta2+e_DRNb5ZkoOz_8pNDGm49ofEyGg@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<174144854199.294105.9417032645816235284.pgcf@coridan.postgresql.org>
<[email protected]>
<[email protected]>
<CA+TgmoYcnAj2WTiNP_wtLTf6DabhF384nR8wvDZ_o8C5AG-K3w@mail.gmail.com>
<[email protected]>
<CA+TgmoZ8dm2-19icz+Lta2+e_DRNb5ZkoOz_8pNDGm49ofEyGg@mail.gmail.com>
On 2025-04-01 21:32, Robert Haas wrote:
Thanks for your comment.
> On Mon, Mar 31, 2025 at 9:43 PM torikoshia <[email protected]>
> 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.
+1.
> 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’t 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.
That’s really appreciated!
I believe some of the comments in Rafael's thread should be reflected in
this one as well, but I haven’t incorporated them yet. Apologies for
that.
--
Regards,
--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.
view thread (3+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: RFC: Logging plan of the running query
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox