Re: RFC: Logging plan of the running query

On 2025-04-01 03:51, Sadeq Dousti wrote:
> Hi,
> 
> Sergey and I reviewed this patch and have a few comments, listed
> below.
Thanks for your review!

>> BTW the patch adds about 400 lines to explain.c and it may be better
>> to split the file as well as 9173e8b6046, but I leave it as it is
> for
>> now.
> 
> 1. As rightfully described by the OP above, explain.c has grown too
> big. In addition, explain.h is added to quite a number of other files.
Agreed.

> 
> 2. We wanted to entertain the possibility of a GUC variable to enable
> the feature. That is, having it disabled by default, and the DBA can
> selectively enable it on the fly. The reasoning is that it can affect
> some workloads in an unforeseen way, so this choice can make the next
> Postgres release safer. In future releases, we can make the default
> "enabled", assuming enough real-world scenarios have proven the
> feature safe (i.e., not affecting the DB performance noticeably).
Agreed.

> 3. In the email thread for this patch, we saw some attempts to measure
> the performance overhead of the feature. We suggest a more rigorous
> study, including memory overhead in addition to time overhead. We came
> to the conclusion that analytical workloads - with complicated query
> plans - and a high query rate are the best to attempt such benchmarks.
Agreed.

> 4. In PGConf EU 2024, there was a talk by Rafael Castro titled
> "Debugging active queries" [1], and he also submitted a recent patch
> titled "Proposal: Progressive explain" [2]. We see a lot of
> similarities in the idea behind that patch and this one, and would
> like to suggest joining forces for an ideal outcome.
As we have been discussing in this thread recently, I believe that since 
this patch has a narrower scope, it makes sense to complete it first and 
then extend it to support progress tracking in a step-by-step manner.

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.