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
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 */
#include "postgres.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "portability/instr_time.h"
#include "port/atomics.h"
#include "storage/ipc.h"
#include "storage/latch.h"
#include "storage/procsignal.h"
#include "storage/s_lock.h"
#include "storage/shmem.h"
#include "storage/spin.h"
#include "utils/elog.h"
#ifdef PG_MODULE_MAGIC
PG_MODULE_MAGIC;
#endif
typedef struct {
slock_t mutex;
Oid userid;
bool isSet;
Latch *callerLatch;
} UserIdSlot;
const Size UserIdSlotSize = BUFFERALIGN(sizeof(UserIdSlot));
static ProcSignalReason extractEffectiveUserReason;
static UserIdSlot *userIdSlot = NULL;
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static void foo_shmem_startup();
static void sendEffectiveUserId(ProcSignalReason reason);
static Oid extractEffectiveUserId(pid_t remoteSessionId);
void
_PG_init()
{
elog(LOG, "Load. My PID = %d", MyProcPid);
extractEffectiveUserReason =
RegisterCustomProcSignalHandler(sendEffectiveUserId);
if (extractEffectiveUserReason == INVALID_PROCSIGNAL)
{
elog(WARNING, "Insufficient custom ProcSignal slots");
return;
}
RequestAddinShmemSpace(UserIdSlotSize);
prev_shmem_startup_hook = shmem_startup_hook;
shmem_startup_hook = foo_shmem_startup;
}
void
foo_shmem_startup()
{
bool found;
elog(LOG, "Stand out shmem. My PID = %d", MyProcPid);
userIdSlot = ShmemInitStruct("foo userid slot", UserIdSlotSize, &found);
if (prev_shmem_startup_hook)
prev_shmem_startup_hook();
}
void
_PG_fini()
{
elog(LOG, "Unload. My PID = %d", MyProcPid);
UnregisterCustomProcSignal(extractEffectiveUserReason);
shmem_startup_hook = prev_shmem_startup_hook;
}
PG_FUNCTION_INFO_V1(remote_effective_user);
Datum
remote_effective_user(PG_FUNCTION_ARGS)
{
pid_t pid = PG_GETARG_INT32(0);
PG_RETURN_INT32(extractEffectiveUserId(pid));
}
Oid
extractEffectiveUserId(pid_t remoteSessionId)
{
Oid result;
int sendSignalStatus;
long timeout = 5000;
int rc = 0;
userIdSlot->isSet = false;
userIdSlot->callerLatch = MyLatch;
pg_write_barrier();
sendSignalStatus = SendProcSignal(
remoteSessionId, extractEffectiveUserReason, InvalidBackendId);
if (sendSignalStatus == -1)
{
switch (errno)
{
case ESRCH:
elog(WARNING, "Process not found");
break;
default:
elog(WARNING, "Error with sending signal");
}
return InvalidOid;
}
for (;;)
{
bool isSet = false;
instr_time start_time;
instr_time end_time;
SpinLockAcquire(&userIdSlot->mutex);
result = userIdSlot->userid;
isSet = userIdSlot->isSet;
SpinLockRelease(&userIdSlot->mutex);
if (isSet)
break;
if (rc & WL_TIMEOUT || timeout <= 0)
{
elog(WARNING, "Remote session is not retry");
return InvalidOid;
}
INSTR_TIME_SET_CURRENT(start_time);
rc = WaitLatch(MyLatch, WL_LATCH_SET | WL_TIMEOUT, timeout,
PG_WAIT_EXTENSION);
INSTR_TIME_SET_CURRENT(end_time);
INSTR_TIME_SUBTRACT(end_time, start_time);
timeout -= (long) INSTR_TIME_GET_MILLISEC(end_time);
CHECK_FOR_INTERRUPTS();
ResetLatch(MyLatch);
}
return result;
}
void
sendEffectiveUserId(ProcSignalReason reason)
{
bool fakeFlag;
AssertArg(reason == extractEffectiveUserReason);
elog(LOG, "Extract effective user. My PID = %d", MyProcPid);
SpinLockAcquire(&userIdSlot->mutex);
GetUserIdAndContext(&userIdSlot->userid, &fakeFlag);
userIdSlot->isSet = true;
SpinLockRelease(&userIdSlot->mutex);
SetLatch(userIdSlot->callerLatch);
}
