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.
