Hi, >Tue, March 29, 2022, 0:31 +03:00 from Andres Freund <and...@anarazel.de>: > >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 >> >> + <literal>login</literal> 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 <literal>login</literal> 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: <important> <para> The <literal>login</literal> 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 <function>pg_is_in_recovery</function>(), see an example below. </para> </important> > 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 'GRANT day_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