Content-Type: multipart/mixed; boundary="------------Y4a1ORitzvUUbBW01Ds0f070"
Message-ID: <6ee42413-41db-91af-4391-614f6af39000@postgrespro.ru>
Date: Mon, 19 Sep 2022 13:42:24 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Content-Language: en-US
To: torikoshia <torikoshia@oss.nttdata.com>,
 pgsql-hackers@lists.postgresql.org
From: "a.rybakina" <a.rybakina@postgrespro.ru>
Subject: Re: RFC: Logging plan of the running query
Archived-At: 
 <https://www.postgresql.org/message-id/6ee42413-41db-91af-4391-614f6af39000%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------Y4a1ORitzvUUbBW01Ds0f070
Content-Type: multipart/alternative;
 boundary="------------UvcgS3YrAPwqVyoXWg0Ymlxb"

--------------UvcgS3YrAPwqVyoXWg0Ymlxb
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

I'm sorry,if this message is duplicated previous this one, but the 
previous message is sent incorrectly. I sent it from email address 
lena.ribackina@yandex.ru.

I liked this idea and after reviewing code I noticed some moments and 
I'd rather ask you some questions.

Firstly, I suggest some editing in the comment of commit. I think, it is 
turned out the more laconic and the same clear. I wrote it below since I 
can't think of any other way to add it.

```
Currently, we have to wait for finishing of the query execution to check 
its plan.
This is not so convenient in investigation long-running queries on 
production
environments where we cannot use debuggers.

To improve this situation there is proposed the patch containing the 
pg_log_query_plan()
function which request to log plan of the specified backend process.

By default, only superusers are allowed to request log of the plan 
otherwise
allowing any users to issue this request could create cause lots of log 
messages
and it can lead to denial of service.

At the next requesting CHECK_FOR_INTERRUPTS(), the target backend logs 
its plan at
LOG_SERVER_ONLY level and therefore this plan will appear in the server 
log only,
not to be sent to the client.
```

Secondly, I have question about deleting USE_ASSERT_CHECKING in lock.h?
It supposed to have been checked in another placed of the code by 
matching values. I am worry about skipping errors due to untesting with 
assert option in the places where it (GetLockMethodLocalHash) 
participates and we won't able to get core file in segfault cases. I 
might not understand something, then can you please explain to me?

Thirdly, I have incomprehension of the point why save_ActiveQueryDesc is 
declared in the pquery.h? I am seemed to save_ActiveQueryDesc to be used 
in an once time in the ExecutorRun function and  its declaration 
superfluous. I added it in the attached patch.

Fourthly, it seems to me there are not enough explanatory comments in 
the code. I also added them in the attached patch.

Lastly, I have incomprehension about handling signals since have been 
unused it before. Could another signal disabled calling this signal to 
log query plan? I noticed this signal to be checked the latest in 
procsignal_sigusr1_handler function.

Regards,

-- 
Alena Rybakina
Postgres Professional
> 19.09.2022, 11:01, "torikoshia" <torikoshia@oss.nttdata.com>:
>
>     On 2022-05-16 17:02, torikoshia wrote:
>
>          On 2022-03-09 19:04, torikoshia wrote:
>
>              On 2022-02-08 01:13, Fujii Masao wrote:
>
>                  AbortSubTransaction() should reset ActiveQueryDesc to
>                  save_ActiveQueryDesc that ExecutorRun() set, instead
>                 of NULL?
>                  Otherwise ActiveQueryDesc of top-level statement will
>                 be unavailable
>                  after subtransaction is aborted in the nested statements.
>
>
>              I once agreed above suggestion and made v20 patch making
>              save_ActiveQueryDesc a global variable, but it caused
>             segfault when
>              calling pg_log_query_plan() after FreeQueryDesc().
>
>              OTOH, doing some kind of reset of ActiveQueryDesc seems
>             necessary
>              since it also caused segfault when running
>             pg_log_query_plan() during
>              installcheck.
>
>              There may be a better way, but resetting ActiveQueryDesc
>             to NULL seems
>              safe and simple.
>              Of course it makes pg_log_query_plan() useless after a
>             subtransaction
>              is aborted.
>              However, if it does not often happen that people want to
>             know the
>              running query's plan whose subtransaction is aborted,
>             resetting
>              ActiveQueryDesc to NULL would be acceptable.
>
>              Attached is a patch that sets ActiveQueryDesc to NULL when a
>              subtransaction is aborted.
>
>              How do you think?
>
>     Attached new patch to fix patch apply failures again.
>
>     --
>     Regards,
>
>     --
>     Atsushi Torikoshi
>     NTT DATA CORPORATION
>
--------------UvcgS3YrAPwqVyoXWg0Ymlxb
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>Hi,
      <br>
    </p>
    <p>I'm sorry,if this message is duplicated previous this one, but
      the previous message is sent incorrectly. I sent it from email
      address <a class="moz-txt-link-abbreviated" href="mailto:lena.ribackina@yandex.ru">lena.ribackina@yandex.ru</a>.
    </p>
    <p>I liked this idea and after reviewing code I noticed some moments
      and I'd rather ask you some questions.</p>
    Firstly, I suggest some editing in the comment of commit. I think,
    it is turned out the more laconic and the same clear. I wrote it
    below since I can't think of any other way to add it.
    <br>
    <br>
    ```
    <br>
    Currently, we have to wait for finishing of the query execution to
    check its plan.
    <br>
    This is not so convenient in investigation long-running queries on
    production
    <br>
    environments where we cannot use debuggers.
    <br>
    <br>
    To improve this situation there is proposed the patch containing the
    pg_log_query_plan()
    <br>
    function which request to log plan of the specified backend process.
    <br>
    <br>
    By default, only superusers are allowed to request log of the plan
    otherwise
    <br>
    allowing any users to issue this request could create cause lots of
    log messages
    <br>
    and it can lead to denial of service.
    <br>
    <br>
    At the next requesting CHECK_FOR_INTERRUPTS(), the target backend
    logs its plan at
    <br>
    LOG_SERVER_ONLY level and therefore this plan will appear in the
    server log only,
    <br>
    not to be sent to the client.
    <br>
    ```
    <br>
    <br>
    Secondly, I have question about deleting USE_ASSERT_CHECKING in
    lock.h?
    <br>
    It supposed to have been checked in another placed of the code by
    matching values. I am worry about skipping errors due to untesting
    with assert option in the places where it (GetLockMethodLocalHash)
    participates and we won't able to get core file in segfault cases. I
    might not understand something, then can you please explain to me?
    <br>
    <br>
    Thirdly, I have incomprehension of the point why
    save_ActiveQueryDesc is declared in the pquery.h? I am seemed to
    save_ActiveQueryDesc to be used in an once time in the ExecutorRun
    function and  its declaration superfluous. I added it in the
    attached patch.
    <br>
    <br>
    Fourthly, it seems to me there are not enough explanatory comments
    in the code. I also added them in the attached patch.
    <br>
    <br>
    Lastly, I have incomprehension about handling signals since have
    been unused it before. Could another signal disabled calling this
    signal to log query plan? I noticed this signal to be checked the
    latest in procsignal_sigusr1_handler function.
    <br>
    <br>
    Regards,
    <br>
    <br>
    --
    <br>
    Alena Rybakina
    <br>
    Postgres Professional
    <blockquote type="cite" cite="mid:598171663576373@mail.yandex.ru">
      <div>19.09.2022, 11:01, "torikoshia"
        <a class="moz-txt-link-rfc2396E" href="mailto:torikoshia@oss.nttdata.com">&lt;torikoshia@oss.nttdata.com&gt;</a>:</div>
      <blockquote>
        <p>On 2022-05-16 17:02, torikoshia wrote:</p>
        <blockquote> On 2022-03-09 19:04, torikoshia wrote:
          <blockquote> On 2022-02-08 01:13, Fujii Masao wrote:
            <blockquote> AbortSubTransaction() should reset
              ActiveQueryDesc to<br>
               save_ActiveQueryDesc that ExecutorRun() set, instead of
              NULL?<br>
               Otherwise ActiveQueryDesc of top-level statement will be
              unavailable<br>
               after subtransaction is aborted in the nested statements.</blockquote>
             <br>
             I once agreed above suggestion and made v20 patch making<br>
             save_ActiveQueryDesc a global variable, but it caused
            segfault when<br>
             calling pg_log_query_plan() after FreeQueryDesc().<br>
             <br>
             OTOH, doing some kind of reset of ActiveQueryDesc seems
            necessary<br>
             since it also caused segfault when running
            pg_log_query_plan() during<br>
             installcheck.<br>
             <br>
             There may be a better way, but resetting ActiveQueryDesc to
            NULL seems<br>
             safe and simple.<br>
             Of course it makes pg_log_query_plan() useless after a
            subtransaction<br>
             is aborted.<br>
             However, if it does not often happen that people want to
            know the<br>
             running query's plan whose subtransaction is aborted,
            resetting<br>
             ActiveQueryDesc to NULL would be acceptable.<br>
             <br>
             Attached is a patch that sets ActiveQueryDesc to NULL when
            a<br>
             subtransaction is aborted.<br>
             <br>
             How do you think?</blockquote>
        </blockquote>
        <p>Attached new patch to fix patch apply failures again.<br>
           </p>
        --<br>
        Regards,<br>
        <br>
        --<br>
        Atsushi Torikoshi<br>
        NTT DATA CORPORATION</blockquote>
    </blockquote>
  </body>
</html>

--------------UvcgS3YrAPwqVyoXWg0Ymlxb--

--------------Y4a1ORitzvUUbBW01Ds0f070
Content-Type: text/x-patch; charset=UTF-8; name="diff_query_plan.patch"
Content-Disposition: attachment; filename="diff_query_plan.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL2V4ZWNNYWluLmMgYi9zcmMvYmFj
a2VuZC9leGVjdXRvci9leGVjTWFpbi5jCmluZGV4IGE4MmFjODc0NTdlLi42NWI2OTJiMGRk
ZiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvZXhlY3V0b3IvZXhlY01haW4uYworKysgYi9z
cmMvYmFja2VuZC9leGVjdXRvci9leGVjTWFpbi5jCkBAIC0zMDYsNiArMzA2LDEyIEBAIEV4
ZWN1dG9yUnVuKFF1ZXJ5RGVzYyAqcXVlcnlEZXNjLAogewogCVF1ZXJ5RGVzYyAqc2F2ZV9B
Y3RpdmVRdWVyeURlc2M7CiAKKwkvKgorCSAqIFNhdmUgdmFsdWUgb2YgQWN0aXZlUXVlcnlE
ZXNjIGJlZm9yZSBoYXZpbmcgY2FsbGVkCisJICogRXhlY3V0b3JSdW5faG9vayBmdW5jdGlv
biBkdWUgdG8gaGF2aW5nIHJlc2V0IGJ5CisJICogQWJvcnRTdWJUcmFuc2FjdGlvbi4KKwkg
Ki8KKwogCXNhdmVfQWN0aXZlUXVlcnlEZXNjID0gQWN0aXZlUXVlcnlEZXNjOwogCUFjdGl2
ZVF1ZXJ5RGVzYyA9IHF1ZXJ5RGVzYzsKIApAQCAtMzE0LDYgKzMyMCw3IEBAIEV4ZWN1dG9y
UnVuKFF1ZXJ5RGVzYyAqcXVlcnlEZXNjLAogCWVsc2UKIAkJc3RhbmRhcmRfRXhlY3V0b3JS
dW4ocXVlcnlEZXNjLCBkaXJlY3Rpb24sIGNvdW50LCBleGVjdXRlX29uY2UpOwogCisJLyog
V2Ugc2V0IHRoZSBhY3R1YWwgdmFsdWUgb2YgQWN0aXZlUXVlcnlEZXNjICovCiAJQWN0aXZl
UXVlcnlEZXNjID0gc2F2ZV9BY3RpdmVRdWVyeURlc2M7CiB9CiAKZGlmZiAtLWdpdCBhL3Ny
Yy9pbmNsdWRlL2NvbW1hbmRzL2V4cGxhaW4uaCBiL3NyYy9pbmNsdWRlL2NvbW1hbmRzL2V4
cGxhaW4uaAppbmRleCBmYzlmOWY4ZTNmMC4uOGU3Y2UzYzk3NmYgMTAwNjQ0Ci0tLSBhL3Ny
Yy9pbmNsdWRlL2NvbW1hbmRzL2V4cGxhaW4uaAorKysgYi9zcmMvaW5jbHVkZS9jb21tYW5k
cy9leHBsYWluLmgKQEAgLTEyNiw2ICsxMjYsNyBAQCBleHRlcm4gdm9pZCBFeHBsYWluT3Bl
bkdyb3VwKGNvbnN0IGNoYXIgKm9ianR5cGUsIGNvbnN0IGNoYXIgKmxhYmVsbmFtZSwKIGV4
dGVybiB2b2lkIEV4cGxhaW5DbG9zZUdyb3VwKGNvbnN0IGNoYXIgKm9ianR5cGUsIGNvbnN0
IGNoYXIgKmxhYmVsbmFtZSwKIAkJCQkJCQkgIGJvb2wgbGFiZWxlZCwgRXhwbGFpblN0YXRl
ICplcyk7CiAKKy8qIEZ1bmN0aW9uIHRvIGhhbmRsZSB0aGUgc2lnbmFsIHRvIG91dHB1dCB0
aGUgcXVlcnkgcGxhbi4gKi8KIGV4dGVybiB2b2lkIEhhbmRsZUxvZ1F1ZXJ5UGxhbkludGVy
cnVwdCh2b2lkKTsKIGV4dGVybiB2b2lkIFByb2Nlc3NMb2dRdWVyeVBsYW5JbnRlcnJ1cHQo
dm9pZCk7CiAjZW5kaWYJCQkJCQkJLyogRVhQTEFJTl9IICovCmRpZmYgLS1naXQgYS9zcmMv
aW5jbHVkZS90Y29wL3BxdWVyeS5oIGIvc3JjL2luY2x1ZGUvdGNvcC9wcXVlcnkuaAppbmRl
eCAyMjdkMjRiOWQ2MC4uZDA0ZGUxZjU1NjYgMTAwNjQ0Ci0tLSBhL3NyYy9pbmNsdWRlL3Rj
b3AvcHF1ZXJ5LmgKKysrIGIvc3JjL2luY2x1ZGUvdGNvcC9wcXVlcnkuaApAQCAtMjIsNyAr
MjIsNiBAQCBzdHJ1Y3QgUGxhbm5lZFN0bXQ7CQkJCS8qIGF2b2lkIGluY2x1ZGluZyBwbGFu
bm9kZXMuaCBoZXJlICovCiAKIGV4dGVybiBQR0RMTElNUE9SVCBQb3J0YWwgQWN0aXZlUG9y
dGFsOwogZXh0ZXJuIFBHRExMSU1QT1JUIFF1ZXJ5RGVzYyAqQWN0aXZlUXVlcnlEZXNjOwot
ZXh0ZXJuIFBHRExMSU1QT1JUIFF1ZXJ5RGVzYyAqc2F2ZV9BY3RpdmVRdWVyeURlc2M7CiAK
IAogZXh0ZXJuIFBvcnRhbFN0cmF0ZWd5IENob29zZVBvcnRhbFN0cmF0ZWd5KExpc3QgKnN0
bXRzKTsK

--------------Y4a1ORitzvUUbBW01Ds0f070--