Re: On login trigger: take three
Alexander Korotkov <aekorotkov@gmail.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Mikhail Gribkov <youzhick@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Greg Nancarrow <gregn4422@gmail.com>, Ivan Panchenko <wao@mail.ru>,
Teodor Sigaev <teodor@sigaev.ru>, Ibrar Ahmed <ibrar.ahmad@gmail.com>, vignesh C <vignesh21@gmail.com>, Pavel Stehule <pavel.stehule@gmail.com>,
PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>,
Masahiko Sawada <sawada.mshk@gmail.com>
Date: 2023-09-30T08:40:23Z
Lists: pgsql-hackers
Attachments
- 0001-Add-support-of-event-triggers-on-authenticated-l-v42.patch (application/octet-stream) patch v42-0001
On Fri, Sep 29, 2023 at 1:15 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > On 28 Sep 2023, at 23:50, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > I don't think I can reproduce the performance regression pointed out > > by Pavel Stehule [1]. > > > I can't confirm the measurable overhead. > > > Running the same pgbench command on my laptop looking at the average connection > times, and the averaging that over five runs (low/avg/high) I see ~5% increase > over master with the patched version (compiled without assertions and debug): > > Patched event_triggers on: 6.858 ms/7.038 ms/7.434 ms > Patched event_triggers off: 6.601 ms/6.958 ms/7.539 ms > Master: 6.676 ms/6.697 ms/6.760 ms > > This is all quite unscientific with a lot of jitter so grains of salt are to be > applied, but I find it odd that you don't see any measurable effect. Are you > seeing the same/similar connection times between master and with this patch > applied? Thank you for doing experiments on your side. I've rechecked. It appears that I didn't do enough runs, thus I didn't see the overhead as more than an error. Now, I also can confirm ~5% overhead. I spent some time thinking about how to overcome this overhead, but I didn't find a brilliant option. Previously pg_database flag was proposed but then criticized as complex and error-prone. I can also imagine shmem caching mechanism. But it would require overcoming possible race conditions between shared cache invalidation and transaction commit etc. So, that would be also complex and error-prone. Any better ideas? > A few small comments on the patch: > > + prevent successful login to the system. Such bugs may be fixed by > + restarting the system in single-user mode (as event triggers are > This paragraph should be reworded to recommend the GUC instead of single-user > mode (while retaining mention of single-user mode, just not as the primary > option). > > > + Also, it's recommended to evade long-running queries in > s/evade/avoid/ perhaps? Fixed. > Thanks for working on this! Thank you as well! ------ Regards, Alexander Korotkov
Commits
-
Fix some typos in event trigger docs
- 5fce30e77fe1 17.0 landed
-
Use heap_inplace_update() to unset pg_database.dathasloginevt
- 8be93177c46b 17.0 landed
-
Remove the flaky check in event_trigger_login regression test
- 4b885d01f967 17.0 landed
-
Fix instable 006_login_trigger.pl test
- 06be01eb266b 17.0 landed
-
Add support event triggers on authenticated login
- e83d1b0c40cc 17.0 landed
-
Add GUC for temporarily disabling event triggers
- 7750fefdb2b8 17.0 landed
-
Fix typo in reference to __FreeBSD__.
- e52f8b301ed5 16.0 cited
-
Restore robustness of TAP tests that wait for postmaster restart.
- f452aaf7d4a9 14.0 cited
-
Restore the portal-level snapshot after procedure COMMIT/ROLLBACK.
- 84f5c2908dad 14.0 cited