On 2024-03-14 04:33, Robert Haas wrote:
Thanks for your review!
On Wed, Mar 13, 2024 at 1:28 AM torikoshia <torikos...@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 <torikos...@oss.nttdata.com>
wrote:
On Fri, Feb 16, 2024 at 11:42 PM torikoshia
<torikos...@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
