Horton

Re: RFC: Logging plan of the running query

torikoshia <torikoshia@oss.nttdata.com>

View thread
From: torikoshia <torikoshia@oss.nttdata.com>
To: jian he <jian.universality@gmail.com>, robertmhaas@gmail.com
Cc: pgsql-hackers@lists.postgresql.org, James Coleman <jtc331@gmail.com>, Julien Rouhaud <rjuju123@gmail.com>, Andres Freund <andres@anarazel.de>, Étienne BERSAC <etienne.bersac@dalibo.com>, ashutosh.bapat.oss@gmail.com, rafaelthca@gmail.com
Date: 2024-03-18T05:39:48Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. ecpg: Catch zero-length Unicode identifiers correctly

  2. Improve warning message in pg_signal_backend()

  3. Add assert to ensure that page locks don't participate in deadlock cycle.

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