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
