Re: RFC: Logging plan of the running query

Alena Rybakina <lena.ribackina@yandex.ru>

From: Alena Rybakina <lena.ribackina@yandex.ru>
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: pgsql-hackers@lists.postgresql.org
Date: 2022-09-21T08:30:25Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →
  1. ecpg: Catch zero-length Unicode identifiers correctly

  2. Improve warning message in pg_signal_backend()

  3. Add assert to ensure that page locks don't participate in deadlock cycle.

  Ok, I get it.

> Since GetLockMethodLocalHash() is only used for assertions, this is 
> only defined when USE_ASSERT_CHECKING is enabled. This patch uses 
> GetLockMethodLocalHash() not only for the assertion purpose, so I 
> removed "ifdef USE_ASSERT_CHECKING" for this function. I belive it 
> does not lead to skip errors. 
Agree.
> Since these processes are needed for nested queries, not only for 
> AbortSubTransaction[1], added comments on it.

I also noticed it. However I also discovered that above function 
declarations to be aplied for explain command and used to be printed 
details of the executed query.

We have a similar task to print the plan of an interrupted process 
making a request for a specific pid.

In short, I think, this task is different and for separating these parts 
I added this comment.

> I feel this comment is unnecessary since the explanation of 
> HandleLogQueryPlanInterrupt() is written in explain.c and no functions 
> in explain.h have comments in it. 

Yes, I was worried about it. I understood it, thank for explaining.

>
> AFAIU both of them are processed since SendProcSignal flags 
> ps_signalFlags for each signal.
>
> ```
> SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId)
> {
> volatile ProcSignalSlot *slot;
> ...(snip)...
> 278 if (slot->pss_pid == pid)
> 279 {
> 280 /* Atomically set the proper flag */
> 281 slot->pss_signalFlags[reason] = true;
> 282 /* Send signal */
> 283 return kill(pid, SIGUSR1);
> ```
>
> Comments of ProcSignalReason also say 'We can cope with concurrent 
> signals for different reasons'.
>
> ```C
> /*
> * Reasons for signaling a Postgres child process (a backend or an 
> auxiliary
> * process, like checkpointer). We can cope with concurrent signals for 
> different
> * reasons. However, if the same reason is signaled multiple times in 
> quick
> * succession, the process is likely to observe only one notification 
> of it.
> * This is okay for the present uses.
> ...
> typedef enum
> {
> PROCSIG_CATCHUP_INTERRUPT, /* sinval catchup interrupt */
> PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
> PROCSIG_PARALLEL_MESSAGE, /* message from cooperating parallel backend */
> PROCSIG_WALSND_INIT_STOPPING, /* ask walsenders to prepare for 
> shutdown */
> PROCSIG_BARRIER, /* global barrier interrupt */
> PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
> PROCSIG_LOG_QUERY_PLAN, /* ask backend to log plan of the current 
> query */
> ...
> } ProcSignalReason;
> ```
>
>
> [1] 
> https://www.postgresql.org/message-id/8b53b32f-26cc-0531-4ac0-27310e0bef4b%40oss.nttdata.com