public inbox for [email protected]
help / color / mirror / Atom feedFrom: a.rybakina <[email protected]>
To: torikoshia <[email protected]>
To: [email protected]
Subject: Re: RFC: Logging plan of the running query
Date: Mon, 19 Sep 2022 13:42:24 +0300
Message-ID: <[email protected]> (raw)
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
[email protected].
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" <[email protected]>:
>
> 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
>
Attachments:
[text/x-patch] diff_query_plan.patch (1.7K, ../[email protected]/3-diff_query_plan.patch)
download | inline diff:
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a82ac87457e..65b692b0ddf 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -306,6 +306,12 @@ ExecutorRun(QueryDesc *queryDesc,
{
QueryDesc *save_ActiveQueryDesc;
+ /*
+ * Save value of ActiveQueryDesc before having called
+ * ExecutorRun_hook function due to having reset by
+ * AbortSubTransaction.
+ */
+
save_ActiveQueryDesc = ActiveQueryDesc;
ActiveQueryDesc = queryDesc;
@@ -314,6 +320,7 @@ ExecutorRun(QueryDesc *queryDesc,
else
standard_ExecutorRun(queryDesc, direction, count, execute_once);
+ /* We set the actual value of ActiveQueryDesc */
ActiveQueryDesc = save_ActiveQueryDesc;
}
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index fc9f9f8e3f0..8e7ce3c976f 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -126,6 +126,7 @@ extern void ExplainOpenGroup(const char *objtype, const char *labelname,
extern void ExplainCloseGroup(const char *objtype, const char *labelname,
bool labeled, ExplainState *es);
+/* Function to handle the signal to output the query plan. */
extern void HandleLogQueryPlanInterrupt(void);
extern void ProcessLogQueryPlanInterrupt(void);
#endif /* EXPLAIN_H */
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 227d24b9d60..d04de1f5566 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -22,7 +22,6 @@ struct PlannedStmt; /* avoid including plannodes.h here */
extern PGDLLIMPORT Portal ActivePortal;
extern PGDLLIMPORT QueryDesc *ActiveQueryDesc;
-extern PGDLLIMPORT QueryDesc *save_ActiveQueryDesc;
extern PortalStrategy ChoosePortalStrategy(List *stmts);
view thread (116+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: RFC: Logging plan of the running query
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox