Re: On login trigger: take three

On 20.12.2020 10:04, Pavel Stehule wrote:

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

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

On 17.12.2020 9:31, Pavel Stehule wrote:

st 16. 12. 2020 v 20:38 odesílatel Pavel Stehule <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.

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.

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