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

From torikoshia
Subject Re: RFC: Logging plan of the running query
Date
Msg-id b1a41d0d32d70fac75a3ffb7cae8cef7@oss.nttdata.com
Whole thread Raw
In response to Re: RFC: Logging plan of the running query  (jian he <jian.universality@gmail.com>)
List pgsql-hackers
On 2024-03-14 04:33, Robert Haas wrote:
Thanks for your review!

> On Wed, Mar 13, 2024 at 1:28 AM torikoshia <torikoshia@oss.nttdata.com> 
> 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 <torikoshia@oss.nttdata.com> 
> wrote:
>> 
>> On Fri, Feb 16, 2024 at 11:42 PM torikoshia 
>> <torikoshia@oss.nttdata.com>
>> 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



pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: PostgreSQL 17 Release Management Team & Feature Freeze
Next
From: "Hayato Kuroda (Fujitsu)"
Date:
Subject: RE: speed up a logical replica setup