Subject: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

From: Mats Kindahl <[email-masked]>
Date: Mon, 4 Mar 2024 12:59:46 +0100

Hi hackers,

I wanted to hook into the EXPLAIN output for queries and add some extra
information, but since there is no standard_ExplainOneQuery() I had to copy
the code and create my own version.

Since the pattern with other hooks for a function WhateverFunction() seems
to be that there is a standard_WhateverFunction() for each
WhateverFunction_hook, I created a patch to follow this pattern for your
consideration.

I was also considering adding a callback so that you can annotate any node
with explanatory information that is not a custom scan node. This could be
used to propagate and summarize information from custom scan nodes, but I
had no immediate use for that so did not add it here. I would still be
interested in hearing if you think this is something that would be useful
to the community.

Best wishes,
Mats Kindahl, Timescale
From eaab4d7c088ff3ee9b0e6ec3de96677bafd184c0 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <[email-masked]>
Date: Mon, 4 Mar 2024 12:38:05 +0100
Subject: Improve support for ExplainOneQuery hook

There is a hook ExplainOneQuery_hook, but there is no corresponding
standard_ExplainOneQuery and the code that belongs there is written
in-line in ExplainOneQuery(). As a result, anybody adding a hook for
ExplainOneQuery_hook has to copy the code from ExplainOneQuery() to run
the standard ExplainOneQuery before adding their own messages.

This commit fixes this by refactoring ExplainOneQuery() and factor out
the standard code into standard_ExplainOneQuery() and call it from
ExplainOneQuery() in a similar manner to how it is done for other
hooks.
---
 src/backend/commands/explain.c | 106 ++++++++++++++++++---------------
 src/include/commands/explain.h |   4 ++
 2 files changed, 61 insertions(+), 49 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 78754bc6ba..3b7bed3ca2 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -391,63 +391,71 @@ ExplainOneQuery(Query *query, int cursorOptions,
 
 	/* if an advisor plugin is present, let it manage things */
 	if (ExplainOneQuery_hook)
-		(*ExplainOneQuery_hook) (query, cursorOptions, into, es,
-								 queryString, params, queryEnv);
+		(*ExplainOneQuery_hook)(query, cursorOptions, into, es,
+							 queryString, params, queryEnv);
 	else
-	{
-		PlannedStmt *plan;
-		instr_time	planstart,
-					planduration;
-		BufferUsage bufusage_start,
-					bufusage;
-		MemoryContextCounters mem_counters;
-		MemoryContext planner_ctx = NULL;
-		MemoryContext saved_ctx = NULL;
-
-		if (es->memory)
-		{
-			/*
-			 * Create a new memory context to measure planner's memory
-			 * consumption accurately.  Note that if the planner were to be
-			 * modified to use a different memory context type, here we would
-			 * be changing that to AllocSet, which might be undesirable.
-			 * However, we don't have a way to create a context of the same
-			 * type as another, so we pray and hope that this is OK.
-			 */
-			planner_ctx = AllocSetContextCreate(CurrentMemoryContext,
-												"explain analyze planner context",
-												ALLOCSET_DEFAULT_SIZES);
-			saved_ctx = MemoryContextSwitchTo(planner_ctx);
-		}
+		standard_ExplainOneQuery(query, cursorOptions, into, es,
+								 queryString, params, queryEnv);
+}
 
-		if (es->buffers)
-			bufusage_start = pgBufferUsage;
-		INSTR_TIME_SET_CURRENT(planstart);
+void
+standard_ExplainOneQuery(Query *query, int cursorOptions,
+						 IntoClause *into, ExplainState *es,
+						 const char *queryString, ParamListInfo params,
+						 QueryEnvironment *queryEnv)
+{
+	PlannedStmt *plan;
+	instr_time	planstart,
+				planduration;
+	BufferUsage bufusage_start,
+				bufusage;
+	MemoryContextCounters mem_counters;
+	MemoryContext planner_ctx = NULL;
+	MemoryContext saved_ctx = NULL;
+
+	if (es->memory)
+	{
+		/*
+		 * Create a new memory context to measure planner's memory consumption
+		 * accurately.  Note that if the planner were to be modified to use a
+		 * different memory context type, here we would be changing that to
+		 * AllocSet, which might be undesirable.  However, we don't have a way
+		 * to create a context of the same type as another, so we pray and
+		 * hope that this is OK.
+		 */
+		planner_ctx = AllocSetContextCreate(CurrentMemoryContext,
+											"explain analyze planner context",
+											ALLOCSET_DEFAULT_SIZES);
+		saved_ctx = MemoryContextSwitchTo(planner_ctx);
+	}
 
-		/* plan the query */
-		plan = pg_plan_query(query, queryString, cursorOptions, params);
+	if (es->buffers)
+		bufusage_start = pgBufferUsage;
+	INSTR_TIME_SET_CURRENT(planstart);
 
-		INSTR_TIME_SET_CURRENT(planduration);
-		INSTR_TIME_SUBTRACT(planduration, planstart);
+	/* plan the query */
+	plan = pg_plan_query(query, queryString, cursorOptions, params);
 
-		if (es->memory)
-		{
-			MemoryContextSwitchTo(saved_ctx);
-			MemoryContextMemConsumed(planner_ctx, &mem_counters);
-		}
+	INSTR_TIME_SET_CURRENT(planduration);
+	INSTR_TIME_SUBTRACT(planduration, planstart);
 
-		/* calc differences of buffer counters. */
-		if (es->buffers)
-		{
-			memset(&bufusage, 0, sizeof(BufferUsage));
-			BufferUsageAccumDiff(&bufusage, &pgBufferUsage, &bufusage_start);
-		}
+	if (es->memory)
+	{
+		MemoryContextSwitchTo(saved_ctx);
+		MemoryContextMemConsumed(planner_ctx, &mem_counters);
+	}
 
-		/* run it (if needed) and produce output */
-		ExplainOnePlan(plan, into, es, queryString, params, queryEnv,
-					   &planduration, (es->buffers ? &bufusage : NULL),
-					   es->memory ? &mem_counters : NULL);
+	/* calc differences of buffer counters. */
+	if (es->buffers)
+	{
+		memset(&bufusage, 0, sizeof(BufferUsage));
+		BufferUsageAccumDiff(&bufusage, &pgBufferUsage, &bufusage_start);
 	}
+
+	/* run it (if needed) and produce output */
+	ExplainOnePlan(plan, into, es, queryString, params, queryEnv,
+				   &planduration, (es->buffers ? &bufusage : NULL),
+				   es->memory ? &mem_counters : NULL);
 }
 
 /*
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 7c0f0b5636..cf195f1359 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -80,6 +80,10 @@ extern PGDLLIMPORT explain_get_index_name_hook_type explain_get_index_name_hook;
 
 extern void ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 						 ParamListInfo params, DestReceiver *dest);
+extern void standard_ExplainOneQuery(Query *query, int cursorOptions,
+									 IntoClause *into, ExplainState *es,
+									 const char *queryString, ParamListInfo params,
+									 QueryEnvironment *queryEnv);
 
 extern ExplainState *NewExplainState(void);
 
-- 
2.34.1

---

From: Aleksander Alekseev <[email-masked]>
Date: Mon, 4 Mar 2024 15:41:16 +0300

Hi,

> I wanted to hook into the EXPLAIN output for queries and add some extra information, but since there is no standard_ExplainOneQuery() I had to copy the code and create my own version.
>
> Since the pattern with other hooks for a function WhateverFunction() seems to be that there is a standard_WhateverFunction() for each WhateverFunction_hook, I created a patch to follow this pattern for your consideration.
>
> I was also considering adding a callback so that you can annotate any node with explanatory information that is not a custom scan node. This could be used to propagate and summarize information from custom scan nodes, but I had no immediate use for that so did not add it here. I would still be interested in hearing if you think this is something that would be useful to the community.

Thanks for the patch. LGTM.

I registered the patch on the nearest open CF [1] and marked it as
RfC. It is a pretty straightforward refactoring.

[1]: https://commitfest.postgresql.org/48/4879/

-- 
Best regards,
Aleksander Alekseev

---

From: Michael Paquier <[email-masked]>
Date: Tue, 5 Mar 2024 15:31:25 +0900

On Mon, Mar 04, 2024 at 03:41:16PM +0300, Aleksander Alekseev wrote:
>> I wanted to hook into the EXPLAIN output for queries and add some
>> extra information, but since there is no standard_ExplainOneQuery() I
>> had to copy the code and create my own version. 
>>
>> Since the pattern with other hooks for a function
>> WhateverFunction() seems to be that there is a
>> standard_WhateverFunction() for each WhateverFunction_hook, I
>> created a patch to follow this pattern for your consideration.

So you've wanted to be able to add some custom information at the end
or the beginning of ExplainState's output buffer, before falling back
to the in-core path.  What was the use case, if I may ask?

>> I was also considering adding a callback so that you can annotate
>> any node with explanatory information that is not a custom scan
>> node. This could be used to propagate and summarize information
>> from custom scan nodes, but I had no immediate use for that so did
>> not add it here. I would still be interested in hearing if you
>> think this is something that would be useful to the community.

That depends.

> [1]: https://commitfest.postgresql.org/48/4879/

I know that we're in the middle of commit fest 47 while this is in 48,
but I can't really see a reason why we should not do that earlier than
v18.  One point about core is to be flexible for extension code.  So I\
have no objections, others are free to comment.
--
Michael

---

From: Mats Kindahl <[email-masked]>
Date: Tue, 5 Mar 2024 08:21:34 +0100

> to the in-core path.  What was the use case, if I may ask?

Yes, that was the use-case. We have some caches added by extending
ExecutorStart and other executor-related functions using the hooks there
and want to show cache hits and misses in the plan.

I realize that a more advanced system is possible to create where you can
customize the output even more, but in this case I just wanted to add a
section with some additional information related to plan execution. Also,
the code in explain.c seems to not be written with extensibility in mind,
so I did not want to make too big a change here before thinking through how
this would work.

> >> think this is something that would be useful to the community.

Just to elaborate: the intention was to allow a section to be added to
every node in the plan containing information from further down and also
allow this information to propagate upwards. We happen to have buffer
information right now, but allowing something similar to be added
dynamically by extending ExplainNode and passing down a callback to
standard_ExplainOneQuery.

Best wishes,
Mats Kindahl

---

From: Michael Paquier <[email-masked]>
Date: Wed, 6 Mar 2024 08:25:16 +0900

> so I did not want to make too big a change here before thinking through how

Sure.

> standard_ExplainOneQuery.

Or an extra hook at the end of ExplainNode() to be able to append more
information at node level?  Not sure if others would agree with that,
though.
--
Michael

---

From: Andrei Lepikhov <[email-masked]>
Date: Wed, 6 Mar 2024 09:26:49 +0700

> information at node level?  Not sure if others would agree with that,

We already discussed EXPLAIN hooks, at least in [1]. IMO, extensions 
should have a chance to add something to the node explain and the 
summary, if only because they can significantly influence the planner 
and executor's behaviour.

[1] 
https://www.postgresql.org/message-id/flat/6cd5caa7-06e1-4460-bf35-00a59da3f677%40garret.ru

-- 
regards,
Andrei Lepikhov
Postgres Professional

---

From: Mats Kindahl <[email-masked]>
Date: Wed, 6 Mar 2024 08:31:31 +0100

> > information at node level?  Not sure if others would agree with that,

That is what I had in mind, yes.

> https://www.postgresql.org/message-id/flat/6cd5caa7-06e1-4460-bf35-00a59da3f677%40garret.ru

This is an excellent example of where such a hook would be useful.
-- 
Best wishes,
Mats Kindahl, Timescale