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