public inbox for [email protected]
help / color / mirror / Atom feedFrom: torikoshia <[email protected]>
To: jian he <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: James Coleman <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Étienne BERSAC <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: RFC: Logging plan of the running query
Date: Mon, 18 Mar 2024 14:39:48 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJufxFr_7c1nTyBKNK1A_Ks0a7N6zT7VEd=QQ8-W_0jMLp1=g@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CACJufxGjtoFxLhXy9JOrfYsXnFyCAMrdu2DTyLPx5Js3qY3J-A@mail.gmail.com>
<[email protected]>
<[email protected]>
<CA+TgmobH+Uto9MCD+vWc71bVbOnd7d8zeYjRT8nXaeLe5hsNJQ@mail.gmail.com>
<[email protected]>
<CA+TgmoZ2Hsb39zzofasjOHXnjsNfJy84YjFF=-KNOO7FsvAi9Q@mail.gmail.com>
<CAAaqYe-gMkdL=M4v47=H0F3+-zi2qL9zFqAv3QsizkRjFiQR0w@mail.gmail.com>
<CA+TgmoYkztFSryutQMNv9DnQ__NDwGE8XhhgnbdCG8e_o4T7yQ@mail.gmail.com>
<hzicr3kidnmwbcqwmwortldzq4fijrfca7c3prncoepgt6rpwf@uydrxvi7qhpk>
<CA+Tgmoa6QniTueRJ1rKcp_-TpVc0c8FZxOtu4ehDop+R=pEcpA@mail.gmail.com>
<[email protected]>
<CA+TgmoYC3_KG8i1WfwGKK6+3YAS7Z0wP0gaTUC4ggEQ=1DrEEg@mail.gmail.com>
<CAAaqYe-7QiAufDmfExAOZKcZTOR5dkuVSMxXRPEooaCaPfFqtA@mail.gmail.com>
<CA+TgmoZVYG+Ot=yPg3HAr2H=2fPvQOyahrx5qwaRf5cqkM7PAQ@mail.gmail.com>
<[email protected]>
<CACJufxFr_7c1nTyBKNK1A_Ks0a7N6zT7VEd=QQ8-W_0jMLp1=g@mail.gmail.com>
On 2024-03-14 04:33, Robert Haas wrote:
Thanks for your review!
> On Wed, Mar 13, 2024 at 1:28 AM torikoshia <[email protected]>
> wrote:
>> - I saw no way to find the next node to be executed from the planstate
>> tree, so the patch wraps all the ExecProcNode of the planstate tree at
>> CHECK_FOR_INTERRUPTS().
>
> I don't think it does this correctly, because some node types have
> children other than the left and right node. See /* special child
> plans */ in ExplainNode().
Agreed.
> But also ... having to wrap the entire plan tree like this seems
> pretty awful. I don't really like the idea of a large-scan plan
> modification like this in the middle of the query.
Yeah, but I haven't come up with other ideas to select the appropriate
node to wrap.
> Andres, did you have some clever idea for this feature that would
> avoid the need to do this?
On 2024-03-14 10:02, jian he wrote:
> On Wed, Mar 13, 2024 at 1:28 PM torikoshia <[email protected]>
> wrote:
>>
>> On Fri, Feb 16, 2024 at 11:42 PM torikoshia
>> <[email protected]>
>> wrote:
>> > I'm not so sure about the implementation now, i.e. finding the next
>> > node
>> > to be executed from the planstate tree, but I'm going to try this
>> > approach.
>>
>> Attached a patch which takes this approach.
>>
>
> one minor issue.
> I understand the regress test, compare the expected outcome with
> testrun outcome,
> but can you enlighten me about how you check if the change you made in
> contrib/auto_explain/t/001_auto_explain.pl is correct.
> (i am not familiar with perl).
$pg_log_plan_query_output saves the output plan of pg_log_query_plan()
and $auto_explain_output saves the output plan of auto_explain.
The test checks the both are the same using cmp_ok().
Did I answer your question?
> diff --git a/src/include/commands/explain.h
> b/src/include/commands/explain.h
> index cf195f1359..2d06bf297e 100644
> --- a/src/include/commands/explain.h
> +++ b/src/include/commands/explain.h
> @@ -17,6 +17,8 @@
> #include "lib/stringinfo.h"
> #include "parser/parse_node.h"
> +extern PGDLLIMPORT bool ProcessLogQueryPlanInterruptActive;
>
> diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
> index 054dd2bf62..3c6bd1ea7c 100644
> --- a/src/include/utils/elog.h
> +++ b/src/include/utils/elog.h
> @@ -167,6 +167,7 @@ struct Node;
> extern bool message_level_is_interesting(int elevel);
> +extern PGDLLIMPORT bool ProcessLogQueryPlanInterruptActive;
>
> utils/elog.h is already included in src/include/postgres.h.
> you don't need to declare ProcessLogQueryPlanInterruptActive at
> include/commands/explain.h?
> generally a variable should only declare once?
Yeah, this declaration is not necessary and we should add include
commands/explain.h to src/backend/access/transam/xact.c.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
view thread (15+ 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], [email protected], [email protected], [email protected], [email protected], [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