Re: On login trigger: take three - Mailing list pgsql-hackers

čt 10. 12. 2020 v 16:48 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.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@entry=1, v1=v1@entry=13836, v2=v2@entry=0,
    v3=v3@entry=0, v4=v4@entry=0) at catcache.c:1273
#2  0x00000000008a7575 in SearchCatCache1 (cache=<optimized out>, v1=v1@entry=13836) at catcache.c:1167
#3  0x00000000008b7f80 in SearchSysCache1 (cacheId=cacheId@entry=21, key1=key1@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@entry=1, argv=argv@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@entry=3, argv=argv@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