Content-Type: multipart/alternative;
 boundary="------------oGJ7zO40KdolAmmMH0GIS1od"
Message-ID: <d4a5ea75-d6a3-7f02-890e-8e3d1a43aeab@yandex.ru>
Date: Wed, 21 Sep 2022 11:30:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: RFC: Logging plan of the running query
Content-Language: en-US
To: torikoshia <torikoshia@oss.nttdata.com>
References: <20220202.170421.204740592565249296.horikyota.ntt@gmail.com>
 <3bb7bacbc5ef9e58e2ece0557adac98b@oss.nttdata.com>
 <03c8dcbb-72d2-d3ad-3ad1-9efe69676529@oss.nttdata.com>
 <20220208.171852.1699766364147189900.horikyota.ntt@gmail.com>
 <ab761e68f3fbcee80cf1ca19b1db85f1@oss.nttdata.com>
 <3e94e9b3-83ef-c328-0c5c-f93aa3224790@oss.nttdata.com>
 <3d121ed5f81cef588bac836b43f5d1f9@oss.nttdata.com>
 <379d69c0b3d058997550f88e73ffddd6@oss.nttdata.com>
 <727712bcaa8d449615820c16d15e452e@oss.nttdata.com>
 <598171663576373@mail.yandex.ru>
 <38f4e19323b264e5975c60010b512b15@oss.nttdata.com>
Cc: pgsql-hackers@lists.postgresql.org
From: Alena Rybakina <lena.ribackina@yandex.ru>
In-Reply-To: <38f4e19323b264e5975c60010b512b15@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/d4a5ea75-d6a3-7f02-890e-8e3d1a43aeab%40yandex.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------oGJ7zO40KdolAmmMH0GIS1od
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

  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
--------------oGJ7zO40KdolAmmMH0GIS1od
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><font size="4"> Ok, I get it.</font></p>
    <blockquote type="cite">
      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. </blockquote>
    <font size="4">Agree.
    </font>
    <blockquote type="cite">
      Since these processes are needed for nested queries, not only for
      AbortSubTransaction[1], added comments on it.</blockquote>
    <p><font size="4">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.</font></p>
    <p><font size="4">We have a similar task to print the plan of an
        interrupted process making a request for a specific pid.</font></p>
    <p><font size="4">In short, I think, this task is different and for
        separating these parts I added this comment.</font></p>
    <blockquote type="cite">
      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.
    </blockquote>
    <p>
      <font size="4">Yes, I was worried about it. I understood it, thank
        for explaining.</font></p>
    <blockquote type="cite">
      <br>
      AFAIU both of them are processed since SendProcSignal flags
      ps_signalFlags for each signal.
      <br>
      <br>
      ```
      <br>
      SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId
      backendId)
      <br>
      {
      <br>
      volatile ProcSignalSlot *slot;
      <br>
      ...(snip)...
      <br>
      278 if (slot-&gt;pss_pid == pid)
      <br>
      279 {
      <br>
      280 /* Atomically set the proper flag */
      <br>
      281 slot-&gt;pss_signalFlags[reason] = true;
      <br>
      282 /* Send signal */
      <br>
      283 return kill(pid, SIGUSR1);
      <br>
      ```
      <br>
      <br>
      Comments of ProcSignalReason also say 'We can cope with concurrent
      signals for different reasons'.
      <br>
      <br>
      ```C
      <br>
      /*
      <br>
      * Reasons for signaling a Postgres child process (a backend or an
      auxiliary
      <br>
      * process, like checkpointer). We can cope with concurrent signals
      for different
      <br>
      * reasons. However, if the same reason is signaled multiple times
      in
      quick
      <br>
      * succession, the process is likely to observe only one
      notification
      of it.
      <br>
      * This is okay for the present uses.
      <br>
      ...
      <br>
      typedef enum
      <br>
      {
      <br>
      PROCSIG_CATCHUP_INTERRUPT, /* sinval catchup interrupt */
      <br>
      PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
      <br>
      PROCSIG_PARALLEL_MESSAGE, /* message from cooperating parallel
      backend */
      <br>
      PROCSIG_WALSND_INIT_STOPPING, /* ask walsenders to prepare for
      shutdown */
      <br>
      PROCSIG_BARRIER, /* global barrier interrupt */
      <br>
      PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory
      contexts */
      <br>
      PROCSIG_LOG_QUERY_PLAN, /* ask backend to log plan of the current
      query */
      <br>
      ...
      <br>
      } ProcSignalReason;
      <br>
      ```
      <br>
      <br>
      <br>
      [1]
<a class="moz-txt-link-freetext" href="https://www.postgresql.org/message-id/8b53b32f-26cc-0531-4ac0-27310e0bef4b%40oss.nttdata.com">https://www.postgresql.org/message-id/8b53b32f-26cc-0531-4ac0-27310e0bef4b%40oss.nttdata.com</a><br>
    </blockquote>
  </body>
</html>

--------------oGJ7zO40KdolAmmMH0GIS1od--