Re: RFC: Logging plan of the running query - Mailing list pgsql-hackers

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