<div><div>Hi,</div><div><div>I'm sorry,if this message is duplicated previous this one, but I'm not sure that the previous message is sent correctly. I sent it from email address <a href="mailto:a(dot)rybakina(at)postgrespro(dot)ru" rel="noopener noreferrer">a(dot)rybakina(at)postgrespro(dot)ru</a> and I couldn't send this one email from those address.</div><div>I like idea to create patch for logging query plan. After reviewing this code and notice some moments and I'd rather ask you some questions.</div></div><div><br />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.</div></div><div> </div><div> </div><div>-- </div><div>Regards,</div><div>Alena Rybakina<br />Postgres Professional</div><div> </div><div> </div><div> </div><div>19.09.2022, 11:01, "torikoshia" <torikoshia(at)oss(dot)nttdata(dot)com>:</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>