Re: [HACKERS] PoC: custom signal handler for extensions

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