PoC: custom signal handler for extensions

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

PoC: custom signal handler for extensions

Maksim Milyutin-2
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


custom_signals.patch (10K) Download Attachment
remote_effective_user.c (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

legrand legrand
+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

Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

Maksim Milyutin-2
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


Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

a.zakirov
In reply to this post by Maksim Milyutin-2
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

Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

Teodor Sigaev
In reply to this post by Maksim Milyutin-2
> 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: [hidden email]
                                                    WWW: http://www.sigaev.ru/

Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

legrand legrand
Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

Maksim Milyutin-2
In reply to this post by Teodor Sigaev

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

custom_signals_v2.patch (6K) Download Attachment
remote_effective_user.c (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

Maksim Milyutin-2
In reply to this post by a.zakirov

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. https://www.postgresql.org/message-id/37a48ac6-aa14-4e36-5f08-cf8581fe1382%40gmail.com

-- 
Regards,
Maksim Milyutin
Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

a.zakirov
In reply to this post by Maksim Milyutin-2
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

Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

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

David Steele
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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

Maksim Milyutin-2
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


Reply | Threaded
Open this post in threaded view
|

Re: PoC: custom signal handler for extensions

David Steele
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
[hidden email]

Previous Thread Next Thread