[HACKERS] PoC: custom signal handler for extensions
Hi, hackers!
I want to propose the patch that allows to define custom signals and
their handlers on extension side. It is based on ProcSignal module,
namely it defines the fixed set (number is specified by constant) of
custom signals that could be reserved on postgres initialization stage
(in _PG_init function of shared preloaded module) through specific
interface functions. Functions that are custom signal handlers are
defined in extension. The relationship between custom signals and
assigned handlers (function addresses) is replicated from postmaster to
child processes including working backends. Using this signals we are
able to call specific handler (via SendProcSignal function) on remote
backend that is actually come into action in CHECK_FOR_INTERRUPTS point.
In perspective, this mechanism provides the low-level instrument to
define remote procedure call on extension side. The simple RPC that
defines effective userid on remote backend (remote_effective_user
function) is attached for example.
C&C welcome!
--
Regards,
Maksim Milyutin
Attachments:
custom_signals.patchtext/plain; charset=UTF-8; name=custom_signals.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index b9302ac630..75d4ea72b7 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -27,6 +27,7 @@
#include "storage/shmem.h"
#include "storage/sinval.h"
#include "tcop/tcopprot.h"
+#include "utils/memutils.h"
/*
@@ -60,12 +61,17 @@ typedef struct
*/
#define NumProcSignalSlots (MaxBackends + NUM_AUXPROCTYPES)
+static bool CustomSignalPendings[NUM_CUSTOM_PROCSIGNALS];
+static ProcSignalHandler_type CustomHandlers[NUM_CUSTOM_PROCSIGNALS];
+
static ProcSignalSlot *ProcSignalSlots = NULL;
static volatile ProcSignalSlot *MyProcSignalSlot = NULL;
static bool CheckProcSignal(ProcSignalReason reason);
static void CleanupProcSignalState(int status, Datum arg);
+static void CustomSignalInterrupt(ProcSignalReason reason);
+
/*
* ProcSignalShmemSize
* Compute space needed for procsignal's shared memory
@@ -166,6 +172,70 @@ CleanupProcSignalState(int status, Datum arg)
}
/*
+ * RegisterCustomProcSignalHandler
+ * Assign specific handler of custom process signal with new ProcSignalReason key
+ *
+ * Return INVALID_PROCSIGNAL if all custom signals have been assigned.
+ */
+ProcSignalReason
+RegisterCustomProcSignalHandler(ProcSignalHandler_type handler)
+{
+ ProcSignalReason reason;
+
+ /* iterate through custom signal keys to find free spot */
+ for (reason = PROCSIG_CUSTOM_1; reason <= PROCSIG_CUSTOM_N; reason++)
+ if (!CustomHandlers[reason - PROCSIG_CUSTOM_1])
+ {
+ CustomHandlers[reason - PROCSIG_CUSTOM_1] = handler;
+ return reason;
+ }
+ return INVALID_PROCSIGNAL;
+}
+
+/*
+ * ReleaseCustomProcSignal
+ * Release slot of specific custom signal
+ */
+void
+ReleaseCustomProcSignal(ProcSignalReason reason)
+{
+ CustomHandlers[reason - PROCSIG_CUSTOM_1] = NULL;
+}
+
+/*
+ * AssignCustomProcSignalHandler
+ * Assign handler of custom process signal with specific ProcSignalReason key
+ *
+ * Return old ProcSignal handler.
+ * Assume incoming reason is one of custom ProcSignals.
+ */
+ProcSignalHandler_type
+AssignCustomProcSignalHandler(ProcSignalReason reason, ProcSignalHandler_type handler)
+{
+ ProcSignalHandler_type old;
+
+ Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N);
+
+ old = CustomHandlers[reason - PROCSIG_CUSTOM_1];
+ CustomHandlers[reason - PROCSIG_CUSTOM_1] = handler;
+ return old;
+}
+
+/*
+ * GetCustomProcSignalHandler
+ * Get handler of custom process signal
+ *
+ * Assume incoming reason is one of custom ProcSignals.
+ */
+ProcSignalHandler_type
+GetCustomProcSignalHandler(ProcSignalReason reason)
+{
+ Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N);
+
+ return CustomHandlers[reason - PROCSIG_CUSTOM_1];
+}
+
+/*
* SendProcSignal
* Send a signal to a Postgres process
*
@@ -260,7 +330,8 @@ CheckProcSignal(ProcSignalReason reason)
void
procsignal_sigusr1_handler(SIGNAL_ARGS)
{
- int save_errno = errno;
+ int save_errno = errno;
+ ProcSignalReason reason;
if (CheckProcSignal(PROCSIG_CATCHUP_INTERRUPT))
HandleCatchupInterrupt();
@@ -292,9 +363,84 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
+ for (reason = PROCSIG_CUSTOM_1; reason <= PROCSIG_CUSTOM_N; reason++)
+ if (CheckProcSignal(reason))
+ CustomSignalInterrupt(reason);
+
SetLatch(MyLatch);
latch_sigusr1_handler();
errno = save_errno;
}
+
+/*
+ * Handle receipt of an interrupt indicating a custom process signal.
+ */
+static void
+CustomSignalInterrupt(ProcSignalReason reason)
+{
+ int save_errno = errno;
+
+ Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N);
+
+ /* set interrupt flags */
+ InterruptPending = true;
+ CustomSignalPendings[reason - PROCSIG_CUSTOM_1] = true;
+
+ /* make sure the event is processed in due course */
+ SetLatch(MyLatch);
+
+ errno = save_errno;
+}
+
+/*
+ * CheckAndHandleCustomSignals
+ * Check custom signal flags and call handler assigned to that signal
+ * if it is not NULL
+ *
+ * This function is called within CHECK_FOR_INTERRUPTS if interrupt have been
+ * occurred. Skeleton is the same as in HandleParallelMessageInterrupt()
+ */
+void
+CheckAndHandleCustomSignals(void)
+{
+ int i;
+ MemoryContext oldcontext;
+
+ static MemoryContext hcs_context = NULL;
+
+ /* Disable interrupts to avoid recursive calls */
+ HOLD_INTERRUPTS();
+
+ /*
+ * Create specific subtop-level memory context in first call of this handler
+ */
+ if (hcs_context == NULL)
+ hcs_context = AllocSetContextCreate(TopMemoryContext,
+ "HandleCustomSignals",
+ ALLOCSET_DEFAULT_SIZES);
+ else
+ MemoryContextReset(hcs_context);
+
+ oldcontext = MemoryContextSwitchTo(hcs_context);
+
+ /* Check on expiring of custom signals and call its handlers if exist */
+ for (i = 0; i < NUM_CUSTOM_PROCSIGNALS; i++)
+ if (CustomSignalPendings[i])
+ {
+ ProcSignalHandler_type handler;
+
+ CustomSignalPendings[i] = false;
+ handler = CustomHandlers[i];
+ if (handler)
+ handler();
+ }
+
+ MemoryContextSwitchTo(oldcontext);
+
+ /* Might as well clear the context on our way out */
+ MemoryContextReset(hcs_context);
+
+ RESUME_INTERRUPTS();
+}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1b24dddbce..b8f54dc74c 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3051,6 +3051,8 @@ ProcessInterrupts(void)
if (ParallelMessagePending)
HandleParallelMessages();
+
+ CheckAndHandleCustomSignals();
}
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 20bb05b177..8122deda86 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -17,6 +17,8 @@
#include "storage/backendid.h"
+#define NUM_CUSTOM_PROCSIGNALS 64
+
/*
* Reasons for signalling a Postgres child process (a backend or an auxiliary
* process, like checkpointer). We can cope with concurrent signals for different
@@ -29,6 +31,8 @@
*/
typedef enum
{
+ INVALID_PROCSIGNAL = -1, /* Must be first */
+
PROCSIG_CATCHUP_INTERRUPT, /* sinval catchup interrupt */
PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
PROCSIG_PARALLEL_MESSAGE, /* message from cooperating parallel backend */
@@ -42,9 +46,20 @@ typedef enum
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
+ PROCSIG_CUSTOM_1,
+ /*
+ * PROCSIG_CUSTOM_2,
+ * ...,
+ * PROCSIG_CUSTOM_N-1,
+ */
+ PROCSIG_CUSTOM_N = PROCSIG_CUSTOM_1 + NUM_CUSTOM_PROCSIGNALS - 1,
+
NUM_PROCSIGNALS /* Must be last! */
} ProcSignalReason;
+/* Handler of custom process signal */
+typedef void (*ProcSignalHandler_type) (void);
+
/*
* prototypes for functions in procsignal.c
*/
@@ -52,9 +67,16 @@ extern Size ProcSignalShmemSize(void);
extern void ProcSignalShmemInit(void);
extern void ProcSignalInit(int pss_idx);
+extern ProcSignalReason RegisterCustomProcSignalHandler(ProcSignalHandler_type handler);
+extern void ReleaseCustomProcSignal(ProcSignalReason reason);
+extern ProcSignalHandler_type AssignCustomProcSignalHandler(ProcSignalReason reason,
+ ProcSignalHandler_type handler);
+extern ProcSignalHandler_type GetCustomProcSignalHandler(ProcSignalReason reason);
extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
BackendId backendId);
+extern void CheckAndHandleCustomSignals(void);
+
extern void procsignal_sigusr1_handler(SIGNAL_ARGS);
#endif /* PROCSIGNAL_H */
+1
if this permits to use extension pg_query_state
<https://github.com/postgrespro/pg_query_state> , that would be great !
--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
23.12.17 12:58, legrand legrand wrote:
+1
if this permits to use extension pg_query_state
<https://github.com/postgrespro/pg_query_state> , that would be great !
Yes, attached patch is the single critical point. It allows to loose
pg_query_state from the need to patch postgres core.
--
Regards,
Maksim Milyutin
Hello,
On Fri, Dec 22, 2017 at 03:05:25PM +0300, Maksim Milyutin wrote:
Hi, hackers!
I want to propose the patch that allows to define custom signals and their
handlers on extension side.
I've looked a little bit on the patch. The patch applyes and regression tests pass.
I have a couple comments.
The relationship between custom signals and
assigned handlers (function addresses) is replicated from postmaster to
child processes including working backends.
I think this happens only if a custom signal registered during initializing shared_preload_libraries.
But from RegisterCustomProcSignalHandler()'s implementation I see that you can register the signal anytime. Am I wrong?
If so then custom signal handlers may not work as expected.
What is purpose of AssignCustomProcSignalHandler() function? This function can erase a handler set by another extension.
For example, extension 1 set handler passing reason PROCSIG_CUSTOM_1, and extension 2 set another handler passing reason PROCSIG_CUSTOM_1 too. Here the handler of the extension 2 will be purged.
+ + Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N); +
I think it is not good to use asserts within AssignCustomProcSignalHandler() and GetCustomProcSignalHandler(). Because this functions can be executed by an external extension, and it may pass a reason outside this boundary. It will be better if the reason will be checked in runtime.
But it is fine for this assert within CustomSignalInterrupt().
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
In perspective, this mechanism provides the low-level instrument to define
remote procedure call on extension side. The simple RPC that defines effective
userid on remote backend (remote_effective_user function) is attached for example.
Suppose, it's useful patch. Some notes:
1) AssignCustomProcSignalHandler is unneeded API function, easy replaced by
GetCustomProcSignalHandler/ReleaseCustomProcSignal/RegisterCustomProcSignalHandler
functions, but in other hand, it isn't looked as widely used feature to reassign
custom signal handler.
2) Naming RegisterCustomProcSignalHandler and ReleaseCustomProcSignal is not
self-consistent. Other parts suggest pair Register/Unregister or Aquire/Release.
Seems, Register/Unregister pair is preferred here.
3) Agree that Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N)
should be replaced with ereport. By the way, ReleaseCustomProcSignal() is a
single place where there isn't a range check of reason arg
4) CustomSignalInterrupt() - play with errno and SetLatch() seems unneeded here
- there isn't call of handler of custom signal, SetLatch will be called several
lines below
5) Using a special memory context for handler call looks questionably, I think
that complicated handlers could manage memory itself (and will do, if they need
to store something between calls). So, I prefer to remove context.
6) As I can see, all possible (not only custom) signal handlers could perfectly
use this API, or, at least, be store in CustomHandlers array, which could be
renamed to InterruptHandlers, for example. Next, custom handler type is possible
to make closier to built-in handlers, let its signature extends to
void (*ProcSignalHandler_type) (ProcSignalReason reason) as others. It will
allow to use single handler to support several reasons.
7) Suppose, API allows to use different handlers in different processes for the
same reason, it's could be reason of confusion. I suggest to restrict
Register/Unregister call only for shared_preload_library, ie only during startup.
8) I'd like to see an example of usage this API somewhere in contrib in exsting
modules. Ideas?
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
This comes from
https://github.com/postgrespro/pg_query_state
Regards
PAscal
--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
On 12.01.2018 18:51, Teodor Sigaev wrote:
In perspective, this mechanism provides the low-level instrument to
define remote procedure call on extension side. The simple RPC that
defines effective userid on remote backend (remote_effective_user
function) is attached for example.Suppose, it's useful patch. Some notes:
1) AssignCustomProcSignalHandler is unneeded API function, easy
replaced by
GetCustomProcSignalHandler/ReleaseCustomProcSignal/RegisterCustomProcSignalHandler
functions, but in other hand, it isn't looked as widely used feature
to reassign custom signal handler.
Agreed. AssignCustomProcSignalHandler is unnecessary. Also I have
removed GetCustomProcSignalHandler as not see any application of this
function.
2) Naming RegisterCustomProcSignalHandler and ReleaseCustomProcSignalО©╫
is not self-consistent. Other parts suggest pair Register/Unregister
or Aquire/Release. Seems, Register/Unregister pair is preferred here.
Thanks for notice. Fixed.
3) Agree that Assert(reason >= PROCSIG_CUSTOM_1 && reason <=
PROCSIG_CUSTOM_N) should be replaced with ereport. By the way,
ReleaseCustomProcSignal() is a single place where there isn't a range
check of reason arg
Oops, I missed this assert check.
I considered assert check in user available routines accepting
procsignal as a precondition for routine's clients, i.e. violation of
this check have to involve uncertain behavior. This checks is not
expensive itself therefore it makes sense to replace their to runtime
checks via ereport calls.
4) CustomSignalInterrupt() - play with errno and SetLatch() seems
unneeded here - there isn't call of handler of custom signal, SetLatch
will be called several lines below
I have noticed that even simple HandleNotifyInterrupt() and
HandleParallelMessageInterrupt() routines add at least
SetLatch(MyLatch). I think it makes sense to leave this call in our
case. Perhaps, I'm wrong. I don't understand entirely the intention of
SetLatch() in those cases.
5) Using a special memory context for handler call looks questionably,
I think that complicated handlers could manage memory itself (and will
do, if they need to store something between calls). So, I prefer to
remove context.
Fixed. But in this case we have to explicitly reflect in documentation
the ambiguity of running memory context under signal handler and the
intention to leave memory management on extension's developer.
6) As I can see, all possible (not only custom) signal handlers could
perfectly use this API, or, at least, be store in CustomHandlers
array, which could be renamed to InterruptHandlers, for example. Next,
custom handler type is possible to make closier to built-in handlers,
let its signature extends to
void (*ProcSignalHandler_type) (ProcSignalReason reason) as others. It
will allow to use single handler to support several reasons.
OK, fixed.
7) Suppose, API allows to use different handlers in different
processes for the same reason, it's could be reason of confusion. I
suggest to restrict Register/Unregister call only for
shared_preload_library, ie only during startup.
Yeah, added checks on process_shared_preload_libraries_in_progress flag.
Thanks for notice.
8) I'd like to see an example of usage this API somewhere in contrib
in exsting modules. Ideas?
Besides of usage in the extension pg_query_state [1] that provides the
way to extract query state from running backend, I see the possibility
with this patch to implement statistical sampling-based profiler for
plpgsql functions on extension side. If we could get a call stack of
plpgsql functions on interruptible backend then there are no obstacles
to organize capturing of stack frames at some intervals over fixed
period of time defined by arguments in extension's function.
I have attached a new version of patch and updated version of
remote_effective_user function implementation that demonstrates the
usage of custom signals API.
1. https://github.com/postgrespro/pg_query_state
--
Regards,
Maksim Milyutin
Attachments:
custom_signals_v2.patchtext/x-patch; name=custom_signals_v2.patchDownload
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index b0dd7d1..ae7d007 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -60,12 +60,20 @@ typedef struct
*/
#define NumProcSignalSlots (MaxBackends + NUM_AUXPROCTYPES)
+#define IsCustomProcSignalReason(reason) \
+ ((reason) >= PROCSIG_CUSTOM_1 && (reason) <= PROCSIG_CUSTOM_N)
+
+static bool CustomSignalPendings[NUM_CUSTOM_PROCSIGNALS];
+static ProcSignalHandler_type CustomInterruptHandlers[NUM_CUSTOM_PROCSIGNALS];
+
static ProcSignalSlot *ProcSignalSlots = NULL;
static volatile ProcSignalSlot *MyProcSignalSlot = NULL;
static bool CheckProcSignal(ProcSignalReason reason);
static void CleanupProcSignalState(int status, Datum arg);
+static void CheckAndSetCustomSignalInterrupts(void);
+
/*
* ProcSignalShmemSize
* Compute space needed for procsignal's shared memory
@@ -166,6 +174,58 @@ CleanupProcSignalState(int status, Datum arg)
}
/*
+ * RegisterCustomProcSignalHandler
+ * Assign specific handler of custom process signal with new
+ * ProcSignalReason key.
+ *
+ * This function have to be called in _PG_init function of extensions at the
+ * stage of loading shared preloaded libraries. Otherwise it throws fatal error.
+ *
+ * Return INVALID_PROCSIGNAL if all slots for custom signals have been occupied.
+ */
+ProcSignalReason
+RegisterCustomProcSignalHandler(ProcSignalHandler_type handler)
+{
+ ProcSignalReason reason;
+
+ if (!process_shared_preload_libraries_in_progress)
+ ereport(FATAL, (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("cannot register custom signal after startup")));
+
+ /* iterate through custom signal keys to find free slot */
+ for (reason = PROCSIG_CUSTOM_1; reason <= PROCSIG_CUSTOM_N; reason++)
+ if (!CustomInterruptHandlers[reason - PROCSIG_CUSTOM_1])
+ {
+ CustomInterruptHandlers[reason - PROCSIG_CUSTOM_1] = handler;
+ return reason;
+ }
+ return INVALID_PROCSIGNAL;
+}
+
+/*
+ * UnregisterCustomProcSignal
+ * Release slot of specific custom signal.
+ *
+ * This function have to be called in _PG_init or _PG_fini functions of
+ * extensions at the stage of loading shared preloaded libraries. Otherwise it
+ * throws fatal error. Also it throws fatal error if argument is not valid
+ * custom signal.
+ */
+void
+UnregisterCustomProcSignal(ProcSignalReason reason)
+{
+ if (!process_shared_preload_libraries_in_progress)
+ ereport(FATAL, (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("cannot unregister custom signal after startup")));
+
+ if (!IsCustomProcSignalReason(reason))
+ ereport(FATAL, (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("try to unregister not custom signal")));
+
+ CustomInterruptHandlers[reason - PROCSIG_CUSTOM_1] = NULL;
+}
+
+/*
* SendProcSignal
* Send a signal to a Postgres process
*
@@ -292,9 +352,66 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
+ CheckAndSetCustomSignalInterrupts();
+
SetLatch(MyLatch);
latch_sigusr1_handler();
errno = save_errno;
}
+
+/*
+ * Handle receipt of an interrupt indicating any of custom process signals.
+ */
+static void
+CheckAndSetCustomSignalInterrupts()
+{
+ ProcSignalReason reason;
+
+ for (reason = PROCSIG_CUSTOM_1; reason <= PROCSIG_CUSTOM_N; reason++)
+ {
+ if (CheckProcSignal(reason))
+ {
+
+ /* set interrupt flags */
+ InterruptPending = true;
+ CustomSignalPendings[reason - PROCSIG_CUSTOM_1] = true;
+ }
+ }
+
+ SetLatch(MyLatch);
+}
+
+/*
+ * CheckAndHandleCustomSignals
+ * Check custom signal flags and call handler assigned to that signal
+ * if it is not NULL
+ *
+ * This function is called within CHECK_FOR_INTERRUPTS if interrupt have been
+ * occurred.
+ */
+void
+CheckAndHandleCustomSignals(void)
+{
+ int i;
+
+ /* Disable interrupts to avoid recursive calls */
+ HOLD_INTERRUPTS();
+
+ /* Check on expiring of custom signals and call its handlers if exist */
+ for (i = 0; i < NUM_CUSTOM_PROCSIGNALS; i++)
+ if (CustomSignalPendings[i])
+ {
+ ProcSignalHandler_type handler;
+ ProcSignalReason reason;
+
+ CustomSignalPendings[i] = false;
+ handler = CustomInterruptHandlers[i];
+ reason = PROCSIG_CUSTOM_1 + i;
+ if (handler)
+ handler(reason);
+ }
+
+ RESUME_INTERRUPTS();
+}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index ddc3ec8..8cba73d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3051,6 +3051,8 @@ ProcessInterrupts(void)
if (ParallelMessagePending)
HandleParallelMessages();
+
+ CheckAndHandleCustomSignals();
}
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 6db0d69..2d35fce 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -17,6 +17,8 @@
#include "storage/backendid.h"
+#define NUM_CUSTOM_PROCSIGNALS 64
+
/*
* Reasons for signalling a Postgres child process (a backend or an auxiliary
* process, like checkpointer). We can cope with concurrent signals for different
@@ -29,6 +31,8 @@
*/
typedef enum
{
+ INVALID_PROCSIGNAL = -1, /* Must be first */
+
PROCSIG_CATCHUP_INTERRUPT, /* sinval catchup interrupt */
PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
PROCSIG_PARALLEL_MESSAGE, /* message from cooperating parallel backend */
@@ -42,9 +46,20 @@ typedef enum
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
+ PROCSIG_CUSTOM_1,
+ /*
+ * PROCSIG_CUSTOM_2,
+ * ...,
+ * PROCSIG_CUSTOM_N-1,
+ */
+ PROCSIG_CUSTOM_N = PROCSIG_CUSTOM_1 + NUM_CUSTOM_PROCSIGNALS - 1,
+
NUM_PROCSIGNALS /* Must be last! */
} ProcSignalReason;
+/* Handler of custom process signal */
+typedef void (*ProcSignalHandler_type) (ProcSignalReason reason);
+
/*
* prototypes for functions in procsignal.c
*/
@@ -52,9 +67,14 @@ extern Size ProcSignalShmemSize(void);
extern void ProcSignalShmemInit(void);
extern void ProcSignalInit(int pss_idx);
+extern ProcSignalReason RegisterCustomProcSignalHandler(ProcSignalHandler_type handler);
+extern void UnregisterCustomProcSignal(ProcSignalReason reason);
+extern ProcSignalHandler_type GetCustomProcSignalHandler(ProcSignalReason reason);
extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
BackendId backendId);
+extern void CheckAndHandleCustomSignals(void);
+
extern void procsignal_sigusr1_handler(SIGNAL_ARGS);
#endif /* PROCSIGNAL_H */
Hello!
On 11.01.2018 18:53, Arthur Zakirov wrote:
The relationship between custom signals and
assigned handlers (function addresses) is replicated from postmaster to
child processes including working backends.I think this happens only if a custom signal registered during initializing shared_preload_libraries.
But from RegisterCustomProcSignalHandler()'s implementation I see that you can register the signal anytime. Am I wrong?If so then custom signal handlers may not work as expected.
Yeah, thanks. Added checks on
process_shared_preload_libraries_in_progress flag.
What is purpose of AssignCustomProcSignalHandler() function? This function can erase a handler set by another extension.
For example, extension 1 set handler passing reason PROCSIG_CUSTOM_1, and extension 2 set another handler passing reason PROCSIG_CUSTOM_1 too. Here the handler of the extension 2 will be purged.+ + Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N); +I think it is not good to use asserts within AssignCustomProcSignalHandler() and GetCustomProcSignalHandler(). Because this functions can be executed by an external extension, and it may pass a reason outside this boundary. It will be better if the reason will be checked in runtime.
I was guided by the consideration that assertions define preconditions
for input parameter (in our case, procsignal) of functions. Meeting
these preconditions have to be provided by function callers. Since these
checks is not expensive it will be good to replace their to ereport calls.
Updated patch is attached in [1].
1.
/messages/by-id/37a48ac6-aa14-4e36-5f08-cf8581fe1382@gmail.com
--
Regards,
Maksim Milyutin
Hello,
On Mon, Jan 22, 2018 at 02:34:58PM +0300, Maksim Milyutin wrote:
...
I have attached a new version of patch and updated version of
remote_effective_user function implementation that demonstrates the usage of
custom signals API.
Thank you.
The patch is applied and build.
+/* + * UnregisterCustomProcSignal + * Release slot of specific custom signal. + * + * This function have to be called in _PG_init or _PG_fini functions of + * extensions at the stage of loading shared preloaded libraries. Otherwise it + * throws fatal error. Also it throws fatal error if argument is not valid + * custom signal. + */ +void +UnregisterCustomProcSignal(ProcSignalReason reason) +{ + if (!process_shared_preload_libraries_in_progress) + ereport(FATAL, (errcode(ERRCODE_INTERNAL_ERROR), + errmsg("cannot unregister custom signal after startup")));
Is there actual need in UnregisterCustomProcSignal() within _PG_init()?
An extension registers a handler and then unregister it doing
nothing. It seems useless.
Also process_shared_preload_libraries_in_progress within _PG_fini() will
be false I think. _PG_fini() won't be called though, because
implementation of internal_unload_library() is disabled.
Actually, is there need in UnregisterCustomProcSignal() at all? It
unregisters a handler only in current backend, for actual unergistering
we need to call it everywhere, if I'm not mistaken.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Hello Maksim,
On 1/27/18 2:19 PM, Arthur Zakirov wrote:
On Mon, Jan 22, 2018 at 02:34:58PM +0300, Maksim Milyutin wrote:
The patch is applied and build.
+/* + * UnregisterCustomProcSignal + * Release slot of specific custom signal. + * + * This function have to be called in _PG_init or _PG_fini functions of + * extensions at the stage of loading shared preloaded libraries. Otherwise it + * throws fatal error. Also it throws fatal error if argument is not valid + * custom signal. + */ +void +UnregisterCustomProcSignal(ProcSignalReason reason) +{ + if (!process_shared_preload_libraries_in_progress) + ereport(FATAL, (errcode(ERRCODE_INTERNAL_ERROR), + errmsg("cannot unregister custom signal after startup")));Is there actual need in UnregisterCustomProcSignal() within _PG_init()?
An extension registers a handler and then unregister it doing
nothing. It seems useless.Also process_shared_preload_libraries_in_progress within _PG_fini() will
be false I think. _PG_fini() won't be called though, because
implementation of internal_unload_library() is disabled.Actually, is there need in UnregisterCustomProcSignal() at all? It
unregisters a handler only in current backend, for actual unergistering
we need to call it everywhere, if I'm not mistaken.
This patch has been in Waiting on Author state for almost three weeks.
Have you had a chance to consider Arthur's suggestions?
Do you know when you'll have an updated patch available?
Thanks,
--
-David
david@pgmasters.net
Hello David,
On 05.03.2018 18:50, David Steele wrote:
Hello Maksim,
On 1/27/18 2:19 PM, Arthur Zakirov wrote:
Is there actual need in UnregisterCustomProcSignal() within _PG_init()?
An extension registers a handler and then unregister it doing
nothing. It seems useless.Also process_shared_preload_libraries_in_progress within _PG_fini() will
be false I think. _PG_fini() won't be called though, because
implementation of internal_unload_library() is disabled.Actually, is there need in UnregisterCustomProcSignal() at all? It
unregisters a handler only in current backend, for actual unergistering
we need to call it everywhere, if I'm not mistaken.This patch has been in Waiting on Author state for almost three weeks.
Have you had a chance to consider Arthur's suggestions?
Yes, I want to rework my patch to enable setup of custom signals on
working backend without preload initialization.
Do you know when you'll have an updated patch available?
I want to actuate the work on this patch for the next 12 release. Sorry,
for now I can not keep up with the current release.
--
Regards,
Maksim Milyutin
Hi Maksim,
On 3/5/18 11:24 AM, Maksim Milyutin wrote:
Hello David,
On 05.03.2018 18:50, David Steele wrote:
Hello Maksim,
On 1/27/18 2:19 PM, Arthur Zakirov wrote:
Is there actual need in UnregisterCustomProcSignal() within _PG_init()?
An extension registers a handler and then unregister it doing
nothing. It seems useless.Also process_shared_preload_libraries_in_progress within _PG_fini() will
be false I think. _PG_fini() won't be called though, because
implementation of internal_unload_library() is disabled.Actually, is there need in UnregisterCustomProcSignal() at all? It
unregisters a handler only in current backend, for actual unergistering
we need to call it everywhere, if I'm not mistaken.This patch has been in Waiting on Author state for almost three weeks.
Have you had a chance to consider Arthur's suggestions?Yes, I want to rework my patch to enable setup of custom signals on
working backend without preload initialization.Do you know when you'll have an updated patch available?
I want to actuate the work on this patch for the next 12 release. Sorry,
for now I can not keep up with the current release.
Understood. I'll mark it Returned with Feedback and you can enter it in
a CF when you have a new patch.
Regards,
--
-David
david@pgmasters.net
Hello Maksim,
reading about
https://www.postgresql-archive.org/Allow-auto-explain-to-log-plans-before-queries-are-executed-td6124646.html
makes me think (again) about your work and pg_query_state ...
Is there a chance to see you restarting working on this patch ?
Regards
PAscal
--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Is there a chance to see you restarting working on this patch ?
I noticed that despite interest to the matter, the discussion in this CF
thread stopped quite a while ago. So I'd like to push here a patch revised
according to the previous discussion. Actually the patch is being used as a
part of the pg_query_state extension during several major PostgreSQL
releases. So I'd like to raise the matter of reviewing this updated patch
and include it into version 14 as I see much use of this change.
I welcome all your considerations,
--
Best regards,
Pavel Borisov
Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>
Attachments:
v3-0001-Custom-signal-handler-for-extensions.patchapplication/octet-stream; name=v3-0001-Custom-signal-handler-for-extensions.patchDownload
From 87fd8c92ee5584e8a0703f03d052a6161f4b746a Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Thu, 3 Sep 2020 15:06:52 +0400
Subject: [PATCH v3] Custom signal handler for extensions
The patch allows to define custom signals and their handlers from extension
side. A fixed set of signals can be reserved on postgres initialization
stage (in _PG_init function of shared preloaded module) through specific
interface functions. The relationship between custom signals and assigned
handlers is replicated from postmaster to child processes, including working
backends. The handler on a remote backend called by this signals (via
SendProcSignal function) comes into action at CHECK_FOR_INTERRUPTS point.
This feature is used by pg_query_state extension
https://github.com/postgrespro/pg_query_state and this API can also be usedful
for profiling extensions etc.
Authors: Maksim Milyutin, Alexey Kondratov
Discussion: https://postgrespro.ru/list/thread-id/2362085#590e3bc2-0eb3-322c-3c1f-4e79ff562bfe@gmail.com
---
src/backend/storage/ipc/procsignal.c | 93 ++++++++++++++++++++++++++++
src/backend/tcop/postgres.c | 2 +
src/include/storage/procsignal.h | 20 ++++++
3 files changed, 115 insertions(+)
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 4fa385b0ec..3e8328d341 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -83,6 +83,12 @@ typedef struct
*/
#define NumProcSignalSlots (MaxBackends + NUM_AUXPROCTYPES)
+#define IsCustomProcSignalReason(reason) \
+ ((reason) >= PROCSIG_CUSTOM_1 && (reason) <= PROCSIG_CUSTOM_N)
+
+static bool CustomSignalPendings[NUM_CUSTOM_PROCSIGNALS];
+static ProcSignalHandler_type CustomInterruptHandlers[NUM_CUSTOM_PROCSIGNALS];
+
/* Check whether the relevant type bit is set in the flags. */
#define BARRIER_SHOULD_CHECK(flags, type) \
(((flags) & (((uint32) 1) << (uint32) (type))) != 0)
@@ -92,6 +98,9 @@ static volatile ProcSignalSlot *MyProcSignalSlot = NULL;
static bool CheckProcSignal(ProcSignalReason reason);
static void CleanupProcSignalState(int status, Datum arg);
+
+static void CheckAndSetCustomSignalInterrupts(void);
+
static void ProcessBarrierPlaceholder(void);
/*
@@ -235,6 +244,36 @@ CleanupProcSignalState(int status, Datum arg)
slot->pss_pid = 0;
}
+/*
+ * RegisterCustomProcSignalHandler
+ * Assign specific handler of custom process signal with new
+ * ProcSignalReason key.
+ *
+ * This function has to be called in _PG_init function of extensions at the
+ * stage of loading shared preloaded libraries. Otherwise it throws fatal error.
+ *
+ * Return INVALID_PROCSIGNAL if all slots for custom signals are occupied.
+ */
+ProcSignalReason
+RegisterCustomProcSignalHandler(ProcSignalHandler_type handler)
+{
+ ProcSignalReason reason;
+
+ if (!process_shared_preload_libraries_in_progress)
+ ereport(FATAL, (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("cannot register custom signal after startup")));
+
+ /* Iterate through custom signal slots to find a free one */
+ for (reason = PROCSIG_CUSTOM_1; reason <= PROCSIG_CUSTOM_N; reason++)
+ if (!CustomInterruptHandlers[reason - PROCSIG_CUSTOM_1])
+ {
+ CustomInterruptHandlers[reason - PROCSIG_CUSTOM_1] = handler;
+ return reason;
+ }
+
+ return INVALID_PROCSIGNAL;
+}
+
/*
* SendProcSignal
* Send a signal to a Postgres process
@@ -585,9 +624,63 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
+ CheckAndSetCustomSignalInterrupts();
+
SetLatch(MyLatch);
latch_sigusr1_handler();
errno = save_errno;
}
+
+/*
+ * Handle receipt of an interrupt indicating any of custom process signals.
+ */
+static void
+CheckAndSetCustomSignalInterrupts()
+{
+ ProcSignalReason reason;
+
+ for (reason = PROCSIG_CUSTOM_1; reason <= PROCSIG_CUSTOM_N; reason++)
+ {
+ if (CheckProcSignal(reason))
+ {
+
+ /* set interrupt flags */
+ InterruptPending = true;
+ CustomSignalPendings[reason - PROCSIG_CUSTOM_1] = true;
+ }
+ }
+
+ SetLatch(MyLatch);
+}
+
+/*
+ * CheckAndHandleCustomSignals
+ * Check custom signal flags and call handler assigned to that signal
+ * if it is not NULL
+ *
+ * This function is called within CHECK_FOR_INTERRUPTS if interrupt occurred.
+ */
+void
+CheckAndHandleCustomSignals(void)
+{
+ int i;
+
+ /* Disable interrupts to avoid recursive calls */
+ HOLD_INTERRUPTS();
+
+ /* Check on expiring of custom signals and call its handlers if exist */
+ for (i = 0; i < NUM_CUSTOM_PROCSIGNALS; i++)
+ if (CustomSignalPendings[i])
+ {
+ ProcSignalHandler_type handler;
+
+ CustomSignalPendings[i] = false;
+ handler = CustomInterruptHandlers[i];
+ if (handler != NULL)
+ handler();
+ }
+
+ RESUME_INTERRUPTS();
+}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index c9424f167c..ea330c9045 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3209,6 +3209,8 @@ ProcessInterrupts(void)
if (ParallelMessagePending)
HandleParallelMessages();
+
+ CheckAndHandleCustomSignals();
}
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 5cb39697f3..14cddb99de 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -16,6 +16,7 @@
#include "storage/backendid.h"
+#define NUM_CUSTOM_PROCSIGNALS 64
/*
* Reasons for signaling a Postgres child process (a backend or an auxiliary
@@ -29,6 +30,8 @@
*/
typedef enum
{
+ INVALID_PROCSIGNAL = -1, /* Must be first */
+
PROCSIG_CATCHUP_INTERRUPT, /* sinval catchup interrupt */
PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
PROCSIG_PARALLEL_MESSAGE, /* message from cooperating parallel backend */
@@ -43,9 +46,20 @@ typedef enum
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
+ PROCSIG_CUSTOM_1,
+ /*
+ * PROCSIG_CUSTOM_2,
+ * ...,
+ * PROCSIG_CUSTOM_N-1,
+ */
+ PROCSIG_CUSTOM_N = PROCSIG_CUSTOM_1 + NUM_CUSTOM_PROCSIGNALS - 1,
+
NUM_PROCSIGNALS /* Must be last! */
} ProcSignalReason;
+/* Handler of custom process signal */
+typedef void (*ProcSignalHandler_type) (void);
+
typedef enum
{
/*
@@ -63,6 +77,10 @@ extern Size ProcSignalShmemSize(void);
extern void ProcSignalShmemInit(void);
extern void ProcSignalInit(int pss_idx);
+
+extern ProcSignalReason
+ RegisterCustomProcSignalHandler(ProcSignalHandler_type handler);
+
extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
BackendId backendId);
@@ -70,6 +88,8 @@ extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type);
extern void WaitForProcSignalBarrier(uint64 generation);
extern void ProcessProcSignalBarrier(void);
+extern void CheckAndHandleCustomSignals(void);
+
extern void procsignal_sigusr1_handler(SIGNAL_ARGS);
#endif /* PROCSIGNAL_H */
--
2.28.0
On 03.09.2020 14:25, Pavel Borisov wrote:
Is there a chance to see you restarting working on this patch ?
I noticed that despite interest to the matter, the discussion in this
CF thread stopped quite a while ago. So I'd like to push here a patch
revised according to the previous discussion. Actually the patch is
being used as a part of the pg_query_state extension during several
major PostgreSQL releases. So I'd like to raise the matter of
reviewing this updated patch and include it into version 14 as I see
much use of this change.I welcome all your considerations,
I took a look at the patch. It looks fine and I see, that it contains
fixes for the latest questions in the thread.
I think we should provide a test module for this feature, that will
serve as both test and example of the use.
This is a feature for extension developers, so I don't know where we
should document it. At the very least we can improve comments. For
example, describe the fact that custom signals are handled after
receiving SIGUSR1.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Wed, Nov 25, 2020 at 06:34:48PM +0300, Anastasia Lubennikova wrote:
I took a look at the patch. It looks fine and I see, that it contains fixes
for the latest questions in the thread.I think we should provide a test module for this feature, that will serve as
both test and example of the use.
Yep. That's missing. The trick here is usually to be able to come up
with something minimalist still useful enough for two reasons:
- This can be used as a template.
- This makes sure that the API work.
As the patch stands, nothing in this architecture is tested.
This is a feature for extension developers, so I don't know where we should
document it. At the very least we can improve comments. For example,
describe the fact that custom signals are handled after receiving SIGUSR1.
As you say, this is for extension developers, so this should be
documented in the headers defining those APIs. FWIW, I am -1 with the
patch as proposed. First, it has zero documentation. Second, it uses
a hardcoded custom number of signals of 64 instead of a flexible
design. In most cases, 64 is a waste. In some cases, 64 may not be
enough. IMO, a design based on the registration of a custom
procsignal done at shmem init time would be much more flexible. You
need one registration API and something to get an ID associated to the
custom entry, and that's roughly what Teodor tells upthread.
This needs more work, so I am marking it as returned with feedback.
--
Michael
On 1/12/18 20:51, Teodor Sigaev wrote:
In perspective, this mechanism provides the low-level instrument to
define remote procedure call on extension side. The simple RPC that
defines effective userid on remote backend (remote_effective_user
function) is attached for example.7) Suppose, API allows to use different handlers in different processes
for the same reason, it's could be reason of confusion. I suggest to
restrict Register/Unregister call only for shared_preload_library, ie
only during startup.
+1
8) I'd like to see an example of usage this API somewhere in contrib in
exsting modules. Ideas?
I imagine, auto_explain could demonstrate usage of the API by
implementing logging of current query state, triggered by a signal. Most
of necessary code is already existed there.
Of course, this option will be enabled if auto_explain loads on startup.
But, maybe it breaks main concept of the auto_explain extension?
--
Regards
Andrey Lepikhov
Postgres Professional