Re: On login trigger: take three

Hi

po 21. 12. 2020 v 11:06 odesílatel Konstantin Knizhnik <
k(dot)knizhnik(at)postgrespro(dot)ru> napsal:

>
>
> On 20.12.2020 10:04, Pavel Stehule wrote:
>
>
>
> č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
>
>
> Done.
> Also I fixed some examples in documentation which involves pg_database.
>

regress tests fails

sysviews ... FAILED 112 ms
test event_trigger ... FAILED (test process exited with exit
code 2) 447 ms
test fast_default ... FAILED 392 ms
test stats ... FAILED 626 ms
============== shutting down postmaster ==============

Regards

Pavel

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