Re: On login trigger: take three

On 17.12.2020 9:31, Pavel Stehule wrote:
>
>
> st 16. 12. 2020 v 20:38 odesílatel Pavel Stehule 
> <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>> napsal:
>
>     Attached please find new versoin of the patch based on
>     on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch
>
>         So there is still only "disable_client_connection_trigger"
>         GUC? because we need possibility to disable client connect
>         triggers and there is no such need for other event types.
>
>         As you suggested  have added "dathaslogontriggers" flag which
>         is set when client connection trigger is created.
>         This flag is never cleaned (to avoid visibility issues
>         mentioned in my previous mail).
>
>
>     This is much better - I don't see any slowdown when logon trigger
>     is not defined
>
>     I did some benchmarks and looks so starting language handler is
>     relatively expensive - it is about 25% and starting handler like
>     event trigger has about 35%. But this problem can be solved later
>     and elsewhere.
>
>     I prefer the inverse form of disable_connection_trigger. Almost
>     all GUC are in positive form - so enable_connection_triggger is better
>
>     I don't think so current handling dathaslogontriggers is good for
>     production usage. The protection against race condition can be
>     solved by lock on pg_event_trigger
>
>
> I thought about it, and probably the counter of connect triggers will 
> be better there. The implementation will be simpler and more robust.
>
>


I prefer to implement different approach: unset dathaslogontriggers flag 
in event trigger itself when no triggers are returned by 
EventTriggerCommonSetup.
I am using double checking to prevent race condition.
The main reason for this approach is that dropping of triggers is not 
done in event_trigger.c: it is done by generic code for all resources.
It seems to be there is no right place where decrementing of trigger 
counters can be inserted.
Also I prefer to keep all logon-trigger specific code in one file.

Also, as you suggested, I renamed disable_connection_trigger to 
enable_connection_trigger.


New version of the patch is attached.




-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company