Re: On login trigger: take three

On 10.12.2020 21:09, Pavel Stehule wrote:
>
>
> čt 10. 12. 2020 v 16:48 odesílatel Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru <mailto:k(dot)knizhnik(at)postgrespro(dot)ru>> napsal:
>
>
>
> On 10.12.2020 18:12, Pavel Stehule wrote:
>>
>> 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.
>>
>
>
> Do you mean some variable in shared memory or GUCs?
> It was my first idea - to use some flag in shared memory to make
> it possible fast check that there are not event triggers.
> But this flag should be sent per database. May be I missed
> something, but there is no any per-database shared memory data
> structure in Postgres.
> Certainly it is possible to organize some hash db->event_info, but
> it makes this patch several times more complex.
>
>
> My idea was a little bit different - just inside process
> initialization, checking existence of event triggers, and later when
> it is necessary to start a transaction for trigger execution. This
> should to reduce useless empty transaction,
>
> I am sure this is a problem on computers with slower CPU s, although I
> have I7, but because this feature is usually unused, then the
> performance impact should be minimal every time.
>
Sorry, may be I missed something. But now I completely confused with
your idea.
Right now processing of login hooks is done in PostgresMain.
In the previous mail you have suggested to did it in InitPostgres which
is invoked from PostgresMain.
So what is the difference (except there open transaction in InitPostgres)?

So what the problem we are trying to solve now?
As far as I understand, your concern is about extra overhead during
backend startup needed to check if there on-login triggers defined.
We are not speaking about trigger execution. We mostly worry about
applications which are not using triggers at all. But still have to pay
some small extra overhead at startup.
This overhead seems to be negligible (1% for dummy connection doing just
"select 1"). But taken in account that 99.9999% applications will not
use connection triggers,
even very small overhead seems to be not good.

But what can we do?
We do not want to scan pg_event_trigger table on backend startup. But
how else we can skip this check, taken in account that
1. Such trigger can be registered at any moment of time
2. Triggers are registered per database, so we can not have just global
flag,signaling lack of event triggers.

The only solution I see at this moment is <db,has_event_triggers> hash
in shared memory.
But it seems to be overkill from my point of view.

This is why I suggested to have disable_login_event_triggers GUC set to
true by default.

>
> From my point of view it is better to have separate GUC disabling
> just client connection events and switch it on by default.
> So only those who need this events with switch it on, other users
> will not pay any extra cost for it.
>
>
> It can work, but this design is not user friendly.  The significant
> bottleneck should be forking new processes, and check of content some
> usually very small tables should be invisible. So if there is a
> possible way to implement some feature that can be enabled by default,
> then we should go this way. It can be pretty unfriendly if somebody
> writes a logon trigger and it will not work by default without any
> warning.
Please notice that event triggers are disabled by default.
You need to call "alter event trigger xxx enable always".
May be instead of GUCs we should somehow use ALTER mechanism?
But I do not understand why it is better and how it will help to solve
this problem (elimination of exrta overhead when there are not triggers).

>
> When I tested last patch I found a problem (I have disabled assertions
> due performance testing)
>
> create role xx login;
> alter system set disable_event_triggers to on; -- better should be
> positive form - enable_event_triggers to on, off
> select pg_reload_conf();
>
> psql -U xx
>
> Thread 2.1 "postmaster" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7f2bff438ec0 (LWP 827497)]
> ResourceOwnerEnlargeCatCacheRefs (owner=0x0) at resowner.c:1025
> 1025 ResourceArrayEnlarge(&(owner->catrefarr));
> (gdb) bt
> #0  ResourceOwnerEnlargeCatCacheRefs (owner=0x0) at resowner.c:1025
> #1  0x00000000008a70f8 in SearchCatCacheInternal (cache=<optimized
> out>, nkeys=nkeys(at)entry=1, v1=v1(at)entry=13836, v2=v2(at)entry=0,
>     v3=v3(at)entry=0, v4=v4(at)entry=0) at catcache.c:1273
> #2  0x00000000008a7575 in SearchCatCache1 (cache=<optimized out>,
> v1=v1(at)entry=13836) at catcache.c:1167
> #3  0x00000000008b7f80 in SearchSysCache1 (cacheId=cacheId(at)entry=21,
> key1=key1(at)entry=13836) at syscache.c:1122
> #4  0x00000000005939cd in pg_database_ownercheck (db_oid=13836,
> roleid=16387) at aclchk.c:5114
> #5  0x0000000000605b42 in EventTriggersDisabled () at event_trigger.c:650
> #6  EventTriggerOnConnect () at event_trigger.c:839
> #7  0x00000000007b46d7 in PostgresMain (argc=argc(at)entry=1,
> argv=argv(at)entry=0x7ffdd6256080, dbname=<optimized out>,
>     username=0xf82508 "tom") at postgres.c:4021
> #8  0x0000000000741afd in BackendRun (port=<optimized out>,
> port=<optimized out>) at postmaster.c:4490
> #9  BackendStartup (port=<optimized out>) at postmaster.c:4212
> #10 ServerLoop () at postmaster.c:1729
> #11 0x0000000000742881 in PostmasterMain (argc=argc(at)entry=3,
> argv=argv(at)entry=0xf44d00) at postmaster.c:1401
> #12 0x00000000004f1ff7 in main (argc=3, argv=0xf44d00) at main.c:209
>

pg_database_ownercheck can not be called outside transaction.
I can split replace call of EventTriggersDisabled in
EventTriggerOnConnect with two separate checks:
disable_event_triggers - before start of transaction
and pg_database_ownercheck(MyDatabaseId, GetUserId()) inside transaction.

But I wonder if we need to check database ownership in this place at all?
May be just allow to alter disable_event_triggers for superusers?

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