On 17.12.2020 9:31, Pavel Stehule wrote:
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
