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.
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.
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().
> 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
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
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