Re: RFC: Logging plan of the running query

On 2021-10-13 23:28, Ekaterina Sokolova wrote:
> Hi, hackers!
>
> • The last version of patch is correct applied. It changes 8 files
> from /src/backend, and 9 other files.
>
> • I have 1 error and 1 warning during compilation on Mac.
>
> explain.c:4985:25: error: implicit declaration of function
> 'GetLockMethodLocalHash' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
> hash_seq_init(&status, GetLockMethodLocalHash());
> explain.c:4985:25: warning: incompatible integer to pointer conversion
> passing 'int' to parameter of type 'HTAB *' (aka 'struct HTAB *')
> [-Wint-conversion]
> hash_seq_init(&status, GetLockMethodLocalHash());
>
> This error doesn't appear at my second machine with Ubuntu.
>
> I found the reason. You delete #ifdef USE_ASSERT_CHECKING from
> implementation of function GetLockMethodLocalHash(void), but this
> ifdef exists around function declaration. There may be a situation,
> when implementation exists without declaration, so files with using of
> function produce errors. I create new version of patch with fix of
> this problem.

Thanks for fixing that!

> I'm agree that seeing the details of a query is a useful feature, but
> I have several doubts:
>
> 1) There are lots of changes of core's code. But not all users need
> this functionality. So adding this functionality like extension seemed
> more reasonable.

It would be good if we can implement this feature in an extension, but
as pg_query_state extension needs applying patches to PostgreSQL, I
think this kind of feature needs PostgreSQL core modification.
IMHO extensions which need core modification are not easy to use in
production environments..

> 2) There are many tools available to monitor the status of a query.
> How much do we need another one? For example:
> • pg_stat_progress_* is set of views with current status of
> ANALYZE, CREATE INDEX, VACUUM, CLUSTER, COPY, Base Backup. You can
> find it in PostgreSQL documentation [1].
> • pg_query_state is contrib with 2 patches for core (I hope
> someday Community will support adding this patches to PostgreSQL). It
> contains function with printing table with pid, full query text, plan
> and current progress of every node like momentary EXPLAIN ANALYSE for
> SELECT, UPDATE, INSERT, DELETE. So it supports every flags and formats
> of EXPLAIN. You can find current version of pg_query_state on github
> [2]. Also I found old discussion about first version of it in
> Community [3].

Thanks for introducing the extension!

I only took a quick look at pg_query_state, I have some questions.

pg_query_state seems using shm_mq to expose the plan information, but
there was a discussion that this kind of architecture would be tricky to
do properly [1].
Does pg_query_state handle difficulties listed on the discussion?

It seems the caller of the pg_query_state() has to wait until the target
process pushes the plan information into shared memory, can it lead to
deadlock situations?
I came up with this question because when trying to make a view for
memory contexts of other backends, we encountered deadlock situations.
After all, we gave up view design and adopted sending signal and
logging.

Some of the comments of [3] seem useful for my patch, I'm going to
consider them. Thanks!

> 3) Have you measured the overload of your feature? It would be really
> interesting to know the changes in speed and performance.

I haven't measured it yet, but I believe that the overhead for backends
which are not called pg_log_current_plan() would be slight since the
patch just adds the logic for saving QueryDesc on ExecutorRun().
The overhead for backends which is called pg_log_current_plan() might
not slight, but since the target process are assumed dealing with
long-running query and the user want to know its plan, its overhead
would be worth the cost.

> Thank you for working on this issue. I would be glad to continue to
> follow the development of this issue.

Thanks for your help!

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION