Re: On login trigger: take three

[Bharath Rupireddy]

On Mon, Oct 16, 2023 at 6:36 AM Alexander Korotkov  wrote:
>
> > On Mon, Oct 16, 2023 at 02:47:03AM +0300, Alexander Korotkov wrote:
> > > The attached revision fixes test failures spotted by
> > > commitfest.cputube.org.  Also, perl scripts passed perltidy.
>
> You are very fast and sharp eye!
> Thank you for fixing the indentation.  I just pushed fixes for the rest.

I noticed some typos related to this feature.

While on this, the event trigger example for C programming language
shown in the docs doesn't compile as-is. fmgr.h is needed.

Please see the attached patch.

[1]
2024-02-06 05:31:24.453 UTC [1401414] LOG:  tag LOGIN, event login

#include "postgres.h"

#include "commands/event_trigger.h"
#include "fmgr.h"

PG_MODULE_MAGIC;

PG_FUNCTION_INFO_V1(onlogin);

Datum
onlogin(PG_FUNCTION_ARGS)
{
EventTriggerData *trigdata;

if (!CALLED_AS_EVENT_TRIGGER(fcinfo))  /* internal error */
elog(ERROR, "not fired by event trigger manager");

trigdata = (EventTriggerData *) fcinfo->context;

elog(LOG, "tag %s, event %s", GetCommandTagName(trigdata->tag),
trigdata->event);

PG_RETURN_NULL();
}

gcc -shared -fPIC -I /home/ubuntu/postgres/pg17/include/server -o
login_trigger.so login_trigger.c

LOAD '/home/ubuntu/postgres/login_trigger.so';

CREATE FUNCTION onlogin() RETURNS event_trigger
AS '/home/ubuntu/postgres/login_trigger' LANGUAGE C;

CREATE EVENT TRIGGER onlogin ON login
EXECUTE FUNCTION onlogin();

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com


v1-0001-Fix-some-typos-in-event-trigger-docs.patch
Description: Binary data

Re: On login trigger: take three

[Alexander Korotkov]

Hi, Alexander!

On Mon, Feb 5, 2024 at 7:00 PM Alexander Lakhin  wrote:
> 05.02.2024 02:51, Alexander Korotkov wrote:
>
> > Usage of heap_inplace_update() seems appropriate for me here.  It
> > avoids trouble with both TOAST and row-level locks.  Alexander, could
> > you please recheck this fixes the problem.
>
> I've re-tested the last problematic scenario and can confirm that the fix
> works for it (though it still doesn't prevent the autovacuum issue (with
> 4b885d01 reverted)), but using heap_inplace_update() was considered risky
> in a recent discussion:
> https://www.postgresql.org/message-id/1596629.1698435146%40sss.pgh.pa.us
> So maybe it's worth to postpone such a fix till that discussion is
> finished or to look for another approach...

Thank you for pointing this out.  I don't think there is a particular
problem with this use case for the following reasons.
1) Race conditions over pg_database.dathasloginevt are protected with lock tag.
2) Unsetting pg_database.dathasloginevt of old tuple version shouldn't
cause a problem.  The next tuple version will be updated by further
connections.
However, I agree that it's better to wait for the discussion you've
pointed out before introducing another use case of
heap_inplace_update().

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Alexander Lakhin]

Hello Alexander,

05.02.2024 02:51, Alexander Korotkov wrote:


Usage of heap_inplace_update() seems appropriate for me here.  It
avoids trouble with both TOAST and row-level locks.  Alexander, could
you please recheck this fixes the problem.


I've re-tested the last problematic scenario and can confirm that the fix
works for it (though it still doesn't prevent the autovacuum issue (with
4b885d01 reverted)), but using heap_inplace_update() was considered risky
in a recent discussion:
https://www.postgresql.org/message-id/1596629.1698435146%40sss.pgh.pa.us
So maybe it's worth to postpone such a fix till that discussion is
finished or to look for another approach...

Best regards,
Alexander

Re: On login trigger: take three

[Alexander Korotkov]

On Tue, Jan 23, 2024 at 11:52 PM Alexander Korotkov
 wrote:
> On Tue, Jan 23, 2024 at 8:00 PM Alexander Lakhin  wrote:
> > Please look at the following query, which triggers an assertion failure on
> > updating the field dathasloginevt for an entry in pg_database:
> > SELECT format('CREATE DATABASE db1 LOCALE_PROVIDER icu ICU_LOCALE en 
> > ENCODING utf8
> > ICU_RULES ''' || repeat(' ', 20) || ''' TEMPLATE template0;')
> > \gexec
> > \c db1 -
> >
> > CREATE FUNCTION on_login_proc() RETURNS event_trigger AS $$
> > BEGIN
> >RAISE NOTICE 'You are welcome!';
> > END;
> > $$ LANGUAGE plpgsql;
> >
> > CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
> > on_login_proc();
> > DROP EVENT TRIGGER on_login_trigger;
> >
> > \c
> >
> > \connect: connection to server on socket "/tmp/.s.PGSQL.5432" failed: 
> > server closed the connection unexpectedly
> >
> > The stack trace of the assertion failure is:
> > ...
> > #5  0x55c8699b9b8d in ExceptionalCondition (
> >  conditionName=conditionName@entry=0x55c869a1f7c0 
> > "HaveRegisteredOrActiveSnapshot()",
> >  fileName=fileName@entry=0x55c869a1f4c6 "toast_internals.c", 
> > lineNumber=lineNumber@entry=669) at assert.c:66
> > #6  0x55c86945df0a in init_toast_snapshot (...) at toast_internals.c:669
> > #7  0x55c86945dfbe in toast_delete_datum (...) at toast_internals.c:429
> > #8  0x55c8694fd1da in toast_tuple_cleanup (...) at toast_helper.c:309
> > #9  0x55c8694b55a1 in heap_toast_insert_or_update (...) at 
> > heaptoast.c:333
> > #10 0x55c8694a8c6c in heap_update (... at heapam.c:3604
> > #11 0x55c8694a96cb in simple_heap_update (...) at heapam.c:4062
> > #12 0x55c869555b7b in CatalogTupleUpdate (...) at indexing.c:322
> > #13 0x55c8695f0725 in EventTriggerOnLogin () at event_trigger.c:957
> > ...
> >
> > Funnily enough, when Tom Lane was wondering, whether pg_database's toast
> > table could pose a risk [1], I came to the conclusion that it's impossible,
> > but that was before the login triggers introduction...
> >
> > [1] https://www.postgresql.org/message-id/1284094.1695479962%40sss.pgh.pa.us
>
> Thank you for reporting.  I'm looking into this...
> I wonder if there is a way to avoid toast update given that we don't
> touch the toasted field here.

Usage of heap_inplace_update() seems appropriate for me here.  It
avoids trouble with both TOAST and row-level locks.  Alexander, could
you please recheck this fixes the problem.

--
Regards,
Alexander Korotkov


0001-Use-heap_inplace_update-to-unset-pg_database.dath-v1.patch
Description: Binary data

Re: On login trigger: take three

[Alexander Korotkov]

On Tue, Jan 23, 2024 at 8:00 PM Alexander Lakhin  wrote:
> Please look at the following query, which triggers an assertion failure on
> updating the field dathasloginevt for an entry in pg_database:
> SELECT format('CREATE DATABASE db1 LOCALE_PROVIDER icu ICU_LOCALE en ENCODING 
> utf8
> ICU_RULES ''' || repeat(' ', 20) || ''' TEMPLATE template0;')
> \gexec
> \c db1 -
>
> CREATE FUNCTION on_login_proc() RETURNS event_trigger AS $$
> BEGIN
>RAISE NOTICE 'You are welcome!';
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
> on_login_proc();
> DROP EVENT TRIGGER on_login_trigger;
>
> \c
>
> \connect: connection to server on socket "/tmp/.s.PGSQL.5432" failed: server 
> closed the connection unexpectedly
>
> The stack trace of the assertion failure is:
> ...
> #5  0x55c8699b9b8d in ExceptionalCondition (
>  conditionName=conditionName@entry=0x55c869a1f7c0 
> "HaveRegisteredOrActiveSnapshot()",
>  fileName=fileName@entry=0x55c869a1f4c6 "toast_internals.c", 
> lineNumber=lineNumber@entry=669) at assert.c:66
> #6  0x55c86945df0a in init_toast_snapshot (...) at toast_internals.c:669
> #7  0x55c86945dfbe in toast_delete_datum (...) at toast_internals.c:429
> #8  0x55c8694fd1da in toast_tuple_cleanup (...) at toast_helper.c:309
> #9  0x55c8694b55a1 in heap_toast_insert_or_update (...) at heaptoast.c:333
> #10 0x55c8694a8c6c in heap_update (... at heapam.c:3604
> #11 0x55c8694a96cb in simple_heap_update (...) at heapam.c:4062
> #12 0x55c869555b7b in CatalogTupleUpdate (...) at indexing.c:322
> #13 0x55c8695f0725 in EventTriggerOnLogin () at event_trigger.c:957
> ...
>
> Funnily enough, when Tom Lane was wondering, whether pg_database's toast
> table could pose a risk [1], I came to the conclusion that it's impossible,
> but that was before the login triggers introduction...
>
> [1] https://www.postgresql.org/message-id/1284094.1695479962%40sss.pgh.pa.us

Thank you for reporting.  I'm looking into this...
I wonder if there is a way to avoid toast update given that we don't
touch the toasted field here.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Alexander Lakhin]

Hello Alexander and Daniel,

Please look at the following query, which triggers an assertion failure on
updating the field dathasloginevt for an entry in pg_database:
SELECT format('CREATE DATABASE db1 LOCALE_PROVIDER icu ICU_LOCALE en ENCODING 
utf8
ICU_RULES ''' || repeat(' ', 20) || ''' TEMPLATE template0;')
\gexec
\c db1 -

CREATE FUNCTION on_login_proc() RETURNS event_trigger AS $$
BEGIN
  RAISE NOTICE 'You are welcome!';
END;
$$ LANGUAGE plpgsql;

CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
on_login_proc();
DROP EVENT TRIGGER on_login_trigger;

\c

\connect: connection to server on socket "/tmp/.s.PGSQL.5432" failed: server 
closed the connection unexpectedly

The stack trace of the assertion failure is:
...
#5  0x55c8699b9b8d in ExceptionalCondition (
    conditionName=conditionName@entry=0x55c869a1f7c0 
"HaveRegisteredOrActiveSnapshot()",
    fileName=fileName@entry=0x55c869a1f4c6 "toast_internals.c", 
lineNumber=lineNumber@entry=669) at assert.c:66
#6  0x55c86945df0a in init_toast_snapshot (...) at toast_internals.c:669
#7  0x55c86945dfbe in toast_delete_datum (...) at toast_internals.c:429
#8  0x55c8694fd1da in toast_tuple_cleanup (...) at toast_helper.c:309
#9  0x55c8694b55a1 in heap_toast_insert_or_update (...) at heaptoast.c:333
#10 0x55c8694a8c6c in heap_update (... at heapam.c:3604
#11 0x55c8694a96cb in simple_heap_update (...) at heapam.c:4062
#12 0x55c869555b7b in CatalogTupleUpdate (...) at indexing.c:322
#13 0x55c8695f0725 in EventTriggerOnLogin () at event_trigger.c:957
...

Funnily enough, when Tom Lane was wondering, whether pg_database's toast
table could pose a risk [1], I came to the conclusion that it's impossible,
but that was before the login triggers introduction...

[1] https://www.postgresql.org/message-id/1284094.1695479962%40sss.pgh.pa.us

Best regards,
Alexander

Re: On login trigger: take three

[Alexander Korotkov]

On Mon, Jan 15, 2024 at 11:29 AM Daniel Gustafsson  wrote:
> > On 13 Jan 2024, at 17:00, Alexander Lakhin  wrote:
>
> > I suspected that this failure was caused by autovacuum, and I've managed to
> > reproduce it without Valgrind or slowing down a machine.
>
> This might be due to the fact that the cleanup codepath to remove the flag 
> when
> there is no login event trigger doesn't block on locking pg_database to avoid
> stalling connections.  There are no guarantees when the flag is cleared, so a
> test like this will always have potential to be flaky it seems.

+1
Thank you, Daniel.  I think you described everything absolutely
correctly.  As I wrote upthread, it doesn't seem much could be done
with this, at least within a regression test.  So, I just removed this
query from the test.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Daniel Gustafsson]

> On 13 Jan 2024, at 17:00, Alexander Lakhin  wrote:

> I suspected that this failure was caused by autovacuum, and I've managed to
> reproduce it without Valgrind or slowing down a machine.

This might be due to the fact that the cleanup codepath to remove the flag when
there is no login event trigger doesn't block on locking pg_database to avoid
stalling connections.  There are no guarantees when the flag is cleared, so a
test like this will always have potential to be flaky it seems.

--
Daniel Gustafsson

Re: On login trigger: take three

[Daniel Gustafsson]

> On 13 Jan 2024, at 17:00, Alexander Lakhin  wrote:

> DROP EVENT TRIGGER olt;
> SELECT dathasloginevt FROM pg_database WHERE datname= current_database();
>  dathasloginevt
> 
>  t
> (1 row)
> 
> Although after reconnection (\c, as done in the event_trigger_login test),
> this query returns 'f'.

This is by design in the patch.  The flag isn't changed on DROP, it is only
cleared on logins iff there is no login event trigger.  The description in the
docs is worded to indicate that it shouldn't be taken as truth for monitoring
purposes:

"This flag is used internally by PostgreSQL and should not be manually
altered or read for monitoring purposes."

--
Daniel Gustafsson

Re: On login trigger: take three

[Alexander Korotkov]

Hi, Alexander!

On Sat, Jan 13, 2024 at 6:00 PM Alexander Lakhin  wrote:
> I've discovered one more instability in the event_trigger_login test.
> Please look for example at case [1]:
> ok 213   + event_trigger   28946 ms
> not ok 214   - event_trigger_login  6430 ms
> ok 215   - fast_default19349 ms
> ok 216   - tablespace  44789 ms
> 1..216
> # 1 of 216 tests failed.

Thank you for reporting this.

I'm going to take a closer look at this tomorrow.  But I doubt I would
find a solution other than removing the flaky parts of the test.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Alexander Lakhin]

Hello Alexander,

I've discovered one more instability in the event_trigger_login test.
Please look for example at case [1]:
ok 213   + event_trigger   28946 ms
not ok 214   - event_trigger_login  6430 ms
ok 215   - fast_default    19349 ms
ok 216   - tablespace  44789 ms
1..216
# 1 of 216 tests failed.

--- /home/bf/bf-build/skink-master/HEAD/pgsql/src/test/regress/expected/event_trigger_login.out 2023-10-27 
22:55:12.574139524 +
+++ /home/bf/bf-build/skink-master/HEAD/pgsql.build/src/test/regress/results/event_trigger_login.out 2024-01-03 
23:49:50.177461816 +

@@ -40,6 +40,6 @@
 SELECT dathasloginevt FROM pg_database WHERE datname= :'DBNAME';
  dathasloginevt
 
- f
+ t
 (1 row)

2024-01-03 23:49:40.378 UTC [2235175][client backend][3/5949:0] STATEMENT:  
REINDEX INDEX CONCURRENTLY concur_reindex_ind;
...
2024-01-03 23:49:50.340 UTC [2260974][autovacuum worker][5/5439:18812] LOG:  automatic vacuum of table 
"regression.pg_catalog.pg_statistic": index scans: 1

(operations logged around 23:49:50.177461816)

I suspected that this failure was caused by autovacuum, and I've managed to
reproduce it without Valgrind or slowing down a machine.
With /tmp/extra.config:
log_autovacuum_min_duration = 0
autovacuum_naptime = 1
autovacuum = on

I run:
for ((i=1;i<10;i++)); do echo "ITERATION $i"; \
TEMP_CONFIG=/tmp/extra.config \
TESTS=$(printf 'event_trigger_login %.0s' `seq 1000`) \
make -s check-tests || break; done
and get failures on iterations 1, 2, 1...
ok 693   - event_trigger_login    15 ms
not ok 694   - event_trigger_login    15 ms
ok 695   - event_trigger_login    21 ms

Also, I've observed an anomaly after dropping a login event trigger:
CREATE FUNCTION on_login_proc() RETURNS event_trigger AS $$
BEGIN RAISE NOTICE 'You are welcome!'; END; $$ LANGUAGE plpgsql;
CREATE EVENT TRIGGER olt ON login EXECUTE PROCEDURE on_login_proc();
SELECT dathasloginevt FROM pg_database WHERE datname= current_database();
 dathasloginevt

 t
(1 row)

DROP EVENT TRIGGER olt;
SELECT dathasloginevt FROM pg_database WHERE datname= current_database();
 dathasloginevt

 t
(1 row)

Although after reconnection (\c, as done in the event_trigger_login test),
this query returns 'f'.

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink=2024-01-03%2023%3A04%3A20
[2] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink=2023-12-26%2019%3A33%3A02

Best regards,
Alexander

Re: On login trigger: take three

[Tom Lane]

Alexander Korotkov  writes:
> On Sun, Oct 29, 2023 at 6:16 PM Tom Lane  wrote:
>> It looks to me that what happened here is that the backend completed the
>> authentication handshake, and then the login trigger caused a FATAL exit,
>> and after than the connected psql session tried to send "SELECT 1" on
>> an already-closed pipe.  That failed, causing IPC::Run to panic.

Looking closer, what must have happened is that the psql session ended
before IPC::Run could jam 'SELECT 1' into its stdin.  I wonder if this
could be stabilized by doing 'psql -c "SELECT 1"' and not having to
write anything to the child process stdin?  But I'm not sure this test
case is valuable enough to put a great deal of work into.

>> mamba is a fairly slow machine and doubtless has timing a bit different
>> from what this test was created on.  But I doubt there is any way to make
>> this behavior perfectly stable across a range of machines, so I recommend
>> just removing the test case involving a fatal exit.

> Makes sense.  Are you good with the attached patch?

OK by me, though I note you misspelled "mallory" in the comment.

regards, tom lane

Re: On login trigger: take three

[Alexander Korotkov]

On Sun, Oct 29, 2023 at 6:16 PM Tom Lane  wrote:
> Mikhail Gribkov  writes:
> > Just for a more complete picture of the final state here.
> > I have posted the described fix (for avoiding race condition in the tests)
> > separately:
> > https://commitfest.postgresql.org/45/4616/
>
> It turns out that the TAP test for this feature (006_login_trigger.pl)
> also has a race condition:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-10-28%2003%3A33%3A28
>
> The critical bit of the log is
>
> ack Broken pipe: write( 14, 'SELECT 1;' ) at 
> /usr/pkg/lib/perl5/vendor_perl/5.36.0/IPC/Run/IO.pm line 550.
>
> It looks to me that what happened here is that the backend completed the
> authentication handshake, and then the login trigger caused a FATAL exit,
> and after than the connected psql session tried to send "SELECT 1" on
> an already-closed pipe.  That failed, causing IPC::Run to panic.
>
> mamba is a fairly slow machine and doubtless has timing a bit different
> from what this test was created on.  But I doubt there is any way to make
> this behavior perfectly stable across a range of machines, so I recommend
> just removing the test case involving a fatal exit.

Makes sense.  Are you good with the attached patch?

--
Regards,
Alexander Korotkov


fix_login_trigger_test_instability.patch
Description: Binary data

Re: On login trigger: take three

[Tom Lane]

Mikhail Gribkov  writes:
> Just for a more complete picture of the final state here.
> I have posted the described fix (for avoiding race condition in the tests)
> separately:
> https://commitfest.postgresql.org/45/4616/

It turns out that the TAP test for this feature (006_login_trigger.pl)
also has a race condition:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-10-28%2003%3A33%3A28

The critical bit of the log is

ack Broken pipe: write( 14, 'SELECT 1;' ) at 
/usr/pkg/lib/perl5/vendor_perl/5.36.0/IPC/Run/IO.pm line 550.

It looks to me that what happened here is that the backend completed the
authentication handshake, and then the login trigger caused a FATAL exit,
and after than the connected psql session tried to send "SELECT 1" on
an already-closed pipe.  That failed, causing IPC::Run to panic.

mamba is a fairly slow machine and doubtless has timing a bit different
from what this test was created on.  But I doubt there is any way to make
this behavior perfectly stable across a range of machines, so I recommend
just removing the test case involving a fatal exit.

regards, tom lane

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Alexander,

>> Thank you for catching it.  Please, post this.

Just for a more complete picture of the final state here.
I have posted the described fix (for avoiding race condition in the tests)
separately:
https://commitfest.postgresql.org/45/4616/

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com



>

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Alexander,

Sorry for my long offline and thanks for the activity. So should we close
the patch on the commitfest page now?

By the way I had one more issue with the login trigger tests (quite a rare
one though). A race condition may occur on some systems, when oidjoins test
starts a moment later than normally and affects logins count for on-login
trigger test. Thus I had to split event_trigger and oidjoins tests into
separate parallel groups. I'll post this as an independent patch then.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Mon, Oct 16, 2023 at 4:05 AM Alexander Korotkov 
wrote:

> On Mon, Oct 16, 2023 at 4:00 AM Michael Paquier 
> wrote:
> > On Mon, Oct 16, 2023 at 02:47:03AM +0300, Alexander Korotkov wrote:
> > > The attached revision fixes test failures spotted by
> > > commitfest.cputube.org.  Also, perl scripts passed perltidy.
> >
> > Still you've missed a few things.  At quick glance:
> > - The code indentation was off a bit in event_trigger.c.
> > - 005_login_trigger.pl fails if the code is compiled with
> > ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS because a WARNING is
> > reported in test "create tmp objects: err equals".
> > - 005_sspi.pl is older than the new test 005_login_trigger.pl, could
> > you rename it with a different number?
>
> You are very fast and sharp eye!
> Thank you for fixing the indentation.  I just pushed fixes for the rest.
>
> --
> Regards,
> Alexander Korotkov
>

Re: On login trigger: take three

[Alexander Korotkov]

Hi, Mikhail.

On Wed, Oct 18, 2023 at 1:30 PM Mikhail Gribkov  wrote:
> Sorry for my long offline and thanks for the activity. So should we close the 
> patch on the commitfest page now?

I have just done this.

> By the way I had one more issue with the login trigger tests (quite a rare 
> one though). A race condition may occur on some systems, when oidjoins test 
> starts a moment later than normally and affects logins count for on-login 
> trigger test. Thus I had to split event_trigger and oidjoins tests into 
> separate parallel groups. I'll post this as an independent patch then.

Thank you for catching it.  Please, post this.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Alexander Korotkov]

On Mon, Oct 16, 2023 at 4:00 AM Michael Paquier  wrote:
> On Mon, Oct 16, 2023 at 02:47:03AM +0300, Alexander Korotkov wrote:
> > The attached revision fixes test failures spotted by
> > commitfest.cputube.org.  Also, perl scripts passed perltidy.
>
> Still you've missed a few things.  At quick glance:
> - The code indentation was off a bit in event_trigger.c.
> - 005_login_trigger.pl fails if the code is compiled with
> ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS because a WARNING is
> reported in test "create tmp objects: err equals".
> - 005_sspi.pl is older than the new test 005_login_trigger.pl, could
> you rename it with a different number?

You are very fast and sharp eye!
Thank you for fixing the indentation.  I just pushed fixes for the rest.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Michael Paquier]

On Mon, Oct 16, 2023 at 02:47:03AM +0300, Alexander Korotkov wrote:
> The attached revision fixes test failures spotted by
> commitfest.cputube.org.  Also, perl scripts passed perltidy.

Still you've missed a few things.  At quick glance:
- The code indentation was off a bit in event_trigger.c.
- 005_login_trigger.pl fails if the code is compiled with
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS because a WARNING is
reported in test "create tmp objects: err equals".
- 005_sspi.pl is older than the new test 005_login_trigger.pl, could
you rename it with a different number?
--
Michael


signature.asc
Description: PGP signature

Re: On login trigger: take three

[Alexander Korotkov]

On Sat, Oct 14, 2023 at 2:10 AM Alexander Korotkov  wrote:
> On Fri, Oct 13, 2023 at 11:26 AM Alexander Korotkov
>  wrote:
> > On Fri, Oct 13, 2023 at 4:18 AM Robert Haas  wrote:
> > > On Thu, Oct 12, 2023 at 6:54 PM Alexander Korotkov  
> > > wrote:
> > > > On Thu, Oct 12, 2023 at 8:35 PM Robert Haas  
> > > > wrote:
> > >
> > > > > Doesn't that mean that if you create the first login trigger in a
> > > > > database and leave the transaction open, nobody can connect to that
> > > > > database until the transaction ends?
> > > >
> > > > It doesn't mean that, because when trying to reset the flag v44 does
> > > > conditional lock.  So, if another transaction is holding the log we
> > > > will just skip resetting the flag.  So, the flag will be cleared on
> > > > the first connection after that transaction ends.
> > >
> > > But in the scenario I am describing the flag is being set, not reset.
> >
> > Sorry, it seems I just missed some logical steps.  Imagine, that
> > transaction A created the first login trigger and hangs open.  Then
> > the new connection B sees no visible triggers yet, but dathasloginevt
> > flag is set.  Therefore, connection B tries conditional lock but just
> > gives up because the lock is held by transaction A.
> >
> > Also, note that the lock has been just some lock with a custom tag.
> > It doesn't effectively block the database.  You may think about it as
> > of custom advisory lock.
>
> I've revised the comments about the lock a bit.  I've also run some
> tests regarding the connection time (5 runs).
>
> v45, event_triggers=on: avg=3.081ms, min=2.992ms, max=3.146ms
> v45, event_triggers=off: avg=3.132ms, min=3.048ms, max=3.186ms
> master: 3.080ms, min=3.023ms, max=3.167ms
>
> So, no measurable overhead (not surprising since no extra catalog lookup).
> I think this patch is in the commitable shape.  Any objections?

The attached revision fixes test failures spotted by
commitfest.cputube.org.  Also, perl scripts passed perltidy.

--
Regards,
Alexander Korotkov


0001-Add-support-event-triggers-on-authenticated-logi-v46.patch
Description: Binary data

Re: On login trigger: take three

[Alexander Korotkov]

On Fri, Oct 13, 2023 at 11:26 AM Alexander Korotkov
 wrote:
> On Fri, Oct 13, 2023 at 4:18 AM Robert Haas  wrote:
> > On Thu, Oct 12, 2023 at 6:54 PM Alexander Korotkov  
> > wrote:
> > > On Thu, Oct 12, 2023 at 8:35 PM Robert Haas  wrote:
> >
> > > > Doesn't that mean that if you create the first login trigger in a
> > > > database and leave the transaction open, nobody can connect to that
> > > > database until the transaction ends?
> > >
> > > It doesn't mean that, because when trying to reset the flag v44 does
> > > conditional lock.  So, if another transaction is holding the log we
> > > will just skip resetting the flag.  So, the flag will be cleared on
> > > the first connection after that transaction ends.
> >
> > But in the scenario I am describing the flag is being set, not reset.
>
> Sorry, it seems I just missed some logical steps.  Imagine, that
> transaction A created the first login trigger and hangs open.  Then
> the new connection B sees no visible triggers yet, but dathasloginevt
> flag is set.  Therefore, connection B tries conditional lock but just
> gives up because the lock is held by transaction A.
>
> Also, note that the lock has been just some lock with a custom tag.
> It doesn't effectively block the database.  You may think about it as
> of custom advisory lock.

I've revised the comments about the lock a bit.  I've also run some
tests regarding the connection time (5 runs).

v45, event_triggers=on: avg=3.081ms, min=2.992ms, max=3.146ms
v45, event_triggers=off: avg=3.132ms, min=3.048ms, max=3.186ms
master: 3.080ms, min=3.023ms, max=3.167ms

So, no measurable overhead (not surprising since no extra catalog lookup).
I think this patch is in the commitable shape.  Any objections?

--
Regards,
Alexander Korotkov
From 1d59d7fe6f510a4606600b9bf41b5dbaa6d2b283 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov 
Date: Mon, 9 Oct 2023 15:00:26 +0300
Subject: [PATCH] Add support event triggers on authenticated login

This commit introduces trigger on login event, allowing to fire some actions
right on the user connection.  This can be useful for logging or connection
check purposes as well as for some personalization of environment.  Usage
details are described in the documentation included, but shortly usage is
the same as for other triggers: create function returning event_trigger and
then create event trigger on login event.

In order to prevent the connection time overhead when there are no triggers
the commit introduces pg_database.dathasloginevt flag, which indicates database
has active login triggers.  This flag is set by CREATE/ALTER EVENT TRIGGER
command, and unset at connection time when no active triggers found.

Author: Konstantin Knizhnik, Mikhail Gribkov
Discussion: https://postgr.es/m/0d46d29f-4558-3af9-9c85-7774e14a7709%40postgrespro.ru
Reviewed-by: Pavel Stehule, Takayuki Tsunakawa, Greg Nancarrow, Ivan Panchenko
Reviewed-by: Daniel Gustafsson, Teodor Sigaev, Robert Haas, Andres Freund
Reviewed-by: Tom Lane, Andrey Sokolov, Zhihong Yu, Sergey Shinderuk
Reviewed-by: Gregory Stark, Nikita Malakhov, Ted Yu
---
 doc/src/sgml/bki.sgml |   2 +-
 doc/src/sgml/catalogs.sgml|  13 ++
 doc/src/sgml/ecpg.sgml|   2 +
 doc/src/sgml/event-trigger.sgml   |  94 +
 src/backend/commands/dbcommands.c |  17 +-
 src/backend/commands/event_trigger.c  | 179 +-
 src/backend/storage/lmgr/lmgr.c   |  38 
 src/backend/tcop/postgres.c   |   4 +
 src/backend/utils/cache/evtcache.c|   2 +
 src/backend/utils/init/globals.c  |   2 +
 src/backend/utils/init/postinit.c |   1 +
 src/bin/pg_dump/pg_dump.c |   5 +
 src/bin/psql/tab-complete.c   |   4 +-
 src/include/catalog/pg_database.dat   |   2 +-
 src/include/catalog/pg_database.h |   3 +
 src/include/commands/event_trigger.h  |   1 +
 src/include/miscadmin.h   |   2 +
 src/include/storage/lmgr.h|   2 +
 src/include/tcop/cmdtaglist.h |   1 +
 src/include/utils/evtcache.h  |   3 +-
 .../authentication/t/005_login_trigger.pl | 157 +++
 src/test/recovery/t/001_stream_rep.pl |  23 +++
 src/test/regress/expected/event_trigger.out   |  45 +
 src/test/regress/sql/event_trigger.sql|  26 +++
 24 files changed, 608 insertions(+), 20 deletions(-)
 create mode 100644 src/test/authentication/t/005_login_trigger.pl

diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml
index f71644e3989..315ba819514 100644
--- a/doc/src/sgml/bki.sgml
+++ b/doc/src/sgml/bki.sgml
@@ -184,7 +184,7 @@
   descr => 'database\'s default template',
   datname => 'template1', encoding => 'ENCODING',
   datlocprovider => 'LOCALE_PROVIDER', datistemplate => 't',
-  datallowconn => 't', datconnlimit => '-1', datfrozenxid => '0',
+  

Re: On login trigger: take three

[Alexander Korotkov]

On Fri, Oct 13, 2023 at 4:18 AM Robert Haas  wrote:
> On Thu, Oct 12, 2023 at 6:54 PM Alexander Korotkov  
> wrote:
> > On Thu, Oct 12, 2023 at 8:35 PM Robert Haas  wrote:
>
> > > Doesn't that mean that if you create the first login trigger in a
> > > database and leave the transaction open, nobody can connect to that
> > > database until the transaction ends?
> >
> > It doesn't mean that, because when trying to reset the flag v44 does
> > conditional lock.  So, if another transaction is holding the log we
> > will just skip resetting the flag.  So, the flag will be cleared on
> > the first connection after that transaction ends.
>
> But in the scenario I am describing the flag is being set, not reset.

Sorry, it seems I just missed some logical steps.  Imagine, that
transaction A created the first login trigger and hangs open.  Then
the new connection B sees no visible triggers yet, but dathasloginevt
flag is set.  Therefore, connection B tries conditional lock but just
gives up because the lock is held by transaction A.

Also, note that the lock has been just some lock with a custom tag.
It doesn't effectively block the database.  You may think about it as
of custom advisory lock.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Robert Haas]

On Thu, Oct 12, 2023 at 6:54 PM Alexander Korotkov  wrote:
> On Thu, Oct 12, 2023 at 8:35 PM Robert Haas  wrote:

> > Doesn't that mean that if you create the first login trigger in a
> > database and leave the transaction open, nobody can connect to that
> > database until the transaction ends?
>
> It doesn't mean that, because when trying to reset the flag v44 does
> conditional lock.  So, if another transaction is holding the log we
> will just skip resetting the flag.  So, the flag will be cleared on
> the first connection after that transaction ends.

But in the scenario I am describing the flag is being set, not reset.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Alexander Korotkov]

On Thu, Oct 12, 2023 at 8:35 PM Robert Haas  wrote:
> On Tue, Oct 10, 2023 at 3:43 PM Alexander Korotkov  
> wrote:
> > Yep, in v43 it worked that way.  One transaction has to wait for
> > another finishing update of pg_database tuple, then fails.  This is
> > obviously ridiculous.  Since overlapping setters of flag will have to
> > wait anyway, I changed lock mode in v44 for them to
> > AccessExclusiveLock.  Now, waiting transaction then sees the updated
> > tuple and doesn't fail.
>
> Doesn't that mean that if you create the first login trigger in a
> database and leave the transaction open, nobody can connect to that
> database until the transaction ends?

It doesn't mean that, because when trying to reset the flag v44 does
conditional lock.  So, if another transaction is holding the log we
will just skip resetting the flag.  So, the flag will be cleared on
the first connection after that transaction ends.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Robert Haas]

On Tue, Oct 10, 2023 at 3:43 PM Alexander Korotkov  wrote:
> Yep, in v43 it worked that way.  One transaction has to wait for
> another finishing update of pg_database tuple, then fails.  This is
> obviously ridiculous.  Since overlapping setters of flag will have to
> wait anyway, I changed lock mode in v44 for them to
> AccessExclusiveLock.  Now, waiting transaction then sees the updated
> tuple and doesn't fail.

Doesn't that mean that if you create the first login trigger in a
database and leave the transaction open, nobody can connect to that
database until the transaction ends?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Alexander Korotkov]

Hi, Robert!

Thank you for your feedback.

On Tue, Oct 10, 2023 at 5:51 PM Robert Haas  wrote:
>
> On Mon, Oct 9, 2023 at 10:11 AM Alexander Korotkov  
> wrote:
> >  * Hold lock during setting of pg_database.dathasloginevt flag (v32
> > version actually didn't prevent race condition).
>
> So ... how does getting this flag set actually work? And how does
> clearing it work?

I hope I explained that in [1].

> In the case of row-level security, you have to explicitly enable the
> flag on the table level using DDL provided for that purpose. In the
> case of relhas{rules,triggers,subclass} the flag is set automatically
> as a side-effect of some other operation. I tend to consider that the
> latter design is somewhat messy. It's potentially even messier here,
> because at least when you add a rule or a trigger to a table you're
> expecting to take a lock on the table anyway. I don't think you
> necessarily expect creating a login trigger to take a lock on the
> database. That's a bit odd and could have visible side effects. And if
> you don't, then what happens is that if you create two login triggers
> in overlapping transactions, then (1) if there were no login triggers
> previously, one of the transactions fails with an internal-looking
> error message about a concurrent tuple update and (2) if there were
> login triggers previously, then it works fine.

Yep, in v43 it worked that way.  One transaction has to wait for
another finishing update of pg_database tuple, then fails.  This is
obviously ridiculous.  Since overlapping setters of flag will have to
wait anyway, I changed lock mode in v44 for them to
AccessExclusiveLock.  Now, waiting transaction then sees the updated
tuple and doesn't fail.

> That's also a bit weird
> and surprising. Now the counter-argument could be that adding new DDL
> to enable login triggers for a database is too much cognitive burden
> and it's better to have the kind of weird and surprising behavior that
> I just discussed. I don't know that I would buy that argument, but it
> could be made ... and my real point here is that I don't even see
> these trade-offs being discussed. Apologies if they were discussed
> earlier and I just missed that; I confess to not having read every
> email message on this topic, and some of the ones I did read I read a
> long time ago.

I have read the thread quite carefully.  I don't think manual setting
of the flag was discussed.  I do think it would be extra burden for
users, and I would prefer automatic flag as long as it works
transparently and reliably.

> > This version should be good and has no overhead.  Any thoughts?
> > Daniel, could you please re-run the performance tests?
>
> Is "no overhead" an overly bold claim here?

Yes, sure.  I meant no extra lookup.  Hopefully that would mean no
measurable overhead when is no enabled login triggers.

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

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Alexander Korotkov]

Hi!

Thank you for the review.

On Tue, Oct 10, 2023 at 7:37 PM Andres Freund  wrote:
> On 2023-10-10 08:18:46 +0300, Alexander Korotkov wrote:
> > @@ -968,7 +969,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
> >
> >   if (!get_db_info(dbtemplate, ShareLock,
> >_dboid, _owner, 
> > _encoding,
> > -  _istemplate, _allowconn,
> > +  _istemplate, _allowconn, 
> > _hasloginevt,
> >_frozenxid, _minmxid, 
> > _deftablespace,
> >_collate, _ctype, 
> > _iculocale, _icurules, _locprovider,
> >_collversion))
>
> This isn't your fault, but this imo has become unreadable. Think we ought to
> move the information about a database to a struct.

Should I do this in a separate patch?

> > @@ -296,6 +306,13 @@ insert_event_trigger_tuple(const char *trigname, const 
> > char *eventname, Oid evtO
> >   CatalogTupleInsert(tgrel, tuple);
> >   heap_freetuple(tuple);
> >
> > + /*
> > +  * Login event triggers have an additional flag in pg_database to 
> > allow
> > +  * faster lookups in hot codepaths. Set the flag unless already True.
> > +  */
> > + if (strcmp(eventname, "login") == 0)
> > + SetDatatabaseHasLoginEventTriggers();
>
> It's not really faster lookups, it's no lookups, right?

Yes, no extra lookups. Fixed.

> >   /* Depend on owner. */
> >   recordDependencyOnOwner(EventTriggerRelationId, trigoid, evtOwner);
> >
> > @@ -357,6 +374,39 @@ filter_list_to_array(List *filterlist)
> >   return PointerGetDatum(construct_array_builtin(data, l, TEXTOID));
> >  }
> >
> > +/*
> > + * Set pg_database.dathasloginevt flag for current database indicating that
> > + * current database has on login triggers.
> > + */
> > +void
> > +SetDatatabaseHasLoginEventTriggers(void)
> > +{
> > + /* Set dathasloginevt flag in pg_database */
> > + Form_pg_database db;
> > + Relationpg_db = table_open(DatabaseRelationId, 
> > RowExclusiveLock);
> > + HeapTuple   tuple;
> > +
> > + /*
> > +  * Use shared lock to prevent a conflit with EventTriggerOnLogin() 
> > trying
> > +  * to reset pg_database.dathasloginevt flag.  Note that we use
> > +  * AccessShareLock allowing setters concurently.
> > +  */
> > + LockSharedObject(DatabaseRelationId, MyDatabaseId, 0, 
> > AccessShareLock);
>
> That seems like a very odd approach - how does this avoid concurrency issues
> with one backend setting and another unsetting the flag?  And outside of that,
> won't this just lead to concurrently updated tuples?

When backend unsets the flag, it acquires the lock first.  If lock is
acquired successfully then no other backends hold it. If the
concurrent backend have already inserted an event trigger then we will
detect it by rechecking event list under lock. If the concurrent
backend inserted/enabled event trigger and waiting for the lock, then
it will set the flag once we release the lock.

> > + tuple = SearchSysCacheCopy1(DATABASEOID, 
> > ObjectIdGetDatum(MyDatabaseId));
> > + if (!HeapTupleIsValid(tuple))
> > + elog(ERROR, "cache lookup failed for database %u", 
> > MyDatabaseId);
> > + db = (Form_pg_database) GETSTRUCT(tuple);
> > + if (!db->dathasloginevt)
> > + {
> > + db->dathasloginevt = true;
> > + CatalogTupleUpdate(pg_db, >t_self, tuple);
> > + CommandCounterIncrement();
> > + }
> > + table_close(pg_db, RowExclusiveLock);
> > + heap_freetuple(tuple);
> > +}
> > +
> >  /*
> >   * ALTER EVENT TRIGGER foo ENABLE|DISABLE|ENABLE ALWAYS|REPLICA
> >   */
> > @@ -391,6 +441,10 @@ AlterEventTrigger(AlterEventTrigStmt *stmt)
> >
> >   CatalogTupleUpdate(tgrel, >t_self, tup);
> >
> > + if (namestrcmp(>evtevent, "login") == 0 &&
> > + tgenabled != TRIGGER_DISABLED)
> > + SetDatatabaseHasLoginEventTriggers();
> > +
> >   InvokeObjectPostAlterHook(EventTriggerRelationId,
> > trigoid, 0);
> >
> > @@ -557,7 +611,7 @@ filter_event_trigger(CommandTag tag, 
> > EventTriggerCacheItem *item)
> >  static List *
> >  EventTriggerCommonSetup(Node *parsetree,
> >   EventTriggerEvent event, 
> > const char *eventstr,
> > - EventTriggerData *trigdata)
> > + EventTriggerData *trigdata, 
> > bool unfiltered)
> >  {
> >   CommandTag  tag;
> >   List   *cachelist;
> > @@ -582,10 +636,15 @@ EventTriggerCommonSetup(Node *parsetree,
> >   {
> >   CommandTag  dbgtag;
> >
> > - dbgtag = CreateCommandTag(parsetree);
> > + if (event == EVT_Login)
> > + dbgtag = 

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2023-10-10 08:18:46 +0300, Alexander Korotkov wrote:
> @@ -968,7 +969,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
>  
>   if (!get_db_info(dbtemplate, ShareLock,
>_dboid, _owner, _encoding,
> -  _istemplate, _allowconn,
> +  _istemplate, _allowconn, 
> _hasloginevt,
>_frozenxid, _minmxid, 
> _deftablespace,
>_collate, _ctype, 
> _iculocale, _icurules, _locprovider,
>_collversion))

This isn't your fault, but this imo has become unreadable. Think we ought to
move the information about a database to a struct.


> @@ -296,6 +306,13 @@ insert_event_trigger_tuple(const char *trigname, const 
> char *eventname, Oid evtO
>   CatalogTupleInsert(tgrel, tuple);
>   heap_freetuple(tuple);
>  
> + /*
> +  * Login event triggers have an additional flag in pg_database to allow
> +  * faster lookups in hot codepaths. Set the flag unless already True.
> +  */
> + if (strcmp(eventname, "login") == 0)
> + SetDatatabaseHasLoginEventTriggers();

It's not really faster lookups, it's no lookups, right?


>   /* Depend on owner. */
>   recordDependencyOnOwner(EventTriggerRelationId, trigoid, evtOwner);
>  
> @@ -357,6 +374,39 @@ filter_list_to_array(List *filterlist)
>   return PointerGetDatum(construct_array_builtin(data, l, TEXTOID));
>  }
>  
> +/*
> + * Set pg_database.dathasloginevt flag for current database indicating that
> + * current database has on login triggers.
> + */
> +void
> +SetDatatabaseHasLoginEventTriggers(void)
> +{
> + /* Set dathasloginevt flag in pg_database */
> + Form_pg_database db;
> + Relationpg_db = table_open(DatabaseRelationId, 
> RowExclusiveLock);
> + HeapTuple   tuple;
> +
> + /*
> +  * Use shared lock to prevent a conflit with EventTriggerOnLogin() 
> trying
> +  * to reset pg_database.dathasloginevt flag.  Note that we use
> +  * AccessShareLock allowing setters concurently.
> +  */
> + LockSharedObject(DatabaseRelationId, MyDatabaseId, 0, AccessShareLock);

That seems like a very odd approach - how does this avoid concurrency issues
with one backend setting and another unsetting the flag?  And outside of that,
won't this just lead to concurrently updated tuples?


> + tuple = SearchSysCacheCopy1(DATABASEOID, 
> ObjectIdGetDatum(MyDatabaseId));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for database %u", 
> MyDatabaseId);
> + db = (Form_pg_database) GETSTRUCT(tuple);
> + if (!db->dathasloginevt)
> + {
> + db->dathasloginevt = true;
> + CatalogTupleUpdate(pg_db, >t_self, tuple);
> + CommandCounterIncrement();
> + }
> + table_close(pg_db, RowExclusiveLock);
> + heap_freetuple(tuple);
> +}
> +
>  /*
>   * ALTER EVENT TRIGGER foo ENABLE|DISABLE|ENABLE ALWAYS|REPLICA
>   */
> @@ -391,6 +441,10 @@ AlterEventTrigger(AlterEventTrigStmt *stmt)
>  
>   CatalogTupleUpdate(tgrel, >t_self, tup);
>  
> + if (namestrcmp(>evtevent, "login") == 0 &&
> + tgenabled != TRIGGER_DISABLED)
> + SetDatatabaseHasLoginEventTriggers();
> +
>   InvokeObjectPostAlterHook(EventTriggerRelationId,
> trigoid, 0);
>  
> @@ -557,7 +611,7 @@ filter_event_trigger(CommandTag tag, 
> EventTriggerCacheItem *item)
>  static List *
>  EventTriggerCommonSetup(Node *parsetree,
>   EventTriggerEvent event, const 
> char *eventstr,
> - EventTriggerData *trigdata)
> + EventTriggerData *trigdata, 
> bool unfiltered)
>  {
>   CommandTag  tag;
>   List   *cachelist;
> @@ -582,10 +636,15 @@ EventTriggerCommonSetup(Node *parsetree,
>   {
>   CommandTag  dbgtag;
>  
> - dbgtag = CreateCommandTag(parsetree);
> + if (event == EVT_Login)
> + dbgtag = CMDTAG_LOGIN;
> + else
> + dbgtag = CreateCommandTag(parsetree);
> +
>   if (event == EVT_DDLCommandStart ||
>   event == EVT_DDLCommandEnd ||
> - event == EVT_SQLDrop)
> + event == EVT_SQLDrop ||
> + event == EVT_Login)
>   {
>   if (!command_tag_event_trigger_ok(dbgtag))
>   elog(ERROR, "unexpected command tag \"%s\"", 
> GetCommandTagName(dbgtag));
> @@ -604,7 +663,10 @@ EventTriggerCommonSetup(Node *parsetree,
>   return NIL;
>  
>   /* Get the command tag. */
> - tag = CreateCommandTag(parsetree);
> + if (event == EVT_Login)
> +

Re: On login trigger: take three

[Robert Haas]

On Mon, Oct 9, 2023 at 10:11 AM Alexander Korotkov  wrote:
>  * Hold lock during setting of pg_database.dathasloginevt flag (v32
> version actually didn't prevent race condition).

So ... how does getting this flag set actually work? And how does
clearing it work?

In the case of row-level security, you have to explicitly enable the
flag on the table level using DDL provided for that purpose. In the
case of relhas{rules,triggers,subclass} the flag is set automatically
as a side-effect of some other operation. I tend to consider that the
latter design is somewhat messy. It's potentially even messier here,
because at least when you add a rule or a trigger to a table you're
expecting to take a lock on the table anyway. I don't think you
necessarily expect creating a login trigger to take a lock on the
database. That's a bit odd and could have visible side effects. And if
you don't, then what happens is that if you create two login triggers
in overlapping transactions, then (1) if there were no login triggers
previously, one of the transactions fails with an internal-looking
error message about a concurrent tuple update and (2) if there were
login triggers previously, then it works fine. That's also a bit weird
and surprising. Now the counter-argument could be that adding new DDL
to enable login triggers for a database is too much cognitive burden
and it's better to have the kind of weird and surprising behavior that
I just discussed. I don't know that I would buy that argument, but it
could be made ... and my real point here is that I don't even see
these trade-offs being discussed. Apologies if they were discussed
earlier and I just missed that; I confess to not having read every
email message on this topic, and some of the ones I did read I read a
long time ago.

> This version should be good and has no overhead.  Any thoughts?
> Daniel, could you please re-run the performance tests?

Is "no overhead" an overly bold claim here?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Alexander Korotkov]

On Mon, Oct 9, 2023 at 11:58 PM Andres Freund  wrote:
> On 2023-10-09 17:11:25 +0300, Alexander Korotkov wrote:
> > This version should be good and has no overhead.  Any thoughts?
>
> I think you forgot to attach the patch?

That's it!

--
Regards,
Alexander Korotkov


0001-Add-support-event-triggers-on-authenticated-logi-v43.patch
Description: Binary data

Re: On login trigger: take three

[Andres Freund]

On 2023-10-09 17:11:25 +0300, Alexander Korotkov wrote:
> This version should be good and has no overhead.  Any thoughts?

I think you forgot to attach the patch?

Re: On login trigger: take three

[Alexander Korotkov]

Hi!

On Tue, Oct 3, 2023 at 8:35 PM Alexander Korotkov  wrote:
> Thank you for the interesting ideas. I'd like to try to revive the
> version with the flag in pg_database.  Will use other ideas as backup
> if no success.

I've revived the patch version with pg_database.dathasloginevt flag.
I took v32 version [1] and made the following changes.

 * Incorporate enchantments made on flagless version of patch.
 * Read dathasloginevt during InitPostgres() to prevent extra catalog
access and even more notable StartTransactionCommand() when there are
no login triggers.
 * Hold lock during setting of pg_database.dathasloginevt flag (v32
version actually didn't prevent race condition).
 * Fix AlterEventTrigger() to check event name not trigger name
 * Acquire conditional lock while resetting pg_database.dathasloginevt
flag to prevent new database connection to hang waiting another
transaction to finish.

This version should be good and has no overhead.  Any thoughts?
Daniel, could you please re-run the performance tests?

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

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Alexander Korotkov]

Hi, Robert!

On Tue, Oct 3, 2023 at 5:21 PM Robert Haas  wrote:
> On Tue, Oct 3, 2023 at 9:43 AM Daniel Gustafsson  wrote:
> > That's exactly what happens, the patch is using BuildEventTriggerCache() to
> > build the hash for EVT which is then checked for login triggers.  This is
> > clearly the bottleneck and there needs to be a fast-path.  There used to be 
> > a
> > cache flag in an earlier version of the patch but it was a but klugy, a 
> > version
> > of that needs to be reimplemented for this patch to fly.
>
> So I haven't looked at this patch, but we basically saying that only
> the superuser can create login triggers, and if they do, those
> triggers apply to every single user on the system? That would seem to
> be the logical extension of the existing event trigger mechanism, but
> it isn't obviously as good of a fit for this case as it is for other
> cases where event triggers are a thing.
>
> Changing the catalog representation could be a way around this. What
> if you only allowed one login trigger per database, and instead of
> being stored in pg_event_trigger, the OID of the function gets
> recorded in the pg_database row? Then this would be a lot cheaper
> since we have to fetch the pg_database row anyway. Or change the SQL
> syntax to something entirely new so you can have different login
> triggers for different users -- and maybe users are allowed to create
> their own -- but the relevant ones can be found by an index scan
> instead of a sequential scan.
>
> I'm just spitballing here. If you think the present design is good and
> just want to try to speed it up, I'm not deeply opposed to that. But
> it's also not obvious to me how to stick a cache in front of something
> that's basically a full-table scan.

Thank you for the interesting ideas. I'd like to try to revive the
version with the flag in pg_database.  Will use other ideas as backup
if no success.

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Robert Haas]

On Tue, Oct 3, 2023 at 9:43 AM Daniel Gustafsson  wrote:
> That's exactly what happens, the patch is using BuildEventTriggerCache() to
> build the hash for EVT which is then checked for login triggers.  This is
> clearly the bottleneck and there needs to be a fast-path.  There used to be a
> cache flag in an earlier version of the patch but it was a but klugy, a 
> version
> of that needs to be reimplemented for this patch to fly.

So I haven't looked at this patch, but we basically saying that only
the superuser can create login triggers, and if they do, those
triggers apply to every single user on the system? That would seem to
be the logical extension of the existing event trigger mechanism, but
it isn't obviously as good of a fit for this case as it is for other
cases where event triggers are a thing.

Changing the catalog representation could be a way around this. What
if you only allowed one login trigger per database, and instead of
being stored in pg_event_trigger, the OID of the function gets
recorded in the pg_database row? Then this would be a lot cheaper
since we have to fetch the pg_database row anyway. Or change the SQL
syntax to something entirely new so you can have different login
triggers for different users -- and maybe users are allowed to create
their own -- but the relevant ones can be found by an index scan
instead of a sequential scan.

I'm just spitballing here. If you think the present design is good and
just want to try to speed it up, I'm not deeply opposed to that. But
it's also not obvious to me how to stick a cache in front of something
that's basically a full-table scan.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Daniel Gustafsson]

> On 2 Oct 2023, at 20:10, Robert Haas  wrote:
> 
> Sorry to have gone dark on this for a long time after having been
> asked for my input back in March. I'm not having a great time trying
> to keep up with email, and the threads getting split up makes it a lot
> worse for me.

Not a problem, thanks for chiming in.

> On Fri, Sep 29, 2023 at 6:15 AM Daniel Gustafsson  wrote:
>> 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 seems kind of crazy to me. Why does it happen? It sounds to me
> like we must be doing a lot of extra catalog access to find out
> whether there are any on-login event triggers. Like maybe a sequential
> scan of pg_event_trigger.

That's exactly what happens, the patch is using BuildEventTriggerCache() to
build the hash for EVT which is then checked for login triggers.  This is
clearly the bottleneck and there needs to be a fast-path.  There used to be a
cache flag in an earlier version of the patch but it was a but klugy, a version
of that needs to be reimplemented for this patch to fly.

> I think a lot of users would say that logins on PostgreSQL are too slow 
> already.

Agreed.

--
Daniel Gustafsson

Re: On login trigger: take three

[Robert Haas]

Sorry to have gone dark on this for a long time after having been
asked for my input back in March. I'm not having a great time trying
to keep up with email, and the threads getting split up makes it a lot
worse for me.

On Fri, Sep 29, 2023 at 6:15 AM Daniel Gustafsson  wrote:
> 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 seems kind of crazy to me. Why does it happen? It sounds to me
like we must be doing a lot of extra catalog access to find out
whether there are any on-login event triggers. Like maybe a sequential
scan of pg_event_trigger. Maybe we need to engineer a way to avoid
that. I don't have a brilliant idea off-hand, but I feel like there
should be something we can do. I think a lot of users would say that
logins on PostgreSQL are too slow already.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Alexander Korotkov]

On Fri, Sep 29, 2023 at 1:15 PM Daniel Gustafsson  wrote:
> > On 28 Sep 2023, at 23:50, Alexander Korotkov  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


0001-Add-support-of-event-triggers-on-authenticated-l-v42.patch
Description: Binary data

Re: On login trigger: take three

[Daniel Gustafsson]

> On 28 Sep 2023, at 23:50, Alexander Korotkov  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?

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?


Thanks for working on this!

--
Daniel Gustafsson

Re: On login trigger: take three

[Alexander Korotkov]

Hi, Daniel!

On Mon, Sep 25, 2023 at 3:42 PM Daniel Gustafsson  wrote:
> > On 25 Sep 2023, at 11:13, Alexander Korotkov  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


0001-Add-support-of-event-triggers-on-authenticated-l-v41.patch
Description: Binary data

Re: On login trigger: take three

[Daniel Gustafsson]

> On 25 Sep 2023, at 11:13, Alexander Korotkov  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).

> 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)?

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

--
Daniel Gustafsson

Re: On login trigger: take three

[Alexander Korotkov]

Hi!

On Wed, Jun 14, 2023 at 10:49 PM Mikhail Gribkov  wrote:
> The attached v40 patch is a fresh rebase on master branch to actualize the 
> state before the upcoming commitfest.
> Nothing has changed beyond rebasing.
>
> And just for convenience, here is a link to the exact message of the thread
> describing the current approach:
> https://www.postgresql.org/message-id/CAMEv5_vg4aJOoUC74XJm%2B5B7%2BTF1nT-Yhtg%2BawtBOESXG5Grfg%40mail.gmail.com

Thank you for the update.  I think the patch is interesting and
demanding.  The code, docs and tests seem to be quite polished
already.  Simultaneously, the thread is long and spread over a long
time.  So, it's easy to lose track.  I'd like to do a short summary of
design issues on this thread.

1. Initially the patch introduced "on login" triggers. It was unclear
if they need to rerun on RESET ALL or DISCARD ALL [1]. The new name is
"client connection" trigger, which seems fine [2].

2. Another question is how to deal with triggers, which hangs or fails
with error [1]. One possible way to workaround that is single-user
mode, which is already advised to workaround the errors in other event
triggers. However, in perspective single-user mode might be deleted.
Also, single-user mode is considered as a worst-case scenario recovery
tool, while it's very easy to block the database connections with
client connection triggers. As addition/alternative to single-user
mode, GUC options to disable all event triggers and/or client
connection triggers. Finally, 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].

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.

4. [10] points that there is no clean way to store information about
unsuccessful connections (declined by either authentication or
trigger). However, this is considered out-of-scope for the current
patch, and could be implemented later if needed.

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.

6. It has been also pointed out that [12] all triggers, which write
data to the database, must check pg_is_in_recovery() to work correctly
on standby. That seems to be currently reflected in the documentation.

So, for me the open issues seem to be 2, 3 and 5.  My plan to revive
this patch is to commit the GUC patch [4], recheck the overhead and
probably leave "^C in psql" problem as a separate standalone issue.
Any thoughts?

Links.

1. 
https://www.postgresql.org/message-id/CAFj8pRBdqdqvkU3mVKzoOnO+jPz-6manRV47CDEa+1jD6x6LFg%40mail.gmail.com
2. 
https://www.postgresql.org/message-id/CAFj8pRCxdQgHy8Mynk3hz6pFsqQ9BN6Vfgy0MJLtQBAUhWDf3w%40mail.gmail.com
3. 
https://www.postgresql.org/message-id/E0D5DC61-C490-45BD-A984-E8D56493EC4F%40yesql.se
4. 
https://www.postgresql.org/message-id/9140106e-f9bf-4d85-8fc8-f2d3c094a...@yesql.se
5. 
https://www.postgresql.org/message-id/20230306221010.gszjoakt5jp7oqpd%40awork3.anarazel.de
6. 
https://www.postgresql.org/message-id/90760f2d-2f9c-12ab-d2c5-e8e6fb7d08de%40postgrespro.ru
7. 
https://www.postgresql.org/message-id/CAFj8pRChwu01VLx76nKBVyScHCsd1YnBGiKfDJ6h17g4CSnUBg%40mail.gmail.com
8. 
https://www.postgresql.org/message-id/4471d472-5dfc-f2b0-ad05-0ff8d0a3bb0c%40postgrespro.ru
9. 
https://www.postgresql.org/message-id/CAMEv5_vg4aJOoUC74XJm%2B5B7%2BTF1nT-Yhtg%2BawtBOESXG5Grfg%40mail.gmail.com
10. 
https://www.postgresql.org/message-id/9c897136-4755-dcfc-2d24-b12bcfe4467f%40sigaev.ru
11. 
https://www.postgresql.org/message-id/CA%2BTgmoZv9f1s797tihx-zXQN4AE4ZFBV5C0K%3DzngbgNu3xNNkg%40mail.gmail.com
12. 
https://www.postgresql.org/message-id/20220312024652.lvgehszwke4hhove%40alap3.anarazel.de

--
Regards,
Alexander Korotkov

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,

The attached v40 patch is a fresh rebase on master branch to actualize the
state before the upcoming commitfest.
Nothing has changed beyond rebasing.

And just for convenience, here is a link to the exact message of the thread
describing the current approach:
https://www.postgresql.org/message-id/CAMEv5_vg4aJOoUC74XJm%2B5B7%2BTF1nT-Yhtg%2BawtBOESXG5Grfg%40mail.gmail.com

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Wed, Mar 22, 2023 at 10:38 PM Daniel Gustafsson  wrote:

> > On 22 Mar 2023, at 18:54, Robert Haas  wrote:
>
> > Basically, I think 0001 is a good idea -- I'm much more nervous about
> > 0002. I think we should get 0001 polished up and committed.
>
> Correct me if I'm wrong, but I believe you commented on v27-0001 of the
> login
> event trigger patch series?  Sorry about the confusion if so, this is a
> very
> old and lengthy thread with many twists and turns.  This series was closed
> downthread when the original authors of login EVT left, and the 0001 GUC
> patch
> extracted into its own thread.  That patch now lives at:
>
> https://commitfest.postgresql.org/42/4013/
>
> This thread was then later revived by Mikhail Gribkov but without 0001
> instead
> referring to the above patch for that part.
>
> The new patch for 0001 is quite different, and I welcome your eyes on that
> since I agree with you that it would be a good idea.
>
> --
> Daniel Gustafsson
>
>
>
>


v40-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Daniel Gustafsson]

> On 22 Mar 2023, at 18:54, Robert Haas  wrote:

> Basically, I think 0001 is a good idea -- I'm much more nervous about
> 0002. I think we should get 0001 polished up and committed.

Correct me if I'm wrong, but I believe you commented on v27-0001 of the login
event trigger patch series?  Sorry about the confusion if so, this is a very
old and lengthy thread with many twists and turns.  This series was closed
downthread when the original authors of login EVT left, and the 0001 GUC patch
extracted into its own thread.  That patch now lives at:

https://commitfest.postgresql.org/42/4013/

This thread was then later revived by Mikhail Gribkov but without 0001 instead
referring to the above patch for that part.

The new patch for 0001 is quite different, and I welcome your eyes on that
since I agree with you that it would be a good idea.

--
Daniel Gustafsson

Re: On login trigger: take three

[Robert Haas]

On Tue, Mar 15, 2022 at 4:52 PM Daniel Gustafsson  wrote:
> Yeah, that was the previously posted v25 from the author (who adopted it from
> the original author).  I took the liberty to quickly poke at the review
> comments you had left as well as the ones that I had found to try and progress
> the patch.  0001 should really go in it's own thread though to not hide it 
> from
> anyone interested who isn't looking at this thread.

Some comments on 0001:

- In general, I think we should prefer to phrase options in terms of
what is done, rather than what is not done. For instance, the
corresponding GUC for row-level security is row_security={on|off}, not
ignore_row_security.

- I think it's odd that the GUC in question doesn't accept true and
false and our usual synonyms for those values. I suggest that it
should, even if we want to add more possible values later.

- "ignoreing" is mispleled. So is gux-ignore-event-trigger. "Even
triggers" -> "Event triggers".

- Perhaps the documentation for the GUC should mention that the GUC is
not relevant in single-user mode because event triggers don't fire
then anyway.

- "Disable event triggers during the session." isn't a very good
description because there is in theory nothing to prevent this from
being set in postgresql.conf.

Basically, I think 0001 is a good idea -- I'm much more nervous about
0002. I think we should get 0001 polished up and committed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Gregory,

Thanks for the note. The problem was that the patch was not aware of
yesterday Tom Lane's changes in the test.
It's fixed now: the attached v39 patch contains the updated version along
with the freshest rebase on master branch.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Wed, Mar 15, 2023 at 8:45 PM Gregory Stark (as CFM) 
wrote:

> It looks like the patch is failing a test by not dumping
> login_event_trigger_func? I'm guessing there's a race condition in the
> test but I don't know. I also don't see the tmp_test_jI6t output file
> being preserved in the artifacts anywhere :(
>
> https://cirrus-ci.com/task/6391161871400960?logs=test_world#L2671
>
>
> [16:16:48.594] # Looks like you failed 1 test of 10350.
>
>
> # Running: pg_dump --no-sync
>
> --file=/tmp/cirrus-ci-build/src/bin/pg_dump/tmp_check/tmp_test_jI6t/only_dump_measurement.sql
> --table-and-children=dump_test.measurement --lock-wait-timeout=18
> postgres
> [16:16:27.027](0.154s) ok 6765 - only_dump_measurement: pg_dump runs
> .
> [16:16:27.035](0.000s) not ok 6870 - only_dump_measurement: should
> dump CREATE FUNCTION dump_test.login_event_trigger_func
> [16:16:27.035](0.000s)
> [16:16:27.035](0.000s) #   Failed test 'only_dump_measurement: should
> dump CREATE FUNCTION dump_test.login_event_trigger_func'
> #   at t/002_pg_dump.pl line 4761.
> .
> [16:16:48.594] +++ tap check in src/bin/pg_dump +++
> [16:16:48.594] [16:16:05] t/001_basic.pl  ok 612 ms (
> 0.01 usr 0.00 sys + 0.24 cusr 0.26 csys = 0.51 CPU)
> [16:16:48.594]
> [16:16:48.594] # Failed test 'only_dump_measurement: should dump
> CREATE FUNCTION dump_test.login_event_trigger_func'
> [16:16:48.594] # at t/002_pg_dump.pl line 4761.
> [16:16:48.594] # Review only_dump_measurement results in
> /tmp/cirrus-ci-build/src/bin/pg_dump/tmp_check/tmp_test_jI6t
>


v39-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Gregory Stark (as CFM)]

It looks like the patch is failing a test by not dumping
login_event_trigger_func? I'm guessing there's a race condition in the
test but I don't know. I also don't see the tmp_test_jI6t output file
being preserved in the artifacts anywhere :(

https://cirrus-ci.com/task/6391161871400960?logs=test_world#L2671


[16:16:48.594] # Looks like you failed 1 test of 10350.


# Running: pg_dump --no-sync
--file=/tmp/cirrus-ci-build/src/bin/pg_dump/tmp_check/tmp_test_jI6t/only_dump_measurement.sql
--table-and-children=dump_test.measurement --lock-wait-timeout=18
postgres
[16:16:27.027](0.154s) ok 6765 - only_dump_measurement: pg_dump runs
.
[16:16:27.035](0.000s) not ok 6870 - only_dump_measurement: should
dump CREATE FUNCTION dump_test.login_event_trigger_func
[16:16:27.035](0.000s)
[16:16:27.035](0.000s) #   Failed test 'only_dump_measurement: should
dump CREATE FUNCTION dump_test.login_event_trigger_func'
#   at t/002_pg_dump.pl line 4761.
.
[16:16:48.594] +++ tap check in src/bin/pg_dump +++
[16:16:48.594] [16:16:05] t/001_basic.pl  ok 612 ms (
0.01 usr 0.00 sys + 0.24 cusr 0.26 csys = 0.51 CPU)
[16:16:48.594]
[16:16:48.594] # Failed test 'only_dump_measurement: should dump
CREATE FUNCTION dump_test.login_event_trigger_func'
[16:16:48.594] # at t/002_pg_dump.pl line 4761.
[16:16:48.594] # Review only_dump_measurement results in
/tmp/cirrus-ci-build/src/bin/pg_dump/tmp_check/tmp_test_jI6t

Re: On login trigger: take three

[Daniel Gustafsson]

> On 6 Mar 2023, at 23:10, Andres Freund  wrote:
> 
> Hi,
> 
> On 2023-03-06 15:55:01 -0500, Gregory Stark (as CFM) wrote:
>> It looks like Daniel Gustafsson, Andres, and Tom have all weighed in
>> on this patch with at least a neutral comment (+-0 from Andres :)
>> 
>> It looks like the main concern was breaking physical replicas and that
>> there was consensus that as long as single-user mode worked that it
>> was ok?
> 
> I don't think it's OK with just single user mode as an scape hatch. We need
> the GUC that was discussed as part of the thread (and I think there's a patch
> for that too).

This is the patch which originated from this thread:

https://commitfest.postgresql.org/42/4013/

--
Daniel Gustafsson

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2023-03-06 15:55:01 -0500, Gregory Stark (as CFM) wrote:
> It looks like Daniel Gustafsson, Andres, and Tom have all weighed in
> on this patch with at least a neutral comment (+-0 from Andres :)
> 
> It looks like the main concern was breaking physical replicas and that
> there was consensus that as long as single-user mode worked that it
> was ok?

I don't think it's OK with just single user mode as an scape hatch. We need
the GUC that was discussed as part of the thread (and I think there's a patch
for that too).

Greetings,

Andres Freund

Re: On login trigger: take three

[Daniel Gustafsson]

> On 6 Mar 2023, at 21:55, Gregory Stark (as CFM)  wrote:
> 
> It looks like Daniel Gustafsson, Andres, and Tom have all weighed in
> on this patch with at least a neutral comment (+-0 from Andres :)

I think the concept of a login event trigger has merits, even though it's kind
of a niche use case.

> It looks like the main concern was breaking physical replicas and that
> there was consensus that as long as single-user mode worked that it
> was ok?

Having a way to not rely on single-user mode and not causing an unacceptable
performance hit (which judging by recent benchmarks might not be an issue?).  I
still intend to revisit this and I hope to get to it during this CF.

--
Daniel Gustafsson

Re: On login trigger: take three

[Gregory Stark (as CFM)]

It looks like Daniel Gustafsson, Andres, and Tom have all weighed in
on this patch with at least a neutral comment (+-0 from Andres :)

It looks like the main concern was breaking physical replicas and that
there was consensus that as long as single-user mode worked that it
was ok?

So maybe it's time after 2 1/2 years to get this one committed?

-- 
Gregory Stark
As Commitfest Manager

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,

The attached v38 patch is a fresh rebase on master branch.
Nothing has changed beyond rebasing.

And just for convenience, here is a link to the exact message of the thread
describing the current approach:
https://www.postgresql.org/message-id/CAMEv5_vg4aJOoUC74XJm%2B5B7%2BTF1nT-Yhtg%2BawtBOESXG5Grfg%40mail.gmail.com

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick


> Thank you
>
> marked as ready for committer
>
> Regards
>
> Pavel
>
>
>>


v38-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Pavel Stehule]

pá 20. 1. 2023 v 19:46 odesílatel Mikhail Gribkov 
napsal:

>   Hi Pavel,
>
> On Mon, Jan 16, 2023 at 9:10 AM Pavel Stehule 
> wrote:
>
>>
>>
>> ne 15. 1. 2023 v 7:32 odesílatel Pavel Stehule 
>> napsal:
>>
>>> Hi
>>>
>>>
 On Thu, Jan 12, 2023 at 9:51 AM Pavel Stehule 
 wrote:

> Hi
>
> I checked this patch and it looks well. All tests passed. Together
> with https://commitfest.postgresql.org/41/4013/ it can be a good
> feature.
>
> I re-tested impact on performance and for the worst case looks like
> less than 1% (0.8%). I think it is acceptable. Tested pgbench scenario
> "SELECT 1"
>
> pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres
>
> 733 tps (master), 727 tps (patched).
>
> I think raising an exception inside should be better tested - not it
> is only in 001_stream_rep.pl - generally more tests are welcome -
> there are no tested handling exceptions.
>

>>> Thank you
>>>
>>> check-world passed without problems
>>> build doc passed without problems
>>> I think so tests are now enough
>>>
>>> I'll mark this patch as ready for committer
>>>
>>
>> Unfortunately, I  forgot one important point. There are not any tests
>> related to backup.
>>
>> I miss pg_dump related tests.
>>
>> I mark this patch as waiting on the author.
>>
>>
>
> Thanks for noticing this.
> I have added sections to pg_dump tests. Attached v37 patch contains these
> additions along with the fresh rebase on master.
>

Thank you

marked as ready for committer

Regards

Pavel


>
> --
>  best regards,
> Mikhail A. Gribkov
>
> e-mail: youzh...@gmail.com
> *http://www.flickr.com/photos/youzhick/albums
> *
> http://www.strava.com/athletes/5085772
> phone: +7(916)604-71-12
> Telegram: @youzhick
>
>
>
>

Re: On login trigger: take three

[Mikhail Gribkov]

  Hi Pavel,

On Mon, Jan 16, 2023 at 9:10 AM Pavel Stehule 
wrote:

>
>
> ne 15. 1. 2023 v 7:32 odesílatel Pavel Stehule 
> napsal:
>
>> Hi
>>
>>
>>> On Thu, Jan 12, 2023 at 9:51 AM Pavel Stehule 
>>> wrote:
>>>
 Hi

 I checked this patch and it looks well. All tests passed. Together with
 https://commitfest.postgresql.org/41/4013/ it can be a good feature.

 I re-tested impact on performance and for the worst case looks like
 less than 1% (0.8%). I think it is acceptable. Tested pgbench scenario
 "SELECT 1"

 pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres

 733 tps (master), 727 tps (patched).

 I think raising an exception inside should be better tested - not it is
 only in 001_stream_rep.pl - generally more tests are welcome - there
 are no tested handling exceptions.

>>>
>> Thank you
>>
>> check-world passed without problems
>> build doc passed without problems
>> I think so tests are now enough
>>
>> I'll mark this patch as ready for committer
>>
>
> Unfortunately, I  forgot one important point. There are not any tests
> related to backup.
>
> I miss pg_dump related tests.
>
> I mark this patch as waiting on the author.
>
>

Thanks for noticing this.
I have added sections to pg_dump tests. Attached v37 patch contains these
additions along with the fresh rebase on master.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick






v37-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Pavel Stehule]

ne 15. 1. 2023 v 7:32 odesílatel Pavel Stehule 
napsal:

> Hi
>
>
>> On Thu, Jan 12, 2023 at 9:51 AM Pavel Stehule 
>> wrote:
>>
>>> Hi
>>>
>>> I checked this patch and it looks well. All tests passed. Together with
>>> https://commitfest.postgresql.org/41/4013/ it can be a good feature.
>>>
>>> I re-tested impact on performance and for the worst case looks like less
>>> than 1% (0.8%). I think it is acceptable. Tested pgbench scenario "SELECT
>>> 1"
>>>
>>> pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres
>>>
>>> 733 tps (master), 727 tps (patched).
>>>
>>> I think raising an exception inside should be better tested - not it is
>>> only in 001_stream_rep.pl - generally more tests are welcome - there
>>> are no tested handling exceptions.
>>>
>>
> Thank you
>
> check-world passed without problems
> build doc passed without problems
> I think so tests are now enough
>
> I'll mark this patch as ready for committer
>

Unfortunately, I  forgot one important point. There are not any tests
related to backup.

I miss pg_dump related tests.

I mark this patch as waiting on the author.

Regards

Pavel



> Regards
>
> Pavel
>
>
>
>
>>
>>> Regards
>>>
>>> Pavel
>>>
>>>

Re: On login trigger: take three

[Pavel Stehule]

Hi


so 14. 1. 2023 v 22:56 odesílatel Mikhail Gribkov 
napsal:

> Hi Pavel,
>
> Thanks for pointing out the tests. I completely agree that using an
> exception inside on-login trigger should be tested. It cannot be done via
> regular *.sql/*.out regress tests, thus I have added another perl test to
> authentication group doing this.
> Attached v36 patch contains this test along with the fresh rebase on
> master.
>
> --
>  best regards,
> Mikhail A. Gribkov
>
> e-mail: youzh...@gmail.com
> *http://www.flickr.com/photos/youzhick/albums
> *
> http://www.strava.com/athletes/5085772
> phone: +7(916)604-71-12
> Telegram: @youzhick
>
>
>
> On Thu, Jan 12, 2023 at 9:51 AM Pavel Stehule 
> wrote:
>
>> Hi
>>
>> I checked this patch and it looks well. All tests passed. Together with
>> https://commitfest.postgresql.org/41/4013/ it can be a good feature.
>>
>> I re-tested impact on performance and for the worst case looks like less
>> than 1% (0.8%). I think it is acceptable. Tested pgbench scenario "SELECT
>> 1"
>>
>> pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres
>>
>> 733 tps (master), 727 tps (patched).
>>
>> I think raising an exception inside should be better tested - not it is
>> only in 001_stream_rep.pl - generally more tests are welcome - there are
>> no tested handling exceptions.
>>
>
Thank you

check-world passed without problems
build doc passed without problems
I think so tests are now enough

I'll mark this patch as ready for committer

Regards

Pavel




>
>> Regards
>>
>> Pavel
>>
>>

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Pavel,

Thanks for pointing out the tests. I completely agree that using an
exception inside on-login trigger should be tested. It cannot be done via
regular *.sql/*.out regress tests, thus I have added another perl test to
authentication group doing this.
Attached v36 patch contains this test along with the fresh rebase on master.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Thu, Jan 12, 2023 at 9:51 AM Pavel Stehule 
wrote:

> Hi
>
> I checked this patch and it looks well. All tests passed. Together with
> https://commitfest.postgresql.org/41/4013/ it can be a good feature.
>
> I re-tested impact on performance and for the worst case looks like less
> than 1% (0.8%). I think it is acceptable. Tested pgbench scenario "SELECT
> 1"
>
> pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres
>
> 733 tps (master), 727 tps (patched).
>
> I think raising an exception inside should be better tested - not it is
> only in 001_stream_rep.pl - generally more tests are welcome - there are
> no tested handling exceptions.
>
> Regards
>
> Pavel
>
>


v36-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Pavel Stehule]

Hi


po 19. 12. 2022 v 10:40 odesílatel Mikhail Gribkov 
napsal:

> Hi Ted,
>
> > bq. in to the system
> > 'in to' -> into
> > bq. Any bugs in a trigger procedure
> > Any bugs -> Any bug
>
> Thanks!  Fixed typos in the attached v35.
>
> >   +   if (event == EVT_Login)
> >   +   dbgtag = CMDTAG_LOGIN;
> >   +   else
> >   +   dbgtag = CreateCommandTag(parsetree);
> > The same snippet appears more than once. It seems CMDTAG_LOGIN handling
> can be moved into `CreateCommandTag`.
>
> It appears twice in fact, both cases are nearby: in the main code and
> under the assert-checking ifdef.
> Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag
> function signature and ideology. CreateCommandTag finds a tag based on the
> command parse tree, while login event is a specific case when we do not
> have any command and the parse tree is NULL. Changing CreateCommandTag
> signature for these two calls looks a little bit overkill as it would lead
> to changing lots of other places the function is called from.
> We could move this snippet to a separate function, but here are the
> similar concerns I think: it would make sense for a more common or a more
> complex snippet, but not for two cases of if-else one-liners.
>

I checked this patch and it looks well. All tests passed. Together with
https://commitfest.postgresql.org/41/4013/ it can be a good feature.

I re-tested impact on performance and for the worst case looks like less
than 1% (0.8%). I think it is acceptable. Tested pgbench scenario "SELECT
1"

pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres

733 tps (master), 727 tps (patched).

I think raising an exception inside should be better tested - not it is
only in 001_stream_rep.pl - generally more tests are welcome - there are no
tested handling exceptions.

Regards

Pavel


> --
>  best regards,
> Mikhail A. Gribkov
>
> e-mail: youzh...@gmail.com
> *http://www.flickr.com/photos/youzhick/albums
> *
> http://www.strava.com/athletes/5085772
> phone: +7(916)604-71-12
> Telegram: @youzhick
>
>
>
> On Sat, Dec 17, 2022 at 3:29 PM Ted Yu  wrote:
>
>>
>>
>> On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov 
>> wrote:
>>
>>> Hi Nikita,
>>>
>>> > Mikhail, I've checked the patch and previous discussion,
>>> > the condition mentioned earlier is still actual:
>>>
>>> Thanks for pointing this out! My bad, I forgot to fix the documentation
>>> example.
>>> Attached v34 has this issue fixed, as well as a couple other problems
>>> with the example code.
>>>
>>> --
>>>  best regards,
>>> Mikhail A. Gribkov
>>>
>> Hi,
>>
>> bq. in to the system
>>
>> 'in to' -> into
>>
>> bq. Any bugs in a trigger procedure
>>
>> Any bugs -> Any bug
>>
>> +   if (event == EVT_Login)
>> +   dbgtag = CMDTAG_LOGIN;
>> +   else
>> +   dbgtag = CreateCommandTag(parsetree);
>>
>> The same snippet appears more than once. It seems CMDTAG_LOGIN handling
>> can be moved into `CreateCommandTag`.
>>
>> Cheers
>>
>

Re: On login trigger: take three

[Ted Yu]

On Mon, Dec 19, 2022 at 1:40 AM Mikhail Gribkov  wrote:

> Hi Ted,
>
> > bq. in to the system
> > 'in to' -> into
> > bq. Any bugs in a trigger procedure
> > Any bugs -> Any bug
>
> Thanks!  Fixed typos in the attached v35.
>
> >   +   if (event == EVT_Login)
> >   +   dbgtag = CMDTAG_LOGIN;
> >   +   else
> >   +   dbgtag = CreateCommandTag(parsetree);
> > The same snippet appears more than once. It seems CMDTAG_LOGIN handling
> can be moved into `CreateCommandTag`.
>
> It appears twice in fact, both cases are nearby: in the main code and
> under the assert-checking ifdef.
> Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag
> function signature and ideology. CreateCommandTag finds a tag based on the
> command parse tree, while login event is a specific case when we do not
> have any command and the parse tree is NULL. Changing CreateCommandTag
> signature for these two calls looks a little bit overkill as it would lead
> to changing lots of other places the function is called from.
> We could move this snippet to a separate function, but here are the
> similar concerns I think: it would make sense for a more common or a more
> complex snippet, but not for two cases of if-else one-liners.
>
> --
>  best regards,
> Mikhail A. Gribkov
>
> e-mail: youzh...@gmail.com
> *http://www.flickr.com/photos/youzhick/albums
> *
> http://www.strava.com/athletes/5085772
> phone: +7(916)604-71-12
> Telegram: @youzhick
>
> Hi, Mikhail:
Thanks for the explanation.
It is Okay to keep the current formation of CMDTAG_LOGIN handling.

Cheers

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Ted,

> bq. in to the system
> 'in to' -> into
> bq. Any bugs in a trigger procedure
> Any bugs -> Any bug

Thanks!  Fixed typos in the attached v35.

>   +   if (event == EVT_Login)
>   +   dbgtag = CMDTAG_LOGIN;
>   +   else
>   +   dbgtag = CreateCommandTag(parsetree);
> The same snippet appears more than once. It seems CMDTAG_LOGIN handling
can be moved into `CreateCommandTag`.

It appears twice in fact, both cases are nearby: in the main code and under
the assert-checking ifdef.
Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag
function signature and ideology. CreateCommandTag finds a tag based on the
command parse tree, while login event is a specific case when we do not
have any command and the parse tree is NULL. Changing CreateCommandTag
signature for these two calls looks a little bit overkill as it would lead
to changing lots of other places the function is called from.
We could move this snippet to a separate function, but here are the similar
concerns I think: it would make sense for a more common or a more complex
snippet, but not for two cases of if-else one-liners.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Sat, Dec 17, 2022 at 3:29 PM Ted Yu  wrote:

>
>
> On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov 
> wrote:
>
>> Hi Nikita,
>>
>> > Mikhail, I've checked the patch and previous discussion,
>> > the condition mentioned earlier is still actual:
>>
>> Thanks for pointing this out! My bad, I forgot to fix the documentation
>> example.
>> Attached v34 has this issue fixed, as well as a couple other problems
>> with the example code.
>>
>> --
>>  best regards,
>> Mikhail A. Gribkov
>>
> Hi,
>
> bq. in to the system
>
> 'in to' -> into
>
> bq. Any bugs in a trigger procedure
>
> Any bugs -> Any bug
>
> +   if (event == EVT_Login)
> +   dbgtag = CMDTAG_LOGIN;
> +   else
> +   dbgtag = CreateCommandTag(parsetree);
>
> The same snippet appears more than once. It seems CMDTAG_LOGIN handling
> can be moved into `CreateCommandTag`.
>
> Cheers
>


v35-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Ted Yu]

On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov  wrote:

> Hi Nikita,
>
> > Mikhail, I've checked the patch and previous discussion,
> > the condition mentioned earlier is still actual:
>
> Thanks for pointing this out! My bad, I forgot to fix the documentation
> example.
> Attached v34 has this issue fixed, as well as a couple other problems with
> the example code.
>
> --
>  best regards,
> Mikhail A. Gribkov
>
Hi,

bq. in to the system

'in to' -> into

bq. Any bugs in a trigger procedure

Any bugs -> Any bug

+   if (event == EVT_Login)
+   dbgtag = CMDTAG_LOGIN;
+   else
+   dbgtag = CreateCommandTag(parsetree);

The same snippet appears more than once. It seems CMDTAG_LOGIN handling can
be moved into `CreateCommandTag`.

Cheers

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Nikita,

> Mikhail, I've checked the patch and previous discussion,
> the condition mentioned earlier is still actual:

Thanks for pointing this out! My bad, I forgot to fix the documentation
example.
Attached v34 has this issue fixed, as well as a couple other problems with
the example code.

--
 best regards,
Mikhail A. Gribkov


v34-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Nikita Malakhov]

Hi,

Mikhail, I've checked the patch and previous discussion,
the condition mentioned earlier is still actual:
>The example trigger from the documentation

+DECLARE

+  hour integer = EXTRACT('hour' FROM current_time);

+  rec boolean;

+BEGIN

+-- 1. Forbid logging in between 2AM and 4AM.

+IF hour BETWEEN 2 AND 4 THEN

+  RAISE EXCEPTION 'Login forbidden';

+END IF;


>can be bypassed with PGOPTIONS='-c timezone=...'. Probably this is
>nothing new and concerns any SECURITY DEFINER function, but still...

along with
+IF hour BETWEEN 8 AND 20 THEN
It seems to be a minor security issue, so just in case you haven't noticed
it.

On Fri, Dec 16, 2022 at 9:14 PM Mikhail Gribkov  wrote:

> Hi hackers,
> Attached v33 is a new rebase of the flagless version of the patch.  As
> there were no objections at first glance, I’ll try to post it to the
> upcoming commitfest, thus the brief recap of all the patch details is below.
>
> v33-On_client_login_event_trigger
> The patch introduces a trigger on login event, allowing to fire some
> actions right on the user connection. This can be useful for  logging or
> connection check purposes as well as for some personalization of the
> environment. Usage details are described in the documentation included, but
> shortly usage is the same as for other triggers: create function returning
> event_trigger and then create event trigger on login event.
>
> The patch is prepared to be applied to the master branch and tested when
> applied after e52f8b301ed54aac5162b185b43f5f1e44b6b17e commit by Thomas
> Munro (Date:   Fri Dec 16 17:36:22 2022 +1300).
> Regression tests and documentation included.
>
> A couple of words about what and why I changed compared to the previous
> author's version.
> First, the (en/dis)abling GUC was removed from the patch because
> ideologically it is a separate feature and nowadays it’s  discussed and
>  supported in a separate thread by Daniel Gustaffson:
>
> https://www.postgresql.org/message-id/flat/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9%40yesql.se
> Second, I have removed the dathasloginevt flag. The flag was initially
> added to the patch for performance reasons and it did the job, although it
> was quite a clumsy construct causing tons of bugs and could potentially
> lead to tons more during further PostgreSQL evolution. I have removed the
> flag and found that the performance drop is not that significant.
>
> And this is an aspect I should describe in more detail.
> The possible performance drop is expected as an increased time used to
> login a user NOT using a login trigger.
> First of all, the method of performance check:
> echo 'select 1;' > ./tst.sql
> pgbench -n -C -T3 -f tst.sql -U postgres postgres
> The output value "average connection time" is the one I use to compare
> performance.
> Now, what are the results.
> * master branch: 0.641 ms
> * patched version: 0.644 ms
> No significant difference here and it is just what was expected. Based on
> the patch design the performance drop can be expected when there are lots
> of event triggers created, but not the login one. Thus I have created 1000
> drop triggers (the script for creating triggers is attached too) in the
> database and repeated the test:
> * master branch: 0.646 ms
> * patched version: 0.754 ms
> For 2000 triggers the patched version connection time is further increased
> to 0.862. Thus we have a login time rise in about 16.5% per 1000 event
> triggers in the database. It is a statistically noticeable value, still I
> don’t think it’s a critical one we should be afraid of.
> N.B. The exact values of the login times  slightly differ from the ones I
> posted in the previous email. Well, that’s the repeatability level we have.
> This convinces me even more that the observed level of performance drop is
> acceptable.
> --
>  best regards,
> Mikhail A. Gribkov
>
> e-mail: youzh...@gmail.com
> *http://www.flickr.com/photos/youzhick/albums
> *
> http://www.strava.com/athletes/5085772
> phone: +7(916)604-71-12
> Telegram: @youzhick
>


-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,
Attached v33 is a new rebase of the flagless version of the patch.  As
there were no objections at first glance, I’ll try to post it to the
upcoming commitfest, thus the brief recap of all the patch details is below.

v33-On_client_login_event_trigger
The patch introduces a trigger on login event, allowing to fire some
actions right on the user connection. This can be useful for  logging or
connection check purposes as well as for some personalization of the
environment. Usage details are described in the documentation included, but
shortly usage is the same as for other triggers: create function returning
event_trigger and then create event trigger on login event.

The patch is prepared to be applied to the master branch and tested when
applied after e52f8b301ed54aac5162b185b43f5f1e44b6b17e commit by Thomas
Munro (Date:   Fri Dec 16 17:36:22 2022 +1300).
Regression tests and documentation included.

A couple of words about what and why I changed compared to the previous
author's version.
First, the (en/dis)abling GUC was removed from the patch because
ideologically it is a separate feature and nowadays it’s  discussed and
 supported in a separate thread by Daniel Gustaffson:
https://www.postgresql.org/message-id/flat/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9%40yesql.se
Second, I have removed the dathasloginevt flag. The flag was initially
added to the patch for performance reasons and it did the job, although it
was quite a clumsy construct causing tons of bugs and could potentially
lead to tons more during further PostgreSQL evolution. I have removed the
flag and found that the performance drop is not that significant.

And this is an aspect I should describe in more detail.
The possible performance drop is expected as an increased time used to
login a user NOT using a login trigger.
First of all, the method of performance check:
echo 'select 1;' > ./tst.sql
pgbench -n -C -T3 -f tst.sql -U postgres postgres
The output value "average connection time" is the one I use to compare
performance.
Now, what are the results.
* master branch: 0.641 ms
* patched version: 0.644 ms
No significant difference here and it is just what was expected. Based on
the patch design the performance drop can be expected when there are lots
of event triggers created, but not the login one. Thus I have created 1000
drop triggers (the script for creating triggers is attached too) in the
database and repeated the test:
* master branch: 0.646 ms
* patched version: 0.754 ms
For 2000 triggers the patched version connection time is further increased
to 0.862. Thus we have a login time rise in about 16.5% per 1000 event
triggers in the database. It is a statistically noticeable value, still I
don’t think it’s a critical one we should be afraid of.
N.B. The exact values of the login times  slightly differ from the ones I
posted in the previous email. Well, that’s the repeatability level we have.
This convinces me even more that the observed level of performance drop is
acceptable.
--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick


v33-On_client_login_event_trigger.patch
Description: Binary data


create_1000_triggers.sql
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,



Since the original authors, as Daniel said,  seems to have retired from the
patch, I have allowed myself to continue the patch polishing.

Attached v32 includes fresh rebase and the following fixes:

- Copying dathasloginevt flag during DB creation from template;

- Restoring dathasloginevt flag after re-enabling a disabled trigger;

- Preserving dathasloginevt flag during not-running a trigger because of
some filters (running with 'session_replication_role=replica' for example);

- Checking tags during the trigger creation;

The (en/dis)abling GUC was removed from the patch by default because it is
now discussed and  supported in a separate thread by Daniel Gustaffson:


*https://www.postgresql.org/message-id/9140106e-f9bf-4d85-8fc8-f2d3c094a...@yesql.se*


Still the back-ported version of the Daniel’s patch is attached here too –
just for convenience.



While the above changes represent the straight work continue, there is
another version to consider. Most of the patch problems seem to originate
from the dathasloginevt flag support. There are lots of known problems
(partly fixed) and probably a lot more unfound. At the same time the flag
itself is not a critical element, but just a performance optimizer for
logging-in of users who are NOT using the on-login triggers.

I have made a simpler version without the flag and tried to compare the
performance. The results show that life without dathasloginevt is still
possible. When there is a small number of non-login event triggers, the
difference is barely noticeable indeed. To show the difference, I have
added 1000 sql_drop event triggers and used pgbench to continuously query
the database for “select 1”  with reconnects for 3 seconds. Here are the
results. For each version I recorded average connection time with 0 and
with 1000 event triggers:

With dathasloginevt flag: 0.609 ms VS 0.611 ms

No-flag version: 0.617 ms VS 0.727 ms

You can see that the difference is noticeable, but not that critical as we
can expect for 1000 triggers (while in a vast majority of real cases there
would be a much lesser number of triggers). I.e. the flagless version of
the patch introduces a huge reliability raise at the cost of a small
performance drop.

What do you think? Maybe it’s better to use the flagless version? Or even a
small performance drop is considered as unacceptable?



The attached files include the two versions of the patch: “v32” for the
updated version with flag and “v31_nf” – for the flagless version. Both
versions contain “0001“ file for the patch itself and “0002” for
back-ported controlling GUC.
--
 best regards,
Mikhail A. Gribkov



On Fri, Nov 4, 2022 at 3:58 AM Ian Lawrence Barwick 
wrote:

> 2022年11月4日(金) 5:23 Daniel Gustafsson :
> >
> > > On 20 Sep 2022, at 16:10, Daniel Gustafsson  wrote:
> >
> > > Since the original authors seems to have retired from the patch
> > > (I've only rebased it to try and help) I am inclined to mark it as
> returned
> > > with feedback.
> >
> > Doing so now since no updates have been posted for quite some time and
> holes
> > have been poked in the patch.
>
> I see it was still "Waiting on Author" so I went ahead and changed the
> status.
>
> > The GUC for temporarily disabling event triggers, to avoid the need for
> single-
> > user mode during troubleshooting, might however be of interest so I will
> break
> > that out and post to a new thread.
>
> For reference this is the new thread:
>
>
> https://www.postgresql.org/message-id/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9%40yesql.se
>
> Regards
>
> Ian Barwick
>
>
>


v32-0002-Add-GUC-for-temporarily-disabling-event-triggers.patch
Description: Binary data


v31_nf-0001-Add-support-event-triggers-on-authenticated-login-noflag.patch
Description: Binary data


v31_nf-0002-Add-GUC-for-temporarily-disabling-event-triggers.patch
Description: Binary data


v32-0001-Add-support-event-triggers-on-authenticated-login.patch
Description: Binary data

Re: On login trigger: take three

[Ian Lawrence Barwick]

2022年11月4日(金) 5:23 Daniel Gustafsson :
>
> > On 20 Sep 2022, at 16:10, Daniel Gustafsson  wrote:
>
> > Since the original authors seems to have retired from the patch
> > (I've only rebased it to try and help) I am inclined to mark it as returned
> > with feedback.
>
> Doing so now since no updates have been posted for quite some time and holes
> have been poked in the patch.

I see it was still "Waiting on Author" so I went ahead and changed the status.

> The GUC for temporarily disabling event triggers, to avoid the need for 
> single-
> user mode during troubleshooting, might however be of interest so I will break
> that out and post to a new thread.

For reference this is the new thread:

  
https://www.postgresql.org/message-id/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9%40yesql.se

Regards

Ian Barwick

Re: On login trigger: take three

[Daniel Gustafsson]

> On 20 Sep 2022, at 16:10, Daniel Gustafsson  wrote:

> Since the original authors seems to have retired from the patch
> (I've only rebased it to try and help) I am inclined to mark it as returned
> with feedback.

Doing so now since no updates have been posted for quite some time and holes
have been poked in the patch.

The GUC for temporarily disabling event triggers, to avoid the need for single-
user mode during troubleshooting, might however be of interest so I will break
that out and post to a new thread.

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[Sergey Shinderuk]

On 20.09.2022 17:10, Daniel Gustafsson wrote:
On 20 Sep 2022, at 15:43, Sergey Shinderuk  wrote: 
There is a race around setting and clearing of dathasloginevt.


Thanks for the report!  The whole dathasloginevt mechanism is IMO too clunky
and unelegant to go ahead with, I wouldn't be surprised if there are other bugs
lurking there.

Indeed.

CREATE DATABASE doesn't copy dathasloginevt from the template database.
ALTER EVENT TRIGGER ... ENABLE doesn't set dathasloginevt.

In both cases triggers are enabled according to \dy output, but don't 
fire. The admin may not notice it immediately.


--
Sergey Shinderukhttps://postgrespro.com/

Re: On login trigger: take three

[Daniel Gustafsson]

> On 20 Sep 2022, at 15:43, Sergey Shinderuk  wrote:
> 
> On 02.09.2022 18:36, Daniel Gustafsson wrote:
>> This had bitrotted a fair bit, attached is a rebase along with (mostly)
>> documentation fixes.  0001 adds a generic GUC for ignoring event triggers and
>> 0002 adds the login event with event trigger support, and hooks it up to the
>> GUC such that broken triggers wont require single-user mode.  Moving the CF
>> entry back to Needs Review.

> There is a race around setting and clearing of dathasloginevt.

Thanks for the report!  The whole dathasloginevt mechanism is IMO too clunky
and unelegant to go ahead with, I wouldn't be surprised if there are other bugs
lurking there.  Since the original authors seems to have retired from the patch
(I've only rebased it to try and help) I am inclined to mark it as returned
with feedback.

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[Sergey Shinderuk]

On 02.09.2022 18:36, Daniel Gustafsson wrote:

This had bitrotted a fair bit, attached is a rebase along with (mostly)
documentation fixes.  0001 adds a generic GUC for ignoring event triggers and
0002 adds the login event with event trigger support, and hooks it up to the
GUC such that broken triggers wont require single-user mode.  Moving the CF
entry back to Needs Review.



Hello!

There is a race around setting and clearing of dathasloginevt.

Steps to reproduce:

1. Create a trigger:

  CREATE FUNCTION on_login_proc() RETURNS event_trigger AS $$
  BEGIN
RAISE NOTICE 'You are welcome!';
  END;
  $$ LANGUAGE plpgsql;

  CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
on_login_proc();


2. Then drop it, but don't start new sessions:

  DROP EVENT TRIGGER on_login_trigger;

3. Create another trigger, but don't commit yet:

  BEGIN;
  CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
on_login_proc();


4. Start a new session. This clears dathasloginevt.

5. Commit the transaction:

  COMMIT;

Now we have a trigger, but it doesn't fire, because dathasloginevt=false.


If two sessions create triggers concurrently, one of them will fail.

Steps:

1. In the first session, start a transaction and create a trigger:

  BEGIN;
  CREATE EVENT TRIGGER on_login_trigger1 ON login EXECUTE PROCEDURE 
on_login_proc();


2. In the second session, create another trigger (this query blocks):

  CREATE EVENT TRIGGER on_login_trigger2 ON login EXECUTE PROCEDURE 
on_login_proc();


3. Commit in the first session:

  COMMIT;

The second session fails:

  postgres=# CREATE EVENT TRIGGER on_login_trigger2 ON login EXECUTE 
PROCEDURE on_login_proc();

  ERROR:  tuple concurrently updated



What else bothers me is that login triggers execute in an environment 
under user control and one has to be very careful. The example trigger 
from the documentation


+DECLARE

+  hour integer = EXTRACT('hour' FROM current_time);

+  rec boolean;

+BEGIN

+-- 1. Forbid logging in between 2AM and 4AM.

+IF hour BETWEEN 2 AND 4 THEN

+  RAISE EXCEPTION 'Login forbidden';

+END IF;


can be bypassed with PGOPTIONS='-c timezone=...'. Probably this is 
nothing new and concerns any SECURITY DEFINER function, but still...



Best regards,

--
Sergey Shinderukhttps://postgrespro.com/

Re: On login trigger: take three

[Zhihong Yu]

On Fri, Sep 2, 2022 at 8:37 AM Daniel Gustafsson  wrote:

> This had bitrotted a fair bit, attached is a rebase along with (mostly)
> documentation fixes.  0001 adds a generic GUC for ignoring event triggers
> and
> 0002 adds the login event with event trigger support, and hooks it up to
> the
> GUC such that broken triggers wont require single-user mode.  Moving the CF
> entry back to Needs Review.
>
> --
> Daniel Gustafsson   https://vmware.com/
>
> Hi,
For EventTriggerOnLogin():

+   LockSharedObject(DatabaseRelationId, MyDatabaseId, 0,
AccessExclusiveLock);
+
+   tuple = SearchSysCacheCopy1(DATABASEOID,
ObjectIdGetDatum(MyDatabaseId));
+
+   if (!HeapTupleIsValid(tuple))
+   elog(ERROR, "cache lookup failed for database %u",
MyDatabaseId);

Should the lock be taken after the check (HeapTupleIsValid) is done ?

Cheers

Re: On login trigger: take three

[Daniel Gustafsson]

This had bitrotted a fair bit, attached is a rebase along with (mostly)
documentation fixes.  0001 adds a generic GUC for ignoring event triggers and
0002 adds the login event with event trigger support, and hooks it up to the
GUC such that broken triggers wont require single-user mode.  Moving the CF
entry back to Needs Review.

--
Daniel Gustafsson   https://vmware.com/



v31-0002-Add-support-event-triggers-on-authenticated-logi.patch
Description: Binary data


v31-0001-Provide-a-GUC-for-temporarily-ignoring-event-tri.patch
Description: Binary data

Re: On login trigger: take three

[Daniel Gustafsson]

> On 1 Apr 2022, at 09:16, a.soko...@postgrespro.ru wrote:

> Please fix a typo in doc/src/sgml/event-trigger.sgml: "precvent"

Will do.  With that fixed I think this is ready and unless I find something on
another read through and test pass I hope to be able to push this before the CF
closes today.

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[a . sokolov]

Daniel Gustafsson писал 2022-03-30 16:48:

On 30 Mar 2022, at 13:21, Ivan Panchenko  wrote:
Maybe side-effects is a bit too general? Emitting a log message, 
rejecting a

login, setting some GUCs, etc are all side-effects too.
Something like this:


I've reworded the docs close to what you suggested here.

Also, please fix a typo in doc/src/sgml/ref/create_event_trigger.sgml 
:


Done.


Regarding the trigger function example:
It does not do anything if run on a standby. To show that it can do 
something on a standby to, I propose to move throwing the night 
exception to the beginning.


Good idea, done.

Finally, let me propose to append to the regression test the 
following:


Also a good idea, done.

--
Daniel Gustafsson   https://vmware.com/


Please fix a typo in doc/src/sgml/event-trigger.sgml: "precvent"

--
Andrey Sokolov
Postgres Professional: http://www.postgrespro.com

Re: On login trigger: take three

[Daniel Gustafsson]

> On 29 Mar 2022, at 00:40, Tom Lane  wrote:
> 
> Andres Freund  writes:
>> On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
>>> Do you think this potential foot-gun is scary enough to reject this patch?
>>> There are lots of creative ways to cause Nagios alerts from ones database, 
>>> but
>>> this has the potential to do so with a small bug in userland code.  Still, I
>>> kind of like the feature so I'm indecisive.
> 
>> It does seem like a huge footgun. But also kinda useful. So I'm really +-0.
> 
> An on-login trigger is *necessarily* a foot-gun; I don't see that this
> particular failure mode makes it any worse than it would be anyway.

Agreed.

> There has to be some not-too-difficult-to-use way to bypass a broken
> login trigger.  Assuming we are happy with the design for doing that,
> might as well accept the hazards.

The GUC in this patchset seems to be in line with what most in this thread have
preferred, and with that in place (and single-user mode which still works for
this) I think we have that covered.

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[Daniel Gustafsson]

> On 30 Mar 2022, at 13:21, Ivan Panchenko  wrote:
> Maybe side-effects is a bit too general? Emitting a log message, rejecting a
> login, setting some GUCs, etc are all side-effects too.
> Something like this:

I've reworded the docs close to what you suggested here.

> Also, please fix a typo in doc/src/sgml/ref/create_event_trigger.sgml :

Done.

> Regarding the trigger function example:
> It does not do anything if run on a standby. To show that it can do something 
> on a standby to, I propose to move throwing the night exception to the 
> beginning.

Good idea, done.

> Finally, let me propose to append to the regression test the following:

Also a good idea, done.

--
Daniel Gustafsson   https://vmware.com/



v30-0002-Add-a-new-login-event-and-login-event-trigger-su.patch
Description: Binary data


v30-0001-Provide-a-GUC-for-temporarily-ignoring-event-tri.patch
Description: Binary data

Re[2]: On login trigger: take three

[Ivan Panchenko]

 
Hi, 
>Tue, March 29, 2022, 0:31 +03:00 from Andres Freund :
> 
>Hi,
>
>On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
>> > On 28 Mar 2022, at 19:10, Andres Freund < and...@anarazel.de > wrote:
>> > On 2022-03-28 15:57:37 +0300,  a.soko...@postgrespro.ru wrote:
>>
>> >> + data initialization. It is vital that any event trigger using the
>> >> + login event checks whether or not the database is in
>> >> + recovery.
>> »
 
>> >> Does any trigger really have to contain a pg_is_in_recovery() call?
>> >
>> > Not *any* trigger, just any trigger that writes.
>>
>> Thats correct, the docs should be updated with something like the below I
>> reckon.
>>
>> It is vital that event trigger using the login event
>> which has side-effects checks whether or not the database is in recovery to
>> ensure they are not performing modifications to hot standby nodes.
>
>Maybe side-effects is a bit too general? Emitting a log message, rejecting a
>login, setting some GUCs, etc are all side-effects too.
Something like this:
 

    
      The login triggers fire also on standby servers.
      To keep them from becoming inaccessible, such triggers should
      avoid writing anything to the database when running on a standby.
      This can be achieved by checking 
pg_is_in_recovery(), see an example below.
    

> 
Also, please fix a typo in doc/src/sgml/ref/create_event_trigger.sgml :
 
- single-user mode and you'll be able to do that. Even triggers can also be
+ single-user mode and you'll be able to do that. Event triggers can also be
 
Regarding the trigger function example:
It does not do anything if run on a standby. To show that it can do something 
on a standby to, I propose to move throwing the night exception to the 
beginning.
So it will be:
 
CREATE OR REPLACE FUNCTION init_session() 
RETURNS event_trigger SECURITY DEFINER LANGUAGE plpgsql AS 
$$ 
DECLARE 
  hour integer = EXTRACT('hour' FROM current_time); 
  rec boolean;
BEGIN

-- 1) Forbid logging in late:
IF hour BETWEEN 2 AND 4 THEN
  RAISE EXCEPTION 'Login forbidden';  -- do not allow to login these hours
END IF;

-- The remaining stuff cannot be done on standbys,
-- so ensure the database is not in recovery
SELECT pg_is_in_recovery() INTO rec;
IF rec THEN
  RETURN;
END IF

-- 2) Assign some roles

IF hour BETWEEN 8 AND 20 THEN -- at daytime grant the day_worker role
  EXECUTE 'REVOKE night_worker FROM ' || quote_ident(session_user);
  EXECUTE 'GRANTday_worker TO '   || quote_ident(session_user);
ELSE  -- at other time grant the night_worker 
role
  EXECUTE 'REVOKE day_worker FROM ' || quote_ident(session_user);
  EXECUTE 'GRANT  night_worker TO ' || quote_ident(session_user);
END IF;

-- 3) Initialize some user session data

CREATE TEMP TABLE session_storage (x float, y integer);

-- 4) Log the connection time

INSERT INTO user_login_log VALUES (session_user, current_timestamp);

END;
$$;
Finally, let me propose to append to the regression test the following:
 
 
\c 
SELECT dathasloginevt FROM pg_database WHERE datname= :'DBNAME';
 
which should output:
 dathasloginevt 

 f
(1 row)
 
So we can check that removal of the event trigger resets this flag in 
pg_database. Note that reconnect (\c) is necessary here.
 
Regards,
Ivan
 
>
>> >> In this message
>> >> ( 
>> >> https://www.postgresql.org/message-id/20220312024652.lvgehszwke4hhove%40alap3.anarazel.de
>> >>  )
>> >> it was only about triggers on hot standby, which run not read-only queries
>> >
>> > The problem precisely is that the login triggers run on hot standby nodes, 
>> > and
>> > that if they do writes, you can't login anymore.
>>
>> Do you think this potential foot-gun is scary enough to reject this patch?
>> There are lots of creative ways to cause Nagios alerts from ones database, 
>> but
>> this has the potential to do so with a small bug in userland code. Still, I
>> kind of like the feature so I'm indecisive.
>
>It does seem like a huge footgun. But also kinda useful. So I'm really +-0.
>
>Greetings,
>
>Andres Freund
 

Re: On login trigger: take three

[Tom Lane]

Andres Freund  writes:
> On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
>> Do you think this potential foot-gun is scary enough to reject this patch?
>> There are lots of creative ways to cause Nagios alerts from ones database, 
>> but
>> this has the potential to do so with a small bug in userland code.  Still, I
>> kind of like the feature so I'm indecisive.

> It does seem like a huge footgun. But also kinda useful. So I'm really +-0.

An on-login trigger is *necessarily* a foot-gun; I don't see that this
particular failure mode makes it any worse than it would be anyway.
There has to be some not-too-difficult-to-use way to bypass a broken
login trigger.  Assuming we are happy with the design for doing that,
might as well accept the hazards.

regards, tom lane

Re: On login trigger: take three

[Daniel Gustafsson]

> On 28 Mar 2022, at 23:31, Andres Freund  wrote:
> 
> Hi,
> 
> On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
>>> On 28 Mar 2022, at 19:10, Andres Freund  wrote:
>>> On 2022-03-28 15:57:37 +0300, a.soko...@postgrespro.ru wrote:
>> 
 +data initialization. It is vital that any event trigger using the
 +login event checks whether or not the database is 
 in
 +recovery.
 
 Does any trigger really have to contain a pg_is_in_recovery() call?
>>> 
>>> Not *any* trigger, just any trigger that writes.
>> 
>> Thats correct, the docs should be updated with something like the below I
>> reckon.
>> 
>>It is vital that event trigger using the login event
>>which has side-effects checks whether or not the database is in recovery 
>> to
>>ensure they are not performing modifications to hot standby nodes.
> 
> Maybe side-effects is a bit too general? Emitting a log message, rejecting a
> login, setting some GUCs, etc are all side-effects too.

Good point, it needs to say that modifications that cause WAL to be generated
are prohibited, but in a more user-friendly readable way.  Perhaps in a big red
warning box.

 In this message
 (https://www.postgresql.org/message-id/20220312024652.lvgehszwke4hhove%40alap3.anarazel.de)
 it was only about triggers on hot standby, which run not read-only queries
>>> 
>>> The problem precisely is that the login triggers run on hot standby nodes, 
>>> and
>>> that if they do writes, you can't login anymore.
>> 
>> Do you think this potential foot-gun is scary enough to reject this patch?
>> There are lots of creative ways to cause Nagios alerts from ones database, 
>> but
>> this has the potential to do so with a small bug in userland code.  Still, I
>> kind of like the feature so I'm indecisive.
> 
> It does seem like a huge footgun. But also kinda useful. So I'm really +-0.

Looks like we are in agreement here.  I'm going to go over it again and sleep
on it some more before the deadline hits.

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
> > On 28 Mar 2022, at 19:10, Andres Freund  wrote:
> > On 2022-03-28 15:57:37 +0300, a.soko...@postgrespro.ru wrote:
> 
> >> +data initialization. It is vital that any event trigger using the
> >> +login event checks whether or not the database is 
> >> in
> >> +recovery.
> >> 
> >> Does any trigger really have to contain a pg_is_in_recovery() call?
> > 
> > Not *any* trigger, just any trigger that writes.
> 
> Thats correct, the docs should be updated with something like the below I
> reckon.
> 
> It is vital that event trigger using the login event
> which has side-effects checks whether or not the database is in recovery 
> to
> ensure they are not performing modifications to hot standby nodes.

Maybe side-effects is a bit too general? Emitting a log message, rejecting a
login, setting some GUCs, etc are all side-effects too.


> >> In this message
> >> (https://www.postgresql.org/message-id/20220312024652.lvgehszwke4hhove%40alap3.anarazel.de)
> >> it was only about triggers on hot standby, which run not read-only queries
> > 
> > The problem precisely is that the login triggers run on hot standby nodes, 
> > and
> > that if they do writes, you can't login anymore.
> 
> Do you think this potential foot-gun is scary enough to reject this patch?
> There are lots of creative ways to cause Nagios alerts from ones database, but
> this has the potential to do so with a small bug in userland code.  Still, I
> kind of like the feature so I'm indecisive.

It does seem like a huge footgun. But also kinda useful. So I'm really +-0.

Greetings,

Andres Freund

Re: On login trigger: take three

[Daniel Gustafsson]

> On 28 Mar 2022, at 19:10, Andres Freund  wrote:
> On 2022-03-28 15:57:37 +0300, a.soko...@postgrespro.ru wrote:

>> +data initialization. It is vital that any event trigger using the
>> +login event checks whether or not the database is in
>> +recovery.
>> 
>> Does any trigger really have to contain a pg_is_in_recovery() call?
> 
> Not *any* trigger, just any trigger that writes.

Thats correct, the docs should be updated with something like the below I
reckon.

It is vital that event trigger using the login event
which has side-effects checks whether or not the database is in recovery to
ensure they are not performing modifications to hot standby nodes.

>> In this message
>> (https://www.postgresql.org/message-id/20220312024652.lvgehszwke4hhove%40alap3.anarazel.de)
>> it was only about triggers on hot standby, which run not read-only queries
> 
> The problem precisely is that the login triggers run on hot standby nodes, and
> that if they do writes, you can't login anymore.

Do you think this potential foot-gun is scary enough to reject this patch?
There are lots of creative ways to cause Nagios alerts from ones database, but
this has the potential to do so with a small bug in userland code.  Still, I
kind of like the feature so I'm indecisive.

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2022-03-28 15:57:37 +0300, a.soko...@postgrespro.ru wrote:
> +data initialization. It is vital that any event trigger using the
> +login event checks whether or not the database is in
> +recovery.
> 
> Does any trigger really have to contain a pg_is_in_recovery() call?

Not *any* trigger, just any trigger that writes.


> In this message
> (https://www.postgresql.org/message-id/20220312024652.lvgehszwke4hhove%40alap3.anarazel.de)
> it was only about triggers on hot standby, which run not read-only queries

The problem precisely is that the login triggers run on hot standby nodes, and
that if they do writes, you can't login anymore.

Greetings,

Andres Freund

Re: On login trigger: take three

[a . sokolov]

Daniel Gustafsson писал 2022-03-22 11:43:

On 22 Mar 2022, at 04:48, Andres Freund  wrote:


docs fail to build: 
https://cirrus-ci.com/task/5556234047717376?logs=docs_build#L349


Ugh, that one was on me and not the original author. Fixed.



+data initialization. It is vital that any event trigger using the
+login event checks whether or not the database 
is in

+recovery.

Does any trigger really have to contain a pg_is_in_recovery() call? In 
this message 
(https://www.postgresql.org/message-id/20220312024652.lvgehszwke4hhove%40alap3.anarazel.de) 
it was only about triggers on hot standby, which run not read-only 
queries


--
Andrey Sokolov
Postgres Professional: http://www.postgrespro.com

Re: On login trigger: take three

[Daniel Gustafsson]

> On 22 Mar 2022, at 04:48, Andres Freund  wrote:

> docs fail to build: 
> https://cirrus-ci.com/task/5556234047717376?logs=docs_build#L349

Ugh, that one was on me and not the original author. Fixed.

--
Daniel Gustafsson   https://vmware.com/



v29-0001-Provide-a-GUC-for-temporarily-ignoring-event-tri.patch
Description: Binary data


v29-0002-Add-a-new-login-event-and-login-event-trigger-su.patch
Description: Binary data

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2022-03-18 17:03:39 +0100, Daniel Gustafsson wrote:
> > On 18 Mar 2022, at 08:24, a.soko...@postgrespro.ru wrote:
> 
> > -   auth_extra   => [ '--create-role', 'repl_role' ]);
> > +   auth_extra   => [ '--create-role', 'repl_role,regress_user' ]);
> 
> Looking at the test in question it's not entirely clear to me what the 
> original
> author really intended here, so I've changed it up a bit to actually test
> something and removed the need for the regress_user role.  I've also fixed the
> silly mistake highlighted in the postgresql.conf.sample test.

docs fail to build: 
https://cirrus-ci.com/task/5556234047717376?logs=docs_build#L349

Greetings,

Andres Freund

Re: On login trigger: take three

[Daniel Gustafsson]

> On 18 Mar 2022, at 08:24, a.soko...@postgrespro.ru wrote:

> -   auth_extra   => [ '--create-role', 'repl_role' ]);
> +   auth_extra   => [ '--create-role', 'repl_role,regress_user' ]);

Looking at the test in question it's not entirely clear to me what the original
author really intended here, so I've changed it up a bit to actually test
something and removed the need for the regress_user role.  I've also fixed the
silly mistake highlighted in the postgresql.conf.sample test.

--
Daniel Gustafsson   https://vmware.com/



v28-0002-Add-a-new-login-event-and-login-event-trigger-su.patch
Description: Binary data


v28-0001-Provide-a-GUC-for-temporarily-ignoring-event-tri.patch
Description: Binary data

Re: On login trigger: take three

[a . sokolov]

Daniel Gustafsson писал 2022-03-15 23:52:

Yeah, that was the previously posted v25 from the author (who adopted 
it from

the original author).  I took the liberty to quickly poke at the review
comments you had left as well as the ones that I had found to try and 
progress
the patch.  0001 should really go in it's own thread though to not hide 
it from

anyone interested who isn't looking at this thread.

Hi
I got an error while running tests on Windows:
2022-03-17 17:30:02.458 MSK [6920] [unknown] LOG:  no match in usermap 
"regress" for user "regress_user" authenticated as 
"vagrant@WINDOWS-2019"
2022-03-17 17:30:02.458 MSK [6920] [unknown] FATAL:  SSPI authentication 
failed for user "regress_user"
2022-03-17 17:30:02.458 MSK [6920] [unknown] DETAIL:  Connection matched 
pg_hba.conf line 2: "host all all 127.0.0.1/32  sspi include_realm=1 
map=regress"
2022-03-17 17:30:02.526 MSK [3432] FATAL:  could not receive data from 
WAL stream: server closed the connection unexpectedly

This probably means the server terminated abnormally
before or while processing the request.

I propose to make such correction:
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -14,7 +14,7 @@ my $node_primary = get_new_node('primary');
 # and it needs proper authentication configuration.
 $node_primary->init(
allows_streaming => 1,
-   auth_extra   => [ '--create-role', 'repl_role' ]);
+   auth_extra   => [ '--create-role', 'repl_role,regress_user' 
]);

 $node_primary->start;
 my $backup_name = 'my_backup';


--
Andrey Sokolov
Postgres Professional: http://www.postgrespro.com

Re: On login trigger: take three

[Daniel Gustafsson]

> On 14 Mar 2022, at 17:10, Andres Freund  wrote:
> 
> Hi,
> 
> On 2022-03-14 14:47:09 +0100, Daniel Gustafsson wrote:
>>> On 14 Mar 2022, at 01:08, Andres Freund  wrote:
>> 
>>> I was thinking that the way to use it would be to specify it as a client
>>> option. Like PGOPTIONS='-c ignore_event_trigger=login' psql.
>> 
>> Attached is a quick PoC/sketch of such a patch, where 0001 adds a guc, 0002 
>> is
>> the previously posted v25 login event trigger patch, and 0003 adapts is to
>> allow ignoring it (0002/0003 split only for illustrative purposes of course).
>> Is this along the lines of what you were thinking?
> 
> Yes.
> 
> Of course there's still the bogus cache invalidation stuff etc that I
> commented on upthread.

Yeah, that was the previously posted v25 from the author (who adopted it from
the original author).  I took the liberty to quickly poke at the review
comments you had left as well as the ones that I had found to try and progress
the patch.  0001 should really go in it's own thread though to not hide it from
anyone interested who isn't looking at this thread.

--
Daniel Gustafsson   https://vmware.com/



v27-0002-Add-a-new-login-event-and-login-event-trigger-su.patch
Description: Binary data


v27-0001-Provide-a-GUC-for-temporarily-ignoring-event-tri.patch
Description: Binary data

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2022-03-14 14:47:09 +0100, Daniel Gustafsson wrote:
> > On 14 Mar 2022, at 01:08, Andres Freund  wrote:
> 
> > I was thinking that the way to use it would be to specify it as a client
> > option. Like PGOPTIONS='-c ignore_event_trigger=login' psql.
> 
> Attached is a quick PoC/sketch of such a patch, where 0001 adds a guc, 0002 is
> the previously posted v25 login event trigger patch, and 0003 adapts is to
> allow ignoring it (0002/0003 split only for illustrative purposes of course).
> Is this along the lines of what you were thinking?

Yes.

Of course there's still the bogus cache invalidation stuff etc that I
commented on upthread.

Greetings,

Andres Freund

Re: On login trigger: take three

[Daniel Gustafsson]

> On 14 Mar 2022, at 01:08, Andres Freund  wrote:

> I was thinking that the way to use it would be to specify it as a client
> option. Like PGOPTIONS='-c ignore_event_trigger=login' psql.

Attached is a quick PoC/sketch of such a patch, where 0001 adds a guc, 0002 is
the previously posted v25 login event trigger patch, and 0003 adapts is to
allow ignoring it (0002/0003 split only for illustrative purposes of course).
Is this along the lines of what you were thinking?

--
Daniel Gustafsson   https://vmware.com/



v26-0003-Add-IGNORE_EVENT_TRIGGER_LOGIN.patch
Description: Binary data


v26-0002-v25-of-event-trigger-patch.patch
Description: Binary data


v26-0001-Provide-a-GUC-for-temporarily-ignoring-event-tri.patch
Description: Binary data

Re: On login trigger: take three

[Robert Haas]

On Sun, Mar 13, 2022 at 7:34 PM Andres Freund  wrote:
> IMO the other types of event triggers make it a heck of a lot harder to get
> yourself into a situation that you can't get out of...

In particular, unless something has changed since I committed this
stuff originally, there's no existing type of event trigger than can
prevent the superuser from logging in and running DROP EVENT TRIGGER
-- or a SELECT on the system catalogs to find out what to drop. That
was very much a deliberate decision on my part.

I think it's fine to require dropping to single-user mode as a way of
recovering from extreme situations where, for example, there are
corrupted database files. If we don't need it even then, cool, but if
we do, I'm not sad. But all we're talking about here is somebody maybe
running a command that perhaps they should not have run. Having to
take the whole system down to recover from that seems excessively
painful.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Daniel Gustafsson]

> On 14 Mar 2022, at 00:33, Andres Freund  wrote:

> We already have GUCs like row_security, so it doesn't seem insane to add one
> that disables login event triggers. What is the danger that you see?

My fear is that GUCs like that end up as permanent fixtures in scripts etc
after having been used temporary, and then X timeunits later someone realize
that the backup has never actually really worked due to a subtle issue, or
something else unpleasant.

The row_security GUC is kind of different IMO, as it's required for pg_dump
(though it can be used in the same way as the above).

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[Andres Freund]

On 2022-03-13 20:35:44 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I was thinking that the way to use it would be to specify it as a client
> > option. Like PGOPTIONS='-c ignore_event_trigger=login' psql.
> 
> Ugh ... that would allow people (at least superusers) to bypass
> the login trigger at will, which seems to me to break a lot of
> the use-cases for the feature.  I supposed we'd want this to be a
> PGC_POSTMASTER setting for security reasons.

Shrug. This doesn't seem to add actual security to me.

Re: On login trigger: take three

[Tom Lane]

Andres Freund  writes:
> I was thinking that the way to use it would be to specify it as a client
> option. Like PGOPTIONS='-c ignore_event_trigger=login' psql.

Ugh ... that would allow people (at least superusers) to bypass
the login trigger at will, which seems to me to break a lot of
the use-cases for the feature.  I supposed we'd want this to be a
PGC_POSTMASTER setting for security reasons.

regards, tom lane

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2022-03-13 19:57:08 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2022-03-13 23:31:03 +0100, Daniel Gustafsson wrote:
> >> Something like a '-c ignore_event_trigger=' GUC perhaps?
>
> > Did you mean login instead of event?
>
> > Something like it would work for me. It probably should be
> > GUC_DISALLOW_IN_FILE?
>
> Why?  Inserting such a setting in postgresql.conf and restarting
> would be the first thing I'd think of if I needed to get out
> of a problem.  The only other way is to set it on the postmaster
> command line, which is going to be awkward-to-impossible with
> most system-provided start scripts.

I was thinking that the way to use it would be to specify it as a client
option. Like PGOPTIONS='-c ignore_event_trigger=login' psql.

Greetings,

Andres Freund

Re: On login trigger: take three

[Tom Lane]

Andres Freund  writes:
> On 2022-03-13 23:31:03 +0100, Daniel Gustafsson wrote:
>> Something like a '-c ignore_event_trigger=' GUC perhaps? 

> Did you mean login instead of event?

> Something like it would work for me. It probably should be
> GUC_DISALLOW_IN_FILE?

Why?  Inserting such a setting in postgresql.conf and restarting
would be the first thing I'd think of if I needed to get out
of a problem.  The only other way is to set it on the postmaster
command line, which is going to be awkward-to-impossible with
most system-provided start scripts.

regards, tom lane

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2022-03-13 23:31:03 +0100, Daniel Gustafsson wrote:
> > On 12 Mar 2022, at 03:46, Andres Freund  wrote:
> 
> >> +   
> >> + The login event occurs when a user logs in to the
> >> + system.
> >> + Any bugs in a trigger procedure for this event may prevent successful
> >> + login to the system. Such bugs may be fixed after first restarting 
> >> the
> >> + system in single-user mode (as event triggers are disabled in this 
> >> mode).
> >> + See the  reference page for details 
> >> about
> >> + using single-user mode.
> >> +   
> > 
> > I'm strongly against adding any new dependencies on single user mode.
> > 
> > A saner approach might be a superuser-only GUC that can be set as part of 
> > the
> > connection data (e.g. PGOPTIONS='-c ignore_login_event_trigger=true').
> 
> This, and similar approaches, has been discussed in this thread.  I'm not a 
> fan
> of holes punched with GUC's like this, but if you plan on removing single-user
> mode (as I recall seeing in an initdb thread) altogether then that kills the
> discussion. So.

Even if we end up not removing single user mode, I think it's not OK to add
new failure modes that require single user mode to resolve after not-absurd
operations (I'm ok with needing single user mode if somebody does delete from
pg_authid). It's too hard to reach for most.

We already have GUCs like row_security, so it doesn't seem insane to add one
that disables login event triggers. What is the danger that you see?


> Since we already recommend single-user mode to handle broken event triggers, 
> we
> should IMO do something to cover all of them rather than risk ending up with
> one disabling GUC per each event type.  Right now we have this on the CREATE
> EVENT TRIGGER reference page:

IMO the other types of event triggers make it a heck of a lot harder to get
yourself into a situation that you can't get out of...


> "Event triggers are disabled in single-user mode (see postgres).  If an
> erroneous event trigger disables the database so much that you can't even
> drop the trigger, restart in single-user mode and you'll be able to do
> that."
> Something like a '-c ignore_event_trigger=' GUC perhaps? 

Did you mean login instead of event?

Something like it would work for me. It probably should be
GUC_DISALLOW_IN_FILE?

Greetings,

Andres Freund

Re: On login trigger: take three

[Daniel Gustafsson]

> On 12 Mar 2022, at 03:46, Andres Freund  wrote:

>> +   
>> + The login event occurs when a user logs in to the
>> + system.
>> + Any bugs in a trigger procedure for this event may prevent successful
>> + login to the system. Such bugs may be fixed after first restarting the
>> + system in single-user mode (as event triggers are disabled in this 
>> mode).
>> + See the  reference page for details about
>> + using single-user mode.
>> +   
> 
> I'm strongly against adding any new dependencies on single user mode.
> 
> A saner approach might be a superuser-only GUC that can be set as part of the
> connection data (e.g. PGOPTIONS='-c ignore_login_event_trigger=true').

This, and similar approaches, has been discussed in this thread.  I'm not a fan
of holes punched with GUC's like this, but if you plan on removing single-user
mode (as I recall seeing in an initdb thread) altogether then that kills the
discussion. So.

Since we already recommend single-user mode to handle broken event triggers, we
should IMO do something to cover all of them rather than risk ending up with
one disabling GUC per each event type.  Right now we have this on the CREATE
EVENT TRIGGER reference page:

"Event triggers are disabled in single-user mode (see postgres).  If an
erroneous event trigger disables the database so much that you can't even
drop the trigger, restart in single-user mode and you'll be able to do
that."

Something like a '-c ignore_event_trigger=' GUC perhaps? 

--
Daniel Gustafsson   https://vmware.com/

Re: On login trigger: take three

[Andres Freund]

Hi,

On 2022-02-15 21:07:15 +1100, Greg Nancarrow wrote:
> Subject: [PATCH v25] Add a new "login" event and login event trigger support.
> 
> The login event occurs when a user logs in to the system.

I think this needs a HUGE warning in the docs about most event triggers
needing to check pg_is_in_recovery() or breaking hot standby. And certainly
the example given needs to include an pg_is_in_recovery() check.


> +   
> + The login event occurs when a user logs in to the
> + system.
> + Any bugs in a trigger procedure for this event may prevent successful
> + login to the system. Such bugs may be fixed after first restarting the
> + system in single-user mode (as event triggers are disabled in this 
> mode).
> + See the  reference page for details about
> + using single-user mode.
> +   

I'm strongly against adding any new dependencies on single user mode.

A saner approach might be a superuser-only GUC that can be set as part of the
connection data (e.g. PGOPTIONS='-c ignore_login_event_trigger=true').


> @@ -293,6 +297,27 @@ insert_event_trigger_tuple(const char *trigname, const 
> char *eventname, Oid evtO
>   CatalogTupleInsert(tgrel, tuple);
>   heap_freetuple(tuple);
>  
> + if (strcmp(eventname, "login") == 0)
> + {
> + Form_pg_database db;
> + Relationpg_db = table_open(DatabaseRelationId, 
> RowExclusiveLock);
> +
> + /* Set dathasloginevt flag in pg_database */
> + tuple = SearchSysCacheCopy1(DATABASEOID, 
> ObjectIdGetDatum(MyDatabaseId));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for database %u", 
> MyDatabaseId);
> + db = (Form_pg_database) GETSTRUCT(tuple);
> + if (!db->dathasloginevt)
> + {
> + db->dathasloginevt = true;
> + CatalogTupleUpdate(pg_db, >t_self, tuple);
> + }
> + else
> + CacheInvalidateRelcacheByTuple(tuple);
> + table_close(pg_db, RowExclusiveLock);
> + heap_freetuple(tuple);
> + }
> +
>   /* Depend on owner. */
>   recordDependencyOnOwner(EventTriggerRelationId, trigoid, evtOwner);

Maybe I am confused, but isn't that CacheInvalidateRelcacheByTuple call
*entirely* bogus? CacheInvalidateRelcacheByTuple() expects a pg_class tuple,
but you're passing in a pg_database tuple?  And what is relcache invalidation
even supposed to achieve here?


I think this should mention that ->dathasloginevt is unset on login when
appropriate.



> +/*
> + * Fire login event triggers.
> + */
> +void
> +EventTriggerOnLogin(void)
> +{
> + List   *runlist;
> + EventTriggerData trigdata;
> +
> + /*
> +  * See EventTriggerDDLCommandStart for a discussion about why event
> +  * triggers are disabled in single user mode.
> +  */
> + if (!IsUnderPostmaster || !OidIsValid(MyDatabaseId))
> + return;
> +
> + StartTransactionCommand();
> +
> + if (DatabaseHasLoginEventTriggers())
> + {
> + runlist = EventTriggerCommonSetup(NULL,
> + 
>   EVT_Login, "login",
> + 
>   );
> +
> + if (runlist != NIL)
> + {
> + /*
> +  * Make sure anything the main command did will be 
> visible to the
> +  * event triggers.
> +  */
> + CommandCounterIncrement();

"Main command"?

It's not clear to my why a CommandCounterIncrement() could be needed here -
which previous writes do you need to make visible?



> + /*
> +  * Runlist is empty: clear dathasloginevt flag
> +  */
> + Relationpg_db = table_open(DatabaseRelationId, 
> RowExclusiveLock);
> + HeapTuple   tuple = 
> SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId));
> + Form_pg_database db;
> +
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for database 
> %u", MyDatabaseId);
> +
> + db = (Form_pg_database) GETSTRUCT(tuple);
> + if (db->dathasloginevt)
> + {
> + /*
> +  * There can be a race condition: a login event 
> trigger may
> +  * have been added after the pg_event_trigger 
> table was
> +  * scanned, and we don't want to erroneously 
> clear the
> +  * dathasloginevt flag in this case. To be sure 
> that this
> +  * hasn't happened, repeat the scan under the 
> pg_database
> 

Re: On login trigger: take three

[Robert Haas]

On Tue, Feb 15, 2022 at 5:07 AM Greg Nancarrow  wrote:
> I've attached a re-based version (no functional changes from the
> previous) to fix cfbot failures.

I tried this:

rhaas=# create function on_login_proc() returns event_trigger as
$$begin perform pg_sleep(1000); end$$ language plpgsql;
CREATE FUNCTION
rhaas=# create event trigger on_login_trigger on login execute
procedure on_login_proc();

When I then attempt to connect via psql, it hangs, as expected. When I
press ^C, psql exits, but the backend process is not cancelled and
just keeps chugging along in the background. The good news is that if
I connect to another database, I can cancel all of the hung sessions
using pg_cancel_backend(), and all of those processes then promptly
exit, and presumably I could accomplish the same thing by sending them
SIGINT directly. But it's still not great behavior. It would be easy
to use up a pile of connection slots inadvertently and have to go to
some trouble to get access to the server again. Since this is a psql
behavior and not a server behavior, one could argue that it's
unrelated to this patch, but in practice this patch seems to increase
the chances of people running into problems quite a lot.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: On login trigger: take three

[Daniel Gustafsson]

> On 15 Feb 2022, at 11:07, Greg Nancarrow  wrote:

> I've attached a re-based version (no functional changes from the
> previous) to fix cfbot failures.

Thanks for adopting this patch, I took another look at it today and have some
comments.

+This flag is used internally by PostgreSQL 
and should not be manually changed by DBA or application.
I think we should reword this to something like "This flag is used internally
by PostgreSQL and should not be altered or relied
upon for monitoring".  We really don't want anyone touching or interpreting
this value since there is no guarantee that it will be accurate.


+   new_record[Anum_pg_database_dathasloginevt - 1] = 
BoolGetDatum(datform->dathasloginevt);
Since the corresponding new_record_repl valus is false, this won't do anything
and can be removed.  We will use the old value anyways.


+   if (strcmp(eventname, "login") == 0)
I think this block warrants a comment on why it only applies to login triggers.


+   db = (Form_pg_database) GETSTRUCT(tuple);
+   if (!db->dathasloginevt)
+   {
+   db->dathasloginevt = true;
+   CatalogTupleUpdate(pg_db, >t_self, tuple);
+   }
+   else
+   CacheInvalidateRelcacheByTuple(tuple);
This needs a CommandCounterIncrement inside the if () { .. } block right?


+   /* Get the command tag. */
+   tag = (event == EVT_Login) ? CMDTAG_LOGIN : CreateCommandTag(parsetree);
To match project style I think this should be an if-then-else block.  Also,
while it's tempting to move this before the assertion block in order to reuse
it there, it does mean that we are calling this in a hot codepath before we
know if it's required.  I think we should restore the previous programming with
a separate CreateCommandTag call inside the assertion block and move this one
back underneath the fast-path exit.


+   CacheInvalidateRelcacheByTuple(tuple);
+   }
+   table_close(pg_db, RowExclusiveLock);
+   heap_freetuple(tuple);
+   }
+   }
+   CommitTransactionCommand();
Since we are commiting the transaction just after closing the table, is the
relcache invalidation going to achieve much?  I guess registering the event
doesn't hurt much?


+   /*
+* There can be a race condition: a login event trigger may
+* have been added after the pg_event_trigger table was
+* scanned, and we don't want to erroneously clear the
+* dathasloginevt flag in this case. To be sure that this
+* hasn't happened, repeat the scan under the pg_database
+* table lock.
+*/
+   AcceptInvalidationMessages();
+   runlist = EventTriggerCommonSetup(NULL,
+ EVT_Login, "login",
+ );
It seems conceptually wrong to allocate this in the TopTransactionContext when
we only generate the runlist to throw it away.  At the very least I think we
should free the returned list.


+   if (runlist == NULL)/* list is still empty, so clear the
This should check for NIL and not NULL.


Have you done any benchmarking on this patch?  With this version I see a small
slowdown on connection time of ~1.5% using pgbench for the case where there are
no login event triggers defined.  With a login event trigger calling an empty
plpgsql function there is a ~30% slowdown (corresponding to ~1ms on my system).
When there is a login event trigger defined all bets are off however, since the
function called can be arbitrarily complex and it's up to the user to measure
and decide - but the bare overhead we impose is still of interest of course.

--
Daniel Gustafsson   https://vmware.com/