Re: RFC: Logging plan of the running query

Hi,

Sergey and I reviewed this patch and have a few comments, listed below.

> 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.

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).

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.

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.

[1] https://www.youtube.com/watch?v=6ahTb-7C05c
[2] https://commitfest.postgresql.org/patch/5473/

Best Regards,
Sadeq Dousti & Sergey Dudoladov