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.
