Re: On login trigger: take three

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>
Cc: Greg Nancarrow <gregn4422(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: On login trigger: take three
Date: 2020-12-10 18:09:07
Message-ID: CAFj8pRAMT45SaGK=YZar51tZgw4BH4T=z5SonrRs0h9dYz5Xjw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

čt 10. 12. 2020 v 16:48 odesílatel Konstantin Knizhnik <
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.

>
> 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.

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

I checked profiles, and although there is significant slowdown when there
is one login trigger, it was not related to plpgsql.

There is big overhead of

8,98% postmaster postgres [.] guc_name_compare

Maybe EventTriggerInvoke is not well optimized for very frequent execution.
Overhead of plpgsql is less than 0.1%, although there is significant
slowdown (when a very simple logon trigger is executed).

Regards

Pavel

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2020-12-10 18:39:49 Re: Allow CURRENT_ROLE in GRANTED BY
Previous Message Robert Haas 2020-12-10 17:32:52 pg_basebackup test coverage