Content-Type: multipart/mixed; boundary="------------0XLpiGYyZzvrkdRrXMBuP3ba"
Message-ID: <89b1f7e0-c8a1-5c2e-e35c-a89688f79b76@postgrespro.ru>
Date: Fri, 16 Sep 2022 21:51:01 +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
From: "a.rybakina" <a.rybakina@postgrespro.ru>
Subject: RFC: Logging plan of the running query
To: torikoshia@oss.nttdata.com
Cc: pgsql-hackers@postgresql.org
Archived-At: 
 <https://www.postgresql.org/message-id/89b1f7e0-c8a1-5c2e-e35c-a89688f79b76%40postgrespro.ru>
Precedence: bulk

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

Hi,

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

--------------0XLpiGYyZzvrkdRrXMBuP3ba
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

--------------0XLpiGYyZzvrkdRrXMBuP3ba--