Re: On login trigger: take three

čt 17. 12. 2020 v 14:04 odesílatel Konstantin Knizhnik <
k(dot)knizhnik(at)postgrespro(dot)ru> napsal:

>
>
> On 17.12.2020 9:31, Pavel Stehule wrote:
>
>
>
> st 16. 12. 2020 v 20:38 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>

yes, it can work

Regards

Pavel

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