Re: On login trigger: take three

čt 17. 12. 2020 v 19:30 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> č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
>

for complete review the new field in pg_doc should be documented

Regards

Pavel

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