Re: On login trigger: take three

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

>
>
> On 10.12.2020 10:45, Pavel Stehule wrote:
>
>
>
> st 9. 12. 2020 v 14:28 odesílatel Konstantin Knizhnik <
> k(dot)knizhnik(at)postgrespro(dot)ru> napsal:
>
>>
>>
>> On 09.12.2020 15:34, Pavel Stehule wrote:
>>
>> Hi
>>
>> st 9. 12. 2020 v 13:17 odesílatel Greg Nancarrow <gregn4422(at)gmail(dot)com>
>> napsal:
>>
>>> On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>>> wrote:
>>> >
>>> >
>>> > There are two maybe generic questions?
>>> >
>>> > 1. Maybe we can introduce more generic GUC for all event triggers like
>>> disable_event_triggers? This GUC can be checked only by the database owner
>>> or super user. It can be an alternative ALTER TABLE DISABLE TRIGGER ALL. It
>>> can be protection against necessity to restart to single mode to repair the
>>> event trigger. I think so more generic solution is better than special
>>> disable_client_connection_trigger GUC.
>>> >
>>> > 2. I have no objection against client_connection. It is probably
>>> better for the mentioned purpose - possibility to block connection to
>>> database. Can be interesting, and I am not sure how much work it is to
>>> introduce the second event - session_start. This event should be started
>>> after connecting - so the exception there doesn't block connect, and should
>>> be started also after the new statement "DISCARD SESSION", that will be
>>> started automatically after DISCARD ALL. This feature should not be
>>> implemented in first step, but it can be a plan for support pooled
>>> connections
>>> >
>>>
>>> I've created a separate patch to address question (1), rather than
>>> include it in the main patch, which I've adjusted accordingly. I'll
>>> leave question (2) until another time, as you suggest.
>>> See the attached patches.
>>>
>>
>> I see two possible questions?
>>
>> 1. when you introduce this event, then the new hook is useless ?
>>
>> 2. what is a performance impact for users that want not to use this
>> feature. What is a overhead of EventTriggerOnConnect and is possible to
>> skip this step if database has not any event trigger
>>
>>
>> As far as I understand this are questions to me rather than to Greg.
>> 1. Do you mean client_connection_hook? It is used to implement this new
>> event type. It can be also used for other purposes.
>>
>
> ok. I don't like it, but there are redundant hooks (against event
> triggers) already.
>
>
> 2. Connection overhead is quite large. Supporting on connect hook requires
>> traversal of event trigger relation. But this overhead is negligible
>> comparing with overhead of establishing connection. In any case I did the
>> following test (with local connection):
>>
>> pgbench -C -S -T 100 -P 1 -M prepared postgres
>>
>> without this patch:
>> tps = 424.287889 (including connections establishing)
>> tps = 952.911068 (excluding connections establishing)
>>
>> with this patch (but without any connection trigger defined):
>> tps = 434.642947 (including connections establishing)
>> tps = 995.525383 (excluding connections establishing)
>>
>> As you can see - there is almost now different (patched version is even
>> faster, but it seems to be just "white noise".
>>
>
> This is not the worst case probably. In this patch the
> StartTransactionCommand is executed on every connection, although it is not
> necessary - and for most use cases it will not be used.
>
> I did more tests - see attachments and I see a 5% slowdown - I don't think
> so it is acceptable for this case. This feature is nice, and for some users
> important, but only really few users can use it.
>
> ┌────────────────┬─────────┬────────────┬─────────────┐
> │ test │ WITH LT │ LT ENABLED │ LT DISABLED │
> ╞════════════════╪═════════╪════════════╪═════════════╡
> │ ro_constant_10 │ 539 │ 877 │ 905 │
> │ ro_index_10 │ 562 │ 808 │ 855 │
> │ ro_constant_50 │ 527 │ 843 │ 863 │
> │ ro_index_50 │ 544 │ 731 │ 742 │
> └────────────────┴─────────┴────────────┴─────────────┘
> (4 rows)
>
> I tested a performance of trigger (results of first column in table):
>
> CREATE OR REPLACE FUNCTION public.foo()
> RETURNS event_trigger
> LANGUAGE plpgsql
> AS $function$
> begin
> if not pg_has_role(session_user, 'postgres', 'member') then
> raise exception 'you are not super user';
> end if;
> end;
> $function$;
>
> There is an available snapshot in InitPostgres, and then there is possible
> to check if for the current database some connect event trigger exists.This
> can reduce an overhead of this patch, when there are no logon triggers.
>
> I think so implemented and used names are ok, but for this feature the
> performance impact should be really very minimal.
>
> There is other small issue - missing tab-complete support for CREATE
> TRIGGER statement in psql
>
> Regards
>
> Pavel
>
>
> Unfortunately I was not able to reproduce your results.
> I just tried the case "select 1" because for this trivial query the
> overhead of session hooks should be the largest.
> In my case variation of results was large enough.
> Did you try to run test multiple times?
>
> pgbench -j 2 -c 10 --connect -f test-ro.sql -T 60 -n postgres
>
> disable_event_triggers = off
> tps = 1812.250463 (including connections establishing)
> tps = 2256.285712 (excluding connections establishing)
>
> tps = 1838.107242 (including connections establishing)
> tps = 2288.507668 (excluding connections establishing)
>
> tps = 1830.879302 (including connections establishing)
> tps = 2279.302553 (excluding connections establishing)
>
>
> disable_event_triggers = on
> tps = 1858.328717 (including connections establishing)
> tps = 2313.661689 (excluding connections establishing)
>
> tps = 1832.932960 (including connections establishing)
> tps = 2282.074346 (excluding connections establishing)
>
> tps = 1868.908521 (including connections establishing)
> tps = 2326.559150 (excluding connections establishing)
>
>
> I tried to increase run time to 1000 seconds.
> Results are the following:
>
> disable_event_triggers = off
> tps = 1813.587876 (including connections establishing)
> tps = 2257.844703 (excluding connections establishing)
>
> disable_event_triggers = on
> tps = 1838.107242 (including connections establishing)
> tps = 2288.507668 (excluding connections establishing)
>
> So the difference on this extreme case is 1%.
>

you have a stronger CPU (2x), and probably you are hitting different
bottleneck or maybe some other is wrong. I can recheck it and I can attach
profiles.

> I tried your suggestion and move client_connection_hook invocation to
> postinit.c
> It slightly improve performance:
>
> tps = 1828.446959 (including connections establishing)
> tps = 2276.142972 (excluding connections establishing)
>
> So result is somewhere in the middle, because it allows to eliminate
> overhead of
> starting transaction or taking snapshots but not of lookup through
> pg_event_trigger table (although it it empty).
>

My idea was a little bit different. Inside postinit initialize some global
variables with info if there are event triggers or not. And later you can
use this variable to start transactions and other things.

There will be two access to pg_event_trigger, but it can eliminate useless
and probably more expensive start_transaction and end_transaction.

Regards

Pavel

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