Re: On login trigger: take three

Hi, Daniel!

On Mon, Sep 25, 2023 at 3:42 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 25 Sep 2023, at 11:13, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> > I'd like to do a short summary of
> > design issues on this thread.
>
> Thanks for summarizing this long thread!
>
> > the patch for the GUC option to disable
> > all event triggers resides in a separate thread [4]. Apparently that
> > patch should be committed first [5].
>
> I have committed the prerequisite patch for temporarily disabling EVTs via a
> GUC in 7750fefdb2.  We should absolutely avoid creating any more dependencies
> on single-user mode (yes, I have changed my mind since the beginning of the
> thread).

Thank you for committing 7750fefdb2.  I've revised this patch
according to it.  I've resolved the conflicts, make use of
event_triggers GUC and adjusted some comments.

> > 3. Yet another question is connection-time overhead introduced by this
> > patch. The overhead estimate varies from no measurable overhead [6] to
> > 5% overhead [7]. In order to overcome that, [8] has introduced a
> > database-level flag indicating whether there are connection triggers.
> > Later this flag was removed [9] in a hope that the possible overhead
> > is acceptable.
>
> While I disliked the flag, I do think the overhead is problematic.  Last time I
> profiled it I found it noticeable, and it seems expensive for such a niche
> feature to impact such a hot path.  Maybe you can think of other ways to reduce
> the cost here (if it indeed is an issue in the latest version of the patch,
> which is not one I've benchmarked)?

I don't think I can reproduce the performance regression pointed out
by Pavel Stehule [1].

I run a simple ";" sql script (this script doesn't even get the
snapshot) on my laptop and run it multiple times with event_triggers =
on and event_triggers = off;

pgbench -c 10 -j 10 -M prepared -f 1.sql -P 1 -T 60 -C postgres
event_triggers = on
run1: 2261
run2: 2301
run3: 2281
event_triggers = off
run1: 2321
run2: 2277
run3: 2267

pgbench -c 10 -j 10 -M prepared -f 1.sql -P 1 -T 60 -C postgres
event_triggers = on
run1: 731
run2: 740
run3: 733
event_triggers = off
run1: 739
run2: 734
run3: 731

I can't confirm the measurable overhead.

> > 5. It was also pointed out [11] that ^C in psql doesn't cancel
> > long-running client connection triggers. That might be considered a
> > psql problem though.
>
> While it is a psql problem, it's exacerbated by a slow login EVT and it breaks
> what I would guess is the mental model of many who press ^C in a stalled login.
> At the very least we should probably document the risk?

Done in the attached patch.

Links
1. https://www.postgresql.org/message-id/CAFj8pRChwu01VLx76nKBVyScHCsd1YnBGiKfDJ6h17g4CSnUBg%40mail.gmail.com

------
Regards,
Alexander Korotkov