Subject: Re: RFC: Logging plan of the running query

From: James Coleman <[email-masked]>
Date: Wed, 21 Feb 2024 19:54:57 -0500

On Mon, Feb 19, 2024 at 11:53 PM Robert Haas <[email-masked]> wrote:
>
> On Fri, Feb 16, 2024 at 12:29 AM Andres Freund <[email-masked]> wrote:
> > If we went with something like tht approach, I think we'd have to do something
> > like redirecting node->ExecProcNode to a wrapper, presumably from within a
> > CFI. That wrapper could then implement the explain support, without slowing
> > down the normal execution path.
>
> That's an annoying complication; maybe there's some better way to
> handle this. But I think we need to do something different than what
> the patch does currently because...
>
> > > It's really hard for me to accept that the heavyweight lock problem
> > > for which the patch contains a workaround is the only one that exists.
> > > I can't see any reason why that should be true.
> >
> > I suspect you're right.
>
> ...I think the current approach is just plain dead, because of this
> issue. We can't take an approach that creates an unbounded number of
> unclear reentrancy issues and then insert hacks one by one to cure
> them (or hack around them, more to the point) as they're discovered.
>
> The premise has to be that we only allow logging the query plan at
> points where we know it's safe, rather than, as at present, allowing
> it in places that are unsafe and then trying to compensate with code
> elsewhere. That's not likely to ever be as stable as we want
> PostgreSQL to be.

This is potentially a bit of a wild idea, but I wonder if having some
kind of argument to CHECK_FOR_INTERRUPTS() signifying we're in
"normal" as opposed to "critical" (using that word differently than
the existing critical sections) would be worth it.

Regards,
James Coleman

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 22 Feb 2024 09:36:06 +0530

> the existing critical sections) would be worth it.

My hunch is this will end up being a maintenance burden since every
caller has to decide (carefully) whether the call is under normal
condition or not. Developers will tend to take a safe approach and
flag calls as critical. But importantly, what's normal for one
interrupt action may be critical for another and vice versa. Approach
would be useful depending upon how easy it is to comprehend the
definition of "normal".

If a query executes for longer than a user defined threashold (session
level GUC? or same value as auto_explain parameter), the executor
proactively prepares an EXPLAIN output and keeps it handy in case
asked for. It can do so at a "known" safe place and time rather than
at any random time and location. Extra time spent in creating EXPLAIN
output may not be noticeable in a long running query. The EXPLAIN
output could be saved in pg_stat_activity or similar place. This will
avoid signaling the backend.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Robert Haas <[email-masked]>
Date: Fri, 23 Feb 2024 10:22:32 +0530

> the existing critical sections) would be worth it.

It's worth considering, but the definition of "normal" vs. "critical"
might be hard to pin down. Or, we might end up with a definition that
is specific to this particular case and not generalizable to others.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Julien Rouhaud <[email-masked]>
Date: Fri, 23 Feb 2024 22:20:51 +0800

Hi,

> is specific to this particular case and not generalizable to others.

But it doesn't have to be all or nothing right?  I mean each call could say
what the situation is like in their context, like
CHECK_FOR_INTERRUPTS(GUARANTEE_NO_HEAVYWEIGHT_LOCK | GUARANTEE_WHATEVER), and
slowly tag calls as needed, similarly to how we add already CFI based on users
report.

---

From: Robert Haas <[email-masked]>
Date: Fri, 23 Feb 2024 20:53:22 +0530

> slowly tag calls as needed, similarly to how we add already CFI based on users

Absolutely. My gut feeling is that it's going to be simpler to pick a
small number of places that are safe and sufficient for this
particular feature and add an extra call there, similar to how we do
vacuum_delay_point(). The reason I think that's likely to be better is
that it will likely require changing only a relatively small number of
places. If we instead start annotating CFIs, well, we've got hundreds
of those. That's a lot more to change, and it also inconveniences
third-party extension authors and people doing back-patching. I'm not
here to say it can't work; I just think it's likely not the easiest
path.

---

From: James Coleman <[email-masked]>
Date: Sat, 24 Feb 2024 08:56:41 -0500

> here to say it can't work; I just think it's likely not the easiest

Yes, I suspect it's not the easiest path. I have a small hunch it
might end up paying more dividends in the future: there isn't just one
of these things that is regularly a thorny discussion for the same
reasons each time (basically "no way to trigger this safely from
another backend interrupting another one at an arbitrary point"), and
if we were able to generalize a solution we may have multiple wins (a
very obvious one in my mind is the inability of auto explain to run an
explain at the precise time it's most useful: when statement timeout
fires).

But it's also possible there are simply ways that get us more than
this scenario also, so I might be wrong; it's merely a hunch.

Regards,
James Coleman

---

From: Julien Rouhaud <[email-masked]>
Date: Sun, 25 Feb 2024 19:30:37 +0800

> explain at the precise time it's most useful: when statement timeout

Yeah, trying to find a generalized solution seems like worth investing some
time to avoid having a bunch of CHECK_FOR_XXX() calls scattered in the code a
few years down the road.

I might be missing something, but since we already have a ton of macro hacks,
why not get another to allow CFI() overloading rather than modifying every
single call?  Something like that should do the trick (and CFIFlagHandler() is
just a naive example with a function call to avoid multiple evaluation, should
be replaced with anything that required more than 10s thinking):

#define CHECK_FOR_INTERRUPTS_0() \
do { \
	if (INTERRUPTS_PENDING_CONDITION()) \
		ProcessInterrupts(); \
} while(0)

#define CHECK_FOR_INTERRUPTS_1(f) \
do { \
	if (INTERRUPTS_PENDING_CONDITION()) \
		ProcessInterrupts(); \
	\
	CFIFlagHandler(f); \
} while(0)

#define CHECK_FOR_INTERRUPTS_X(x, f, CFI_IMPL, ...) CFI_IMPL

#define CHECK_FOR_INTERRUPTS(...) \
	CHECK_FOR_INTERRUPTS_X(, ##__VA_ARGS__, \
							CHECK_FOR_INTERRUPTS_1(__VA_ARGS__), \
							CHECK_FOR_INTERRUPTS_0(__VA_ARGS__) \
						)

We would have to duplicate the current CFI body, but it should never really
change, and we shouldn't end up with more than 2 overloads anyway so I don't
see it being much of a problem.

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 26 Feb 2024 12:19:42 +0530

> > > > slowly tag calls as needed, similarly to how we add already CFI based on users

That has some potential ...

>From your earlier description I thought you are talking about flags
that can be ORed. We need only two macros above. Why are we calling
CFIFLagHandler() after ProcessInterrupts()? Shouldn't we pass flags to
ProcessInterrupts() itself.

> see it being much of a problem.

Why do we need this complex macro?

-- 
Best Wishes,
Ashutosh Bapat

---

From: Julien Rouhaud <[email-masked]>
Date: Mon, 26 Feb 2024 15:55:04 +0800

> ProcessInterrupts() itself.

Yes, I'm still talking about ORed flags passed to CFI().  That CFIFlagHandler
call is just an example for a generalized function that would act those flags,
rather than having it coded inside the macro.
> Why do we need this complex macro?

So that client code can use either CHECK_FOR_INTERRUPTS() or
CHECK_FOR_INTERRUPTS(flag) rather that transforming every single
CHECK_FOR_INTERRUPTS() to CHECK_FOR_INTERRUPTS(0), which was Robert's
complaint about this approach.

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 26 Feb 2024 13:45:08 +0530

> complaint about this approach.

It might be better to just create two marcos  (with names like
CHECK_FOR_INTERRUPTS() and CHECK_FOR_INTERRUPTS_SAFE()) to call
ProcessInterrupt() directly and modify ProcessInterrupts() to accept
the flag (and if required call CFIFlagHandler() additionally). Last
macro is hard to understand.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Robert Haas <[email-masked]>
Date: Mon, 26 Feb 2024 13:56:44 +0530

> time to avoid having a bunch of CHECK_FOR_XXX() calls scattered in the code a

I just don't really see how to do it. I suspect that every task that
wants to run at some CFIs but not others is going to have slightly
different requirements, and we probably can't foresee what all of
those requirements are.

Said another way, if in the future we want to call
DoSomethingOrOther() from the CFI handler, well then we need to know
that we're not already in the middle of using any subsystem that
DoSomethingOrOther() might also try to use ... and we also need to
know that we're not in the middle of doing anything that's more
critical than DoSomethingOrOther(). But both of these are likely to
vary in each case.

EXPLAIN might be one member of a general class of things that require
catalog access (and thus might take locks or lwlocks, access the
catalogs, trigger invalidations, etc.) but it's not clear how general
that class really is. Also, I think if we try to do too many different
kinds of things at CFIs, the whole thing is going to fall apart.
You'll end up failing to foresee some interactions, or the stack will
get too deep, or ... something.

---

From: torikoshia <[email-masked]>
Date: Mon, 26 Feb 2024 21:01:45 +0900

On 2024-02-24 00:23, Robert Haas wrote:
> particular feature and add an extra call there

Hmm, whether extending CHECK_FOR_INTERRUPTS() or adding extras call 
directly, currently I'm not sure where are the good 'places', which 
don't give performance impact.

As attached PoC patch, I experimentally added extra calls on 
ExecScanFetch() which would be less called than ExecProcNode()[1].
When running sequential scan on pgbench_accounts which is on the memory, 
there seems a performance degradation.

   - Executed "select * from pgbench_accounts" for 20 times
   - Compared the elapsed time between the patch applied and not applied 
on 874d817baa160ca7e68
   - there were no heap_blks_read during the query
   - pgbench_accounts has 3000000 rows

   patch NOT applied:
   - average: 335.88 ms
   - max: 367.313 ms
   - min: 309.609 ms

   patch applied:
   - average: 342.57 ms
   - max: 380.099 ms
   - min: 324.270 ms

It would be nice if there was a place accessed once every few seconds or 
so..

[1] 
https://www.postgresql.org/message-id/20240215185911.v4o6fo444md6a3w7%40awork3.anarazel.de

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

From dee1ce857c89bf646ff98f94c846746b96869ebb Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email-masked]>
Date: Mon, 26 Feb 2024 19:19:35 +0900
Subject: [PATCH v37] Add function to log the plan of the query

Currently, we have to wait for the query execution to finish
to check its plan. This is not so convenient when
investigating long-running queries on production environments
where we cannot use debuggers.
To improve this situation, this patch adds
pg_log_query_plan() function that requests to log the
plan of the specified backend process.

By default, only superusers are allowed to request to log the
plans because allowing any users to issue this request at an
unbounded rate would cause lots of log messages and which can
lead to denial of service.

On receipt of the request, at the next
CHECK_LOG_QUERY_PLAN_PENDING() the target backend logs its
plan at LOG_SERVER_ONLY level, so that these plans will appear
in the server log but not be sent to the client.

CHECK_LOG_QUERY_PLAN_PENDING() is called from ExecScanFetch()
and ProcessClientReadInterrupt(). The former is intended to
log plans during any kind of scans and the latter is intended
to log that the no query is executing during client read.

---
 contrib/auto_explain/Makefile                 |   2 +
 contrib/auto_explain/auto_explain.c           |  23 +--
 contrib/auto_explain/t/001_auto_explain.pl    |  35 ++++
 doc/src/sgml/func.sgml                        |  50 +++++
 src/backend/access/transam/xact.c             |  19 ++
 src/backend/catalog/system_functions.sql      |   2 +
 src/backend/commands/explain.c                | 176 +++++++++++++++++-
 src/backend/executor/execMain.c               |  19 ++
 src/backend/executor/execScan.c               |   2 +
 src/backend/storage/ipc/procsignal.c          |   4 +
 src/backend/tcop/postgres.c                   |   5 +
 src/backend/utils/init/globals.c              |   2 +
 src/include/catalog/pg_proc.dat               |   6 +
 src/include/commands/explain.h                |  16 ++
 src/include/miscadmin.h                       |   1 +
 src/include/storage/procsignal.h              |   1 +
 src/include/tcop/pquery.h                     |   2 +-
 src/include/utils/elog.h                      |   1 +
 .../injection_points/injection_points.c       |  11 ++
 src/test/regress/expected/misc_functions.out  |  54 +++++-
 src/test/regress/sql/misc_functions.sql       |  41 +++-
 21 files changed, 430 insertions(+), 42 deletions(-)

diff --git a/contrib/auto_explain/Makefile b/contrib/auto_explain/Makefile
index efd127d3ca..64fe0e0573 100644
--- a/contrib/auto_explain/Makefile
+++ b/contrib/auto_explain/Makefile
@@ -8,6 +8,8 @@ PGFILEDESC = "auto_explain - logging facility for execution plans"
 
 TAP_TESTS = 1
 
+EXTRA_INSTALL = src/test/modules/injection_points
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 677c135f59..e041b10b0e 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -401,26 +401,9 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
 			es->format = auto_explain_log_format;
 			es->settings = auto_explain_log_settings;
 
-			ExplainBeginOutput(es);
-			ExplainQueryText(es, queryDesc);
-			ExplainQueryParameters(es, queryDesc->params, auto_explain_log_parameter_max_length);
-			ExplainPrintPlan(es, queryDesc);
-			if (es->analyze && auto_explain_log_triggers)
-				ExplainPrintTriggers(es, queryDesc);
-			if (es->costs)
-				ExplainPrintJITSummary(es, queryDesc);
-			ExplainEndOutput(es);
-
-			/* Remove last line break */
-			if (es->str->len > 0 && es->str->data[es->str->len - 1] == '\n')
-				es->str->data[--es->str->len] = '\0';
-
-			/* Fix JSON to output an object */
-			if (auto_explain_log_format == EXPLAIN_FORMAT_JSON)
-			{
-				es->str->data[0] = '{';
-				es->str->data[es->str->len - 1] = '}';
-			}
+			ExplainAssembleLogOutput(es, queryDesc, auto_explain_log_format,
+									 auto_explain_log_triggers,
+									 auto_explain_log_parameter_max_length);
 
 			/*
 			 * Note: we rely on the existing logging of context or
diff --git a/contrib/auto_explain/t/001_auto_explain.pl b/contrib/auto_explain/t/001_auto_explain.pl
index 0e5b34afa9..36a787174e 100644
--- a/contrib/auto_explain/t/001_auto_explain.pl
+++ b/contrib/auto_explain/t/001_auto_explain.pl
@@ -35,6 +35,8 @@ $node->append_conf('postgresql.conf', "auto_explain.log_min_duration = 0");
 $node->append_conf('postgresql.conf', "auto_explain.log_analyze = on");
 $node->start;
 
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
 # Simple query.
 my $log_contents = query_log($node, "SELECT * FROM pg_class;");
 
@@ -212,4 +214,37 @@ REVOKE SET ON PARAMETER auto_explain.log_format FROM regress_user1;
 DROP USER regress_user1;
 });
 
+# Check that using both auto_explain and pg_log_query_plan() works fine
+
+$node->safe_psql('postgres', q{SELECT injection_points_attach('executor-run', 'logqueryplan')});
+
+$log_contents = query_log(
+	$node,
+	"SELECT * FROM pg_class;",
+	{
+		"auto_explain.log_verbose" => "on",
+		"auto_explain.log_settings" => "on",
+		"auto_explain.log_analyze" => "off",
+		"compute_query_id" => "on"
+	});
+
+like(
+	$log_contents,
+	qr/query plan running on backend with PID/,
+	"with pg_log_query_plan(), pg_log_query_plan() logged");
+
+like(
+	$log_contents,
+	qr/duration: .+ms  plan:/,
+	"with pg_log_query_plan(), auto_explain logged");
+
+$log_contents =~ /(Query Text:.*Query Identifier: \d+).*(Query Text:.*Query Identifier: \d+)/s;
+my $pg_log_plan_query_output = $1;
+my $auto_explain_output = $2;
+
+cmp_ok(
+	$pg_log_plan_query_output, "eq",
+	$auto_explain_output,
+	"with pg_log_plan_query_log(), logged plans are the same");
+
 done_testing();
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e5fa82c161..9a6cd566ef 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27195,6 +27195,25 @@ SELECT collation for ('foo' COLLATE "de_DE");
        </para></entry>
       </row>
 
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_log_query_plan</primary>
+        </indexterm>
+        <function>pg_log_query_plan</function> ( <parameter>pid</parameter> <type>integer</type> )
+        <returnvalue>boolean</returnvalue>
+       </para>
+       <para>
+        Requests to log the plan of the query currently running on the
+        backend with specified process ID.
+        It will be logged at <literal>LOG</literal> message level and
+        will appear in the server log based on the log
+        configuration set (See <xref linkend="runtime-config-logging"/>
+        for more information), but will not be sent to the client
+        regardless of <xref linkend="guc-client-min-messages"/>.
+        </para></entry>
+      </row>
+
       <row>
        <entry role="func_table_entry"><para role="func_signature">
         <indexterm>
@@ -27309,6 +27328,37 @@ LOG:  Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560
     because it may generate a large number of log messages.
    </para>
 
+   <para>
+    <function>pg_log_query_plan</function> can be used
+    to log the plan of a backend process. For example:
+<programlisting>
+postgres=# SELECT pg_log_query_plan(201116);
+ pg_log_query_plan
+---------------------------
+ t
+(1 row)
+</programlisting>
+The format of the query plan is the same as when <literal>VERBOSE</literal>,
+<literal>COSTS</literal>, <literal>SETTINGS</literal> and
+<literal>FORMAT TEXT</literal> are used in the <command>EXPLAIN</command>
+command. For example:
+<screen>
+LOG:  query plan running on backend with PID 201116 is:
+        Query Text: SELECT * FROM pgbench_accounts;
+        Seq Scan on public.pgbench_accounts  (cost=0.00..52787.00 rows=2000000 width=97)
+          Output: aid, bid, abalance, filler
+        Settings: work_mem = '1MB'
+        Query Identifier: 8621255546560739680
+</screen>
+    Note that when statements are executed inside a function, only the
+    plan of the most deeply nested query is logged.
+   </para>
+
+   <para>
+    When a subtransaction is aborted, <function>pg_log_query_plan</function>
+    cannot log the query plan after the abort.
+   </para>
+
   </sect2>
 
   <sect2 id="functions-admin-backup">
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 70ab6e27a1..9f661baf69 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -61,6 +61,7 @@
 #include "storage/procarray.h"
 #include "storage/sinvaladt.h"
 #include "storage/smgr.h"
+#include "tcop/pquery.h"
 #include "utils/builtins.h"
 #include "utils/catcache.h"
 #include "utils/combocid.h"
@@ -2766,6 +2767,15 @@ AbortTransaction(void)
 	 */
 	sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
 
+	/*
+	 * Reset pg_log_query_plan() related global variables.
+	 * When ActiveQueryDesc is referenced after abort, some of its elements
+	 * are freed. To avoid accessing them, reset ActiveQueryDesc here.
+	 */
+	ActiveQueryDesc = NULL;
+	ProcessLogQueryPlanInterruptActive = false;
+	LogQueryPlanPending = false;
+
 	/*
 	 * check the current transaction state
 	 */
@@ -5125,6 +5135,15 @@ AbortSubTransaction(void)
 	 */
 	sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
 
+	/*
+	 * Reset pg_log_query_plan() related global variables.
+	 * When ActiveQueryDesc is referenced after abort, some of its elements
+	 * are freed. To avoid accessing them, reset ActiveQueryDesc here.
+	 */
+	ActiveQueryDesc = NULL;
+	ProcessLogQueryPlanInterruptActive = false;
+	LogQueryPlanPending = false;
+
 	/*
 	 * check the current transaction state
 	 */
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index fe2bb50f46..2e50b596cd 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -757,6 +757,8 @@ REVOKE EXECUTE ON FUNCTION pg_ls_logicalmapdir() FROM PUBLIC;
 
 REVOKE EXECUTE ON FUNCTION pg_ls_replslotdir(text) FROM PUBLIC;
 
+REVOKE EXECUTE ON FUNCTION pg_log_query_plan(integer) FROM PUBLIC;
+
 --
 -- We also set up some things as accessible to standard roles.
 --
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 83d00a4663..da2ee48667 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -21,6 +21,7 @@
 #include "executor/nodeHash.h"
 #include "foreign/fdwapi.h"
 #include "jit/jit.h"
+#include "miscadmin.h"
 #include "nodes/extensible.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -28,7 +29,10 @@
 #include "parser/parsetree.h"
 #include "rewrite/rewriteHandler.h"
 #include "storage/bufmgr.h"
+#include "storage/procarray.h"
+#include "tcop/pquery.h"
 #include "tcop/tcopprot.h"
+#include "utils/backend_status.h"
 #include "utils/builtins.h"
 #include "utils/guc_tables.h"
 #include "utils/json.h"
@@ -40,6 +44,7 @@
 #include "utils/typcache.h"
 #include "utils/xml.h"
 
+bool ProcessLogQueryPlanInterruptActive = false;
 
 /* Hook for plugins to get control in ExplainOneQuery() */
 ExplainOneQuery_hook_type ExplainOneQuery_hook = NULL;
@@ -784,6 +789,37 @@ ExplainPrintSettings(ExplainState *es)
 	}
 }
 
+/*
+ * ExplainAssembleLogOutput -
+ *   Assemble es->str for logging according to specified contents and format
+ */
+
+void
+ExplainAssembleLogOutput(ExplainState *es, QueryDesc *queryDesc, int logFormat,
+						 bool logTriggers, int logParameterMaxLength)
+{
+	ExplainBeginOutput(es);
+	ExplainQueryText(es, queryDesc);
+	ExplainQueryParameters(es, queryDesc->params, logParameterMaxLength);
+	ExplainPrintPlan(es, queryDesc);
+	if (es->analyze && logTriggers)
+		ExplainPrintTriggers(es, queryDesc);
+	if (es->costs)
+		ExplainPrintJITSummary(es, queryDesc);
+	ExplainEndOutput(es);
+
+	/* Remove last line break */
+	if (es->str->len > 0 && es->str->data[es->str->len - 1] == '\n')
+		es->str->data[--es->str->len] = '\0';
+
+	/* Fix JSON to output an object */
+	if (logFormat == EXPLAIN_FORMAT_JSON)
+	{
+		es->str->data[0] = '{';
+		es->str->data[es->str->len - 1] = '}';
+	}
+}
+
 /*
  * ExplainPrintPlan -
  *	  convert a QueryDesc's plan tree to text and append it to es->str
@@ -1694,6 +1730,9 @@ ExplainNode(PlanState *planstate, List *ancestors,
 	/*
 	 * We have to forcibly clean up the instrumentation state because we
 	 * haven't done ExecutorEnd yet.  This is pretty grotty ...
+	 * This cleanup should not be done when the query has already been
+	 * executed and explain has been called by signal, as the target query
+	 * may use instrumentation and clean itself up.
 	 *
 	 * Note: contrib/auto_explain could cause instrumentation to be set up
 	 * even though we didn't ask for it here.  Be careful not to print any
@@ -1701,7 +1740,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
 	 * InstrEndLoop call anyway, if possible, to reduce the number of cases
 	 * auto_explain has to contend with.
 	 */
-	if (planstate->instrument)
+	if (planstate->instrument && !es->signaled)
 		InstrEndLoop(planstate->instrument);
 
 	if (es->analyze &&
@@ -5182,3 +5221,138 @@ escape_yaml(StringInfo buf, const char *str)
 {
 	escape_json(buf, str);
 }
+
+/*
+ * HandleLogQueryPlanInterrupt
+ *		Handle receipt of an interrupt indicating logging the plan of
+ *		the currently running query.
+ *
+ * All the actual work is deferred to ProcessLogQueryPlanInterrupt(),
+ * because we cannot safely emit a log message inside the signal handler.
+ */
+void
+HandleLogQueryPlanInterrupt(void)
+{
+	/*
+	 * Do not set InterruptPending since ProcessLogQueryPlanInterrupt() is not
+	 * called from CHECK_FOR_INTERRUPTS().
+	 */
+	LogQueryPlanPending = true;
+	/* latch will be set by procsignal_sigusr1_handler */
+}
+
+/*
+ * ProcessLogQueryPlanInterrupt
+ * 		Perform logging the plan of the currently running query on this
+ * 		backend process.
+ *
+ * Any backend that participates in ProcSignal signaling must arrange to call
+ * this function if we see LogQueryPlanPending set.
+ * It is called from CHECK_FOR_INTERRUPTS(), which is enough because the target
+ * process for logging plan is a backend.
+ */
+void
+ProcessLogQueryPlanInterrupt(void)
+{
+	ExplainState *es;
+	MemoryContext cxt;
+	MemoryContext old_cxt;
+	LogQueryPlanPending = false;
+
+	ereport(LOG_SERVER_ONLY,
+			errmsg("ProcessLogQueryPlanInterrupt"),
+			 errhidestmt(true),
+			 errhidecontext(true));
+
+	/* Cannot re-enter. */
+	if (ProcessLogQueryPlanInterruptActive)
+		return;
+
+	ProcessLogQueryPlanInterruptActive = true;
+
+	if (ActiveQueryDesc == NULL)
+	{
+		ereport(LOG_SERVER_ONLY,
+				errmsg("backend with PID %d is not running a query or a subtransaction is aborted",
+					MyProcPid),
+				errhidestmt(true),
+				errhidecontext(true));
+
+		ProcessLogQueryPlanInterruptActive = false;
+		return;
+	}
+
+	cxt = AllocSetContextCreate(CurrentMemoryContext,
+								"log_query_plan temporary context",
+								ALLOCSET_DEFAULT_SIZES);
+
+	old_cxt = MemoryContextSwitchTo(cxt);
+
+	es = NewExplainState();
+
+	es->format = EXPLAIN_FORMAT_TEXT;
+	es->settings = true;
+	es->verbose = true;
+	es->signaled = true;
+
+	ExplainAssembleLogOutput(es, ActiveQueryDesc, EXPLAIN_FORMAT_TEXT, 0, -1);
+
+	ereport(LOG_SERVER_ONLY,
+			errmsg("query plan running on backend with PID %d is:\n%s",
+					MyProcPid, es->str->data),
+			 errhidestmt(true),
+			 errhidecontext(true));
+
+	MemoryContextSwitchTo(old_cxt);
+	MemoryContextDelete(cxt);
+
+	ProcessLogQueryPlanInterruptActive = false;
+}
+
+/*
+ * pg_log_query_plan
+ *		Signal a backend process to log the query plan of the running query.
+ *
+ * By default, only superusers are allowed to signal to log the plan because
+ * allowing any users to issue this request at an unbounded rate would
+ * cause lots of log messages and which can lead to denial of service.
+ * Additional roles can be permitted with GRANT.
+ */
+Datum
+pg_log_query_plan(PG_FUNCTION_ARGS)
+{
+	int			pid = PG_GETARG_INT32(0);
+	PGPROC	   *proc;
+	PgBackendStatus	*be_status;
+
+	proc = BackendPidGetProc(pid);
+
+	if (proc == NULL)
+	{
+		/*
+		 * This is just a warning so a loop-through-resultset will not abort
+		 * if one backend terminated on its own during the run.
+		 */
+		ereport(WARNING,
+				(errmsg("PID %d is not a PostgreSQL backend process", pid)));
+		PG_RETURN_BOOL(false);
+	}
+
+	be_status = pgstat_get_beentry_by_backend_id(proc->backendId);
+	if (be_status->st_backendType != B_BACKEND)
+	{
+		ereport(WARNING,
+				(errmsg("PID %d is not a PostgreSQL client backend process", pid)));
+		PG_RETURN_BOOL(false);
+	}
+
+	if (SendProcSignal(pid, PROCSIG_LOG_QUERY_PLAN, proc->backendId) < 0)
+	{
+		/* Again, just a warning to allow loops */
+		ereport(WARNING,
+				(errmsg("could not send signal to process %d: %m", pid)));
+		PG_RETURN_BOOL(false);
+	}
+
+	PG_RETURN_BOOL(true);
+}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 13a9b7da83..4eee998161 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -61,6 +61,7 @@
 #include "tcop/utility.h"
 #include "utils/acl.h"
 #include "utils/backend_status.h"
+#include "utils/injection_point.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/partcache.h"
@@ -78,6 +79,9 @@ ExecutorEnd_hook_type ExecutorEnd_hook = NULL;
 /* Hook for plugin to get control in ExecCheckPermissions() */
 ExecutorCheckPerms_hook_type ExecutorCheckPerms_hook = NULL;
 
+/* Currently executing query's QueryDesc */
+QueryDesc *ActiveQueryDesc = NULL;
+
 /* decls for local routines only used within this module */
 static void InitPlan(QueryDesc *queryDesc, int eflags);
 static void CheckValidRowMarkRel(Relation rel, RowMarkType markType);
@@ -303,10 +307,25 @@ ExecutorRun(QueryDesc *queryDesc,
 			ScanDirection direction, uint64 count,
 			bool execute_once)
 {
+	/*
+	 * Update ActiveQueryDesc here to enable retrieval of the currently
+	 * running queryDesc for nested queries.
+	 */
+	QueryDesc *save_ActiveQueryDesc;
+
+	save_ActiveQueryDesc = ActiveQueryDesc;
+	ActiveQueryDesc = queryDesc;
+
+#ifdef USE_INJECTION_POINTS
+	INJECTION_POINT("executor-run");
+#endif
+
 	if (ExecutorRun_hook)
 		(*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
 	else
 		standard_ExecutorRun(queryDesc, direction, count, execute_once);
+
+	ActiveQueryDesc = save_ActiveQueryDesc;
 }
 
 void
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index bf4d4133c5..63bd199e4d 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -18,6 +18,7 @@
  */
 #include "postgres.h"
 
+#include "commands/explain.h"
 #include "executor/executor.h"
 #include "miscadmin.h"
 #include "utils/memutils.h"
@@ -39,6 +40,7 @@ ExecScanFetch(ScanState *node,
 	EState	   *estate = node->ps.state;
 
 	CHECK_FOR_INTERRUPTS();
+	CHECK_LOG_QUERY_PLAN_PENDING();
 
 	if (estate->es_epq_active != NULL)
 	{
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 0f9f90d2c7..3c535fbecc 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -20,6 +20,7 @@
 #include "access/parallel.h"
 #include "port/pg_bitutils.h"
 #include "commands/async.h"
+#include "commands/explain.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "replication/logicalworker.h"
@@ -656,6 +657,9 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
 	if (CheckProcSignal(PROCSIG_LOG_MEMORY_CONTEXT))
 		HandleLogMemoryContextInterrupt();
 
+	if (CheckProcSignal(PROCSIG_LOG_QUERY_PLAN))
+		HandleLogQueryPlanInterrupt();
+
 	if (CheckProcSignal(PROCSIG_PARALLEL_APPLY_MESSAGE))
 		HandleParallelApplyMessageInterrupt();
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 59ab812d2e..5ed59c6e85 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -37,6 +37,7 @@
 #include "catalog/pg_type.h"
 #include "commands/async.h"
 #include "commands/event_trigger.h"
+#include "commands/explain.h"
 #include "commands/prepare.h"
 #include "common/pg_prng.h"
 #include "jit/jit.h"
@@ -513,6 +514,7 @@ ProcessClientReadInterrupt(bool blocked)
 	{
 		/* Check for general interrupts that arrived before/while reading */
 		CHECK_FOR_INTERRUPTS();
+		CHECK_LOG_QUERY_PLAN_PENDING();
 
 		/* Process sinval catchup interrupts, if any */
 		if (catchupInterruptPending)
@@ -3460,6 +3462,9 @@ ProcessInterrupts(void)
 	if (LogMemoryContextPending)
 		ProcessLogMemoryContextInterrupt();
 
+	if (LogQueryPlanPending)
+		ProcessLogQueryPlanInterrupt();
+
 	if (ParallelApplyMessagePending)
 		HandleParallelApplyMessages();
 }
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index f024b1a849..8f49a676cc 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -38,6 +38,8 @@ volatile sig_atomic_t IdleSessionTimeoutPending = false;
 volatile sig_atomic_t ProcSignalBarrierPending = false;
 volatile sig_atomic_t LogMemoryContextPending = false;
 volatile sig_atomic_t IdleStatsUpdateTimeoutPending = false;
+volatile sig_atomic_t LogQueryPlanPending = false;
+
 volatile uint32 InterruptHoldoffCount = 0;
 volatile uint32 QueryCancelHoldoffCount = 0;
 volatile uint32 CritSectionCount = 0;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9c120fc2b7..a54bf97c60 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8274,6 +8274,12 @@
   prorettype => 'bool', proargtypes => 'int4',
   prosrc => 'pg_log_backend_memory_contexts' },
 
+# logging plan of the running query on the specified backend
+{ oid => '8000', descr => 'log plan of the running query on the specified backend',
+  proname => 'pg_log_query_plan',
+  provolatile => 'v', prorettype => 'bool',
+  proargtypes => 'int4', prosrc => 'pg_log_query_plan' },
+
 # non-persistent series generator
 { oid => '1066', descr => 'non-persistent series generator',
   proname => 'generate_series', prorows => '1000',
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 7c0f0b5636..f1deb7d7e1 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;
+
 typedef enum ExplainFormat
 {
 	EXPLAIN_FORMAT_TEXT,
@@ -61,6 +63,7 @@ typedef struct ExplainState
 	bool		hide_workers;	/* set if we find an invisible Gather */
 	/* state related to the current plan node */
 	ExplainWorkersState *workers_state; /* needed if parallel plan */
+	bool		signaled;		/* whether explain is called by signal */
 } ExplainState;
 
 /* Hook for plugins to get control in ExplainOneQuery() */
@@ -96,6 +99,10 @@ extern void ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into,
 						   const BufferUsage *bufusage,
 						   const MemoryContextCounters *mem_counters);
 
+extern void ExplainAssembleLogOutput(ExplainState *es, QueryDesc *queryDesc,
+									 int logFormat, bool logTriggers,
+									 int logParameterMaxLength);
+
 extern void ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc);
 extern void ExplainPrintTriggers(ExplainState *es, QueryDesc *queryDesc);
 
@@ -128,4 +135,13 @@ extern void ExplainOpenGroup(const char *objtype, const char *labelname,
 extern void ExplainCloseGroup(const char *objtype, const char *labelname,
 							  bool labeled, ExplainState *es);
 
+extern void HandleLogQueryPlanInterrupt(void);
+extern void ProcessLogQueryPlanInterrupt(void);
+
+#define CHECK_LOG_QUERY_PLAN_PENDING() \
+do { \
+	if (unlikely(LogQueryPlanPending)) \
+		ProcessLogQueryPlanInterrupt(); \
+} while(0)
+
 #endif							/* EXPLAIN_H */
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 612fb5f42e..4ba1e18d1c 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -96,6 +96,7 @@ extern PGDLLIMPORT volatile sig_atomic_t IdleSessionTimeoutPending;
 extern PGDLLIMPORT volatile sig_atomic_t ProcSignalBarrierPending;
 extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending;
 extern PGDLLIMPORT volatile sig_atomic_t IdleStatsUpdateTimeoutPending;
+extern PGDLLIMPORT volatile sig_atomic_t LogQueryPlanPending;
 
 extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending;
 extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost;
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 52dcb4c2ad..400c527292 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -35,6 +35,7 @@ typedef enum
 	PROCSIG_WALSND_INIT_STOPPING,	/* ask walsenders to prepare for shutdown  */
 	PROCSIG_BARRIER,			/* global barrier interrupt  */
 	PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
+	PROCSIG_LOG_QUERY_PLAN, /* ask backend to log plan of the current query */
 	PROCSIG_PARALLEL_APPLY_MESSAGE, /* Message from parallel apply workers */
 
 	/* Recovery conflict reasons */
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 073fb323bc..3dd7edf93c 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -21,7 +21,7 @@ struct PlannedStmt;				/* avoid including plannodes.h here */

 extern PGDLLIMPORT Portal ActivePortal;
-
+extern PGDLLIMPORT QueryDesc *ActiveQueryDesc;
 
 extern PortalStrategy ChoosePortalStrategy(List *stmts);
 
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;
 extern bool errstart(int elevel, const char *domain);
 extern pg_attribute_cold bool errstart_cold(int elevel, const char *domain);
 extern void errfinish(const char *filename, int lineno, const char *funcname);
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index e843e6594f..db0440dd91 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -17,6 +17,7 @@
 
 #include "postgres.h"
 
+#include "commands/explain.h"
 #include "fmgr.h"
 #include "storage/lwlock.h"
 #include "storage/shmem.h"
@@ -28,6 +29,7 @@ PG_MODULE_MAGIC;
 
 extern PGDLLEXPORT void injection_error(const char *name);
 extern PGDLLEXPORT void injection_notice(const char *name);
+extern PGDLLEXPORT void injection_HandleLogQueryPlanInterrupt(const char *name);

 /* Set of callbacks available to be attached to an injection point. */
@@ -43,6 +45,13 @@ injection_notice(const char *name)
 	elog(NOTICE, "notice triggered for injection point %s", name);
 }
 
+void
+injection_HandleLogQueryPlanInterrupt(const char *name)
+{
+	HandleLogQueryPlanInterrupt();
+	elog(LOG, "triggered injection_HandleLogQueryPlanInterrupt for injection point %s", name);
+}
+
 /*
  * SQL function for creating an injection point.
  */
@@ -58,6 +67,8 @@ injection_points_attach(PG_FUNCTION_ARGS)
 		function = "injection_error";
 	else if (strcmp(action, "notice") == 0)
 		function = "injection_notice";
+	else if (strcmp(action, "logqueryplan") == 0)
+		function = "injection_HandleLogQueryPlanInterrupt";
 	else
 		elog(ERROR, "incorrect action \"%s\" for injection point creation", action);
 
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index d5f61dfad9..c51f379b84 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -276,14 +276,15 @@ SELECT test_canonicalize_path('./abc/./def/.././ghi/../../../jkl/mno');
  ../jkl/mno
 (1 row)
 
+-- Test logging functions
 --
--- pg_log_backend_memory_contexts()
---
--- Memory contexts are logged and they are not returned to the function.
+-- The outputs of these functions are logged and they are not returned to
+-- the function.
 -- Furthermore, their contents can vary depending on the timing. However,
 -- we can at least verify that the code doesn't fail, and that the
 -- permissions are set properly.
 --
+-- pg_log_backend_memory_contexts()
 SELECT pg_log_backend_memory_contexts(pg_backend_pid());
  pg_log_backend_memory_contexts 
 --------------------------------
@@ -297,8 +298,8 @@ SELECT pg_log_backend_memory_contexts(pid) FROM pg_stat_activity
  t
 (1 row)
 
-CREATE ROLE regress_log_memory;
-SELECT has_function_privilege('regress_log_memory',
+CREATE ROLE regress_log_function;
+SELECT has_function_privilege('regress_log_function',
   'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
  has_function_privilege 
 ------------------------
@@ -306,15 +307,15 @@ SELECT has_function_privilege('regress_log_memory',
 (1 row)
 
 GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
-  TO regress_log_memory;
-SELECT has_function_privilege('regress_log_memory',
+  TO regress_log_function;
+SELECT has_function_privilege('regress_log_function',
   'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
  has_function_privilege 
 ------------------------
  t
 (1 row)
 
-SET ROLE regress_log_memory;
+SET ROLE regress_log_function;
 SELECT pg_log_backend_memory_contexts(pg_backend_pid());
  pg_log_backend_memory_contexts 
 --------------------------------
@@ -323,8 +324,41 @@ SELECT pg_log_backend_memory_contexts(pg_backend_pid());
 
 RESET ROLE;
 REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
-  FROM regress_log_memory;
-DROP ROLE regress_log_memory;
+  FROM regress_log_function;
+-- pg_log_query_plan()
+SELECT pg_log_query_plan(pg_backend_pid());
+ pg_log_query_plan 
+-------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_log_function',
+  'pg_log_query_plan(integer)', 'EXECUTE'); -- no
+ has_function_privilege 
+------------------------
+ f
+(1 row)
+
+GRANT EXECUTE ON FUNCTION pg_log_query_plan(integer)
+  TO regress_log_function;
+SELECT has_function_privilege('regress_log_function',
+  'pg_log_query_plan(integer)', 'EXECUTE'); -- yes
+ has_function_privilege 
+------------------------
+ t
+(1 row)
+
+SET ROLE regress_log_function;
+SELECT pg_log_query_plan(pg_backend_pid());
+ pg_log_query_plan 
+-------------------
+ t
+(1 row)
+
+RESET ROLE;
+REVOKE EXECUTE ON FUNCTION pg_log_query_plan(integer)
+  FROM regress_log_function;
+DROP ROLE regress_log_function;
 --
 -- Test some built-in SRFs
 --
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index 928b04db7f..8e603b7255 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -68,39 +68,60 @@ SELECT test_canonicalize_path('./abc/./def/.');
 SELECT test_canonicalize_path('./abc/././def/.');
 SELECT test_canonicalize_path('./abc/./def/.././ghi/../../../jkl/mno');
 
+-- Test logging functions
 --
--- pg_log_backend_memory_contexts()
---
--- Memory contexts are logged and they are not returned to the function.
+-- The outputs of these functions are logged and they are not returned to
+-- the function.
 -- Furthermore, their contents can vary depending on the timing. However,
 -- we can at least verify that the code doesn't fail, and that the
 -- permissions are set properly.
 --
 
+-- pg_log_backend_memory_contexts()
+
 SELECT pg_log_backend_memory_contexts(pg_backend_pid());
 
 SELECT pg_log_backend_memory_contexts(pid) FROM pg_stat_activity
   WHERE backend_type = 'checkpointer';
 
-CREATE ROLE regress_log_memory;
+CREATE ROLE regress_log_function;
 
-SELECT has_function_privilege('regress_log_memory',
+SELECT has_function_privilege('regress_log_function',
   'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
 
 GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
-  TO regress_log_memory;
+  TO regress_log_function;
 
-SELECT has_function_privilege('regress_log_memory',
+SELECT has_function_privilege('regress_log_function',
   'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
 
-SET ROLE regress_log_memory;
+SET ROLE regress_log_function;
 SELECT pg_log_backend_memory_contexts(pg_backend_pid());
 RESET ROLE;
 
 REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
-  FROM regress_log_memory;
+  FROM regress_log_function;
+
+-- pg_log_query_plan()
+SELECT pg_log_query_plan(pg_backend_pid());
+
+SELECT has_function_privilege('regress_log_function',
+  'pg_log_query_plan(integer)', 'EXECUTE'); -- no
+
+GRANT EXECUTE ON FUNCTION pg_log_query_plan(integer)
+  TO regress_log_function;
+
+SELECT has_function_privilege('regress_log_function',
+  'pg_log_query_plan(integer)', 'EXECUTE'); -- yes
+
+SET ROLE regress_log_function;
+SELECT pg_log_query_plan(pg_backend_pid());
+RESET ROLE;
+
+REVOKE EXECUTE ON FUNCTION pg_log_query_plan(integer)
+  FROM regress_log_function;
 
-DROP ROLE regress_log_memory;
+DROP ROLE regress_log_function;
 
 --
 -- Test some built-in SRFs

base-commit: 6e951bf98e2e0230ed95db2fafc244536bd7502f
-- 
2.39.2

---

From: Julien Rouhaud <[email-masked]>
Date: Mon, 26 Feb 2024 21:54:41 +0800

> get too deep, or ... something.

I still fail to understand your point.  So you say we might want to check for
safe condition to run explain or DoSomethingOrOther or even DoMeh, with all
different requirements.  IIUC what you're saying is that we should have
CHECK_FOR_EXPLAIN(), CHECK_FOR_DOSOMETHINGOROTHER() and CHECK_FOR_MEH()?

And so in some code path A we could have

CHECK_FOR_INTERRUPTS();
CHECK_FOR_EXPLAIN();

In another

CHECK_FOR_INTERRUPTS();
CHECK_FOR_DOSOMETHINGOROTHER();

and in one happy path

CHECK_FOR_INTERRUPTS();
CHECK_FOR_EXPLAIN();
CHECK_FOR_DOSOMETHINGOROTHER();
CHECK_FOR_MEH();

Or that we should still have all of those but they shouldn't be anywhere close
to a CHECK_FOR_INTERRUPTS(), or something totally different?

In the first case, I'm not sure why having CHECK_FOR_INTERRUPTS(EXPLAIN|...),
with a combination of flags and with the flag handling being done after
ProcessIterrupts(), would be any different apart from having less boilerplate
lines.  Similarly for the 2nd case, but relying on a single more general
CHECK_FOR_CONDITION(EXPLAIN | ...) rather than N CHECK_FOR_XXX?

If you just want something totally different then sure.

---

From: Robert Haas <[email-masked]>
Date: Wed, 28 Feb 2024 11:48:32 +0530

> It would be nice if there was a place accessed once every few seconds or

I think this comment earlier from Andres deserves close attention:

# If we went with something like tht approach, I think we'd have to do something
# like redirecting node->ExecProcNode to a wrapper, presumably from within a
# CFI. That wrapper could then implement the explain support, without slowing
# down the normal execution path.

If this is correctly implemented, the overhead in the case where the
feature isn't used should be essentially zero, I believe.

---

From: James Coleman <[email-masked]>
Date: Sat, 2 Mar 2024 10:46:36 -0500

> feature isn't used should be essentially zero, I believe.

If I can rephrase this idea: it's basically "delay this interrupt
until inline to the next ExecProcNode execution".

That seems pretty promising to me as well.

Regards,
James Coleman

---

From: Robert Haas <[email-masked]>
Date: Mon, 4 Mar 2024 13:19:45 -0500

> until inline to the next ExecProcNode execution".

Yes, but it's not just that. It also means that the code which would
handle the interrupt doesn't need to be called at every ExecProcNode.
Only when the interrupt actually arrives do we enable the code that
handles it.