Обсуждение: On login trigger: take three

Hi hackers,

Recently I have asked once again by one of our customers about login 
trigger in postgres. People are migrating to Postgres from Oracle and  
looking for Postgres analog of this Oracle feature.
This topic is not new:

https://www.postgresql.org/message-id/flat/1570308356720-0.post%40n3.nabble.com#4748bcb0c5fc98cec0a735dbdffb0c68

https://www.postgresql.org/message-id/flat/OSAPR01MB507373499CCCEA00EAE79875FE2D0%40OSAPR01MB5073.jpnprd01.prod.outlook.com#ed50c248be32be6955c385ca67d6cdc1

end even session connect/disconnect hooks were sometimes committed (but 
then reverted).
As far as I understand most of the concerns were related with disconnect 
hook.
Performing some action on session disconnect is actually much more 
complicated than on login.
But customers are not needed it, unlike actions performed at session start.

I wonder if we are really going to make some steps in this directions?
The discussion above was finished with "We haven't rejected the concept 
altogether, AFAICT"

I have tried to resurrect this patch and implement on-connect trigger on 
top of it.
The syntax is almost the same as proposed by Takayuki:

CREATE EVENT TRIGGER mytrigger
AFTER CONNECTION ON mydatabase
EXECUTE {PROCEDURE | FUNCTION} myproc();

I have replaced CONNECT with CONNECTION because last keyword is already 
recognized by Postgres and
make ON clause mandatory to avoid shift-reduce conflicts.

Actually specifying database name is redundant, because we can define 
on-connect trigger only for self database (just because triggers and 
functions are local to the database).
It may be considered as argument against handling session start using 
trigger. But it seems to be the most natural mechanism for users.

On connect trigger can be dropped almost in the same way as normal (on 
relation) trigger, but with specifying name of the database instead of 
relation name:

DROP TRIGGER mytrigger ON mydatabase;

It is possible to define arbitrary number of on-connect triggers with 
different names.

I attached my prototype implementation of this feature.
I just to be sure first that this feature will be interested to community.
If so, I will continue work in it and prepare new version of the patch 
for the commitfest.

Example

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
> Recently I have asked once again by one of our customers about login trigger in
> postgres. People are migrating to Postgres from Oracle and looking for Postgres
> analog of this Oracle feature.
> This topic is not new:

> I attached my prototype implementation of this feature.
> I just to be sure first that this feature will be interested to community.
> If so, I will continue work in it and prepare new version of the patch for the
> commitfest.

Thanks a lot for taking on this! +10

> It may be considered as argument against handling session start using trigger.
> But it seems to be the most natural mechanism for users.

Yeah, it's natural, just like the Unix shells run some shell scripts in the home directory.


Regards
Takayuki Tsunakawa

Sorry, attached version of the patch is missing changes in one file. 
Here is it correct patch.


-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

čt 3. 9. 2020 v 15:43 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

Hi hackers,

Recently I have asked once again by one of our customers about login
trigger in postgres. People are migrating to Postgres from Oracle and 
looking for Postgres analog of this Oracle feature.
This topic is not new:

https://www.postgresql.org/message-id/flat/1570308356720-0.post%40n3.nabble.com#4748bcb0c5fc98cec0a735dbdffb0c68
https://www.postgresql.org/message-id/flat/OSAPR01MB507373499CCCEA00EAE79875FE2D0%40OSAPR01MB5073.jpnprd01.prod.outlook.com#ed50c248be32be6955c385ca67d6cdc1

end even session connect/disconnect hooks were sometimes committed (but
then reverted).
As far as I understand most of the concerns were related with disconnect
hook.
Performing some action on session disconnect is actually much more
complicated than on login.
But customers are not needed it, unlike actions performed at session start.

I wonder if we are really going to make some steps in this directions?
The discussion above was finished with "We haven't rejected the concept
altogether, AFAICT"

I have tried to resurrect this patch and implement on-connect trigger on
top of it.
The syntax is almost the same as proposed by Takayuki:

CREATE EVENT TRIGGER mytrigger
AFTER CONNECTION ON mydatabase
EXECUTE {PROCEDURE | FUNCTION} myproc();

I have replaced CONNECT with CONNECTION because last keyword is already
recognized by Postgres and
make ON clause mandatory to avoid shift-reduce conflicts.

Actually specifying database name is redundant, because we can define
on-connect trigger only for self database (just because triggers and
functions are local to the database).
It may be considered as argument against handling session start using
trigger. But it seems to be the most natural mechanism for users.

On connect trigger can be dropped almost in the same way as normal (on
relation) trigger, but with specifying name of the database instead of
relation name:

DROP TRIGGER mytrigger ON mydatabase;

It is possible to define arbitrary number of on-connect triggers with
different names.

I attached my prototype implementation of this feature.
I just to be sure first that this feature will be interested to community.
If so, I will continue work in it and prepare new version of the patch
for the commitfest.

I have a customer that requires this feature too. Now it uses a solution based on dll session autoloading.  Native solution can be great.

+1

On 14.09.2020 12:44, Pavel Stehule wrote:
> Hi
>
> I am checking last patch, and there are notices
>
> 1. disable_session_start_trigger should be SU_BACKEND instead SUSET
>
> 2. The documentation should be enhanced - there is not any note about 
> behave when there are unhandled exceptions, about motivation for this 
> event trigger
>
> 3. regress tests should be enhanced - the cases with exceptions are 
> not tested
>
> 4. This trigger is not executed again after RESET ALL or DISCARD ALL - 
> it can be a problem if somebody wants to use this trigger for 
> initialisation of some session objects with some pooling solutions.
>
> 5. The handling errors don't work well for canceling. If event trigger 
> waits for some event, then cancel disallow connect although connected 
> user is superuser
>
> CREATE OR REPLACE FUNCTION on_login_proc2() RETURNS EVENT_TRIGGER AS 
> $$ begin perform pg_sleep(10000); raise notice '%', fx1(100);raise 
> notice 'kuku kuku'; end  $$ language plpgsql;
>
> probably nobody will use pg_sleep in this routine, but there can be 
> wait on some locks ...
>
> Regards
>
> Pavel
>
>
>

Hi
Thank you very much for looking at my patch for connection triggers.
I have fixed 1-3 issues in the attached patch.
Concerning 4 and 5 I have some doubts:

4. Should I add some extra GUC to allow firing of session_start trigger 
in case of  RESET ALL or DISCARD ALL ?
Looks like such behavior contradicts with event name "session_start"...
And do we really need it? If some pooler is using RESET ALL/DISCARD ALL 
to emulate session semantic then  most likely it provides way to define 
custom actions which
should be perform for session initialization. As far as I know, for 
example pgbouncer allows do define own on-connect hooks.

5. I do not quite understand your concern. If I define  trigger 
procedure which is  blocked (for example as in your example), then I can 
use pg_cancel_backend to interrupt execution of login trigger and 
superuser can login. What should be changed here?



-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

If we introduce buildin session trigger , we should to define what is the session. Your design is much more related to the process than to session. So the correct name should be "process_start" trigger, or some should be different. I think there are two different events - process_start, and session_start, and there should be two different event triggers. Maybe the name "session_start" is just ambiguous and should be used with a different name. 

 

5. I do not quite understand your concern. If I define  trigger
procedure which is  blocked (for example as in your example), then I can
use pg_cancel_backend to interrupt execution of login trigger and
superuser can login. What should be changed here?

You cannot run pg_cancel_backend, because you cannot open a new session. There is a cycle.

On Tue, Sep 15, 2020 at 2:12 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>>
>> It is always possible to login by disabling startup triggers using disable_session_start_trigger GUC:
>>
>> psql "dbname=postgres options='-c disable_session_start_trigger=true'"
>
>
> sure, I know. Just this behavior can be a very unpleasant surprise, and my question is if it can be fixed.  Creating
customlibpq variables can be the stop for people that use pgAdmin.
 
>

Hi,

I thought in the case of using pgAdmin (assuming you can connect as
superuser to a database, say the default "postgres" maintenance
database, that doesn't have an EVENT TRIGGER defined for the
session_start event) you could issue the query "ALTER SYSTEM SET
disable_session_start_trigger TO true;"  and then reload the
configuration?

Anyway, I am wondering if this patch is still being actively developed/improved?

Regarding the last-posted patch, I'd like to give some feedback. I
found that the documentation part wouldn't build because of errors in
the SGML tags. There are some grammatical errors too, and some minor
inconsistencies with the current documentation, and some descriptions
could be improved. I think that a colon separator should be added to
the NOTICE message for superuser, so it's clear exactly where the text
of the underlying error message starts. Also, I think that
"client_connection" is perhaps a better and more intuitive event name
than "session_start", or the suggested "user_backend_start".
I've therefore attached an updated patch with these suggested minor
improvements, please take a look and see what you think (please
compare with the original patch).

Regards,
Greg Nancarrow
Fujitsu Australia

On 04.12.2020 3:50, Greg Nancarrow wrote:
> On Tue, Sep 15, 2020 at 2:12 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>> It is always possible to login by disabling startup triggers using disable_session_start_trigger GUC:
>>>
>>> psql "dbname=postgres options='-c disable_session_start_trigger=true'"
>>
>> sure, I know. Just this behavior can be a very unpleasant surprise, and my question is if it can be fixed.  Creating
customlibpq variables can be the stop for people that use pgAdmin.
 
>>
> Hi,
>
> I thought in the case of using pgAdmin (assuming you can connect as
> superuser to a database, say the default "postgres" maintenance
> database, that doesn't have an EVENT TRIGGER defined for the
> session_start event) you could issue the query "ALTER SYSTEM SET
> disable_session_start_trigger TO true;"  and then reload the
> configuration?
As far as I understand Pavel concern was about the case when superuser 
defines wrong login trigger which prevents login to the system
all user including himself. Right now solution of this problem is to 
include "options='-c disable_session_start_trigger=true'" in connection 
string.
I do not know if it can be done with pgAdmin.
>
> Anyway, I am wondering if this patch is still being actively developed/improved?

I do not know what else has to be improved.
If you, Pavel or anybody else have some suggestions: please let me know.
>
> Regarding the last-posted patch, I'd like to give some feedback. I
> found that the documentation part wouldn't build because of errors in
> the SGML tags. There are some grammatical errors too, and some minor
> inconsistencies with the current documentation, and some descriptions
> could be improved. I think that a colon separator should be added to
> the NOTICE message for superuser, so it's clear exactly where the text
> of the underlying error message starts. Also, I think that
> "client_connection" is perhaps a better and more intuitive event name
> than "session_start", or the suggested "user_backend_start".
> I've therefore attached an updated patch with these suggested minor
> improvements, please take a look and see what you think (please
> compare with the original patch).

Thank you very much for detecting the problems and much more thanks for 
fixing them and providing your version of the patch.
I have nothing against renaming "session_start" to "client_connection".

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Dec 4, 2020 at 9:05 PM Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:
>
> As far as I understand Pavel concern was about the case when superuser
> defines wrong login trigger which prevents login to the system
> all user including himself. Right now solution of this problem is to
> include "options='-c disable_session_start_trigger=true'" in connection
> string.
> I do not know if it can be done with pgAdmin.
> >

As an event trigger is tied to a particular database, and a GUC is
global to the cluster, as long as there is one database in the cluster
for which an event trigger for the "client_connection" event is NOT
defined (say the default "postgres" maintenance database), then the
superuser can always connect to that database, issue "ALTER SYSTEM SET
disable_client_connection_trigger TO true" and reload the
configuration. I tested this with pgAdmin4 and it worked fine for me,
to allow login to a database for which login was previously prevented
due to a badly-defined logon trigger.

Pavel, is this an acceptable solution or do you still see problems
with this approach?


Regards,
Greg Nancarrow
Fujitsu Australia

On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> There are two maybe generic questions?
>
> 1. Maybe we can introduce more generic GUC for all event triggers like disable_event_triggers? This GUC can be
checkedonly by the database owner or super user. It can be an alternative ALTER TABLE DISABLE TRIGGER ALL. It can be
protectionagainst necessity to restart to single mode to repair the event trigger. I think so more generic solution is
betterthan special disable_client_connection_trigger GUC. 
>
> 2. I have no objection against client_connection. It is probably better for the mentioned purpose - possibility to
blockconnection to database. Can be interesting, and I am not sure how much work it is to introduce the second event -
session_start.This event should be started after connecting - so the exception there doesn't block connect, and should
bestarted also after the new statement "DISCARD SESSION", that will be started automatically after DISCARD ALL.  This
featureshould not be implemented in first step, but it can be a plan for support pooled connections 
>

I've created a separate patch to address question (1), rather than
include it in the main patch, which I've adjusted accordingly. I'll
leave question (2) until another time, as you suggest.
See the attached patches.

Regards,
Greg Nancarrow
Fujitsu Australia

Hi

st 9. 12. 2020 v 13:17 odesílatel Greg Nancarrow <gregn4422@gmail.com> napsal:

On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> There are two maybe generic questions?
>
> 1. Maybe we can introduce more generic GUC for all event triggers like disable_event_triggers? This GUC can be checked only by the database owner or super user. It can be an alternative ALTER TABLE DISABLE TRIGGER ALL. It can be protection against necessity to restart to single mode to repair the event trigger. I think so more generic solution is better than special disable_client_connection_trigger GUC.
>
> 2. I have no objection against client_connection. It is probably better for the mentioned purpose - possibility to block connection to database. Can be interesting, and I am not sure how much work it is to introduce the second event - session_start. This event should be started after connecting - so the exception there doesn't block connect, and should be started also after the new statement "DISCARD SESSION", that will be started automatically after DISCARD ALL.  This feature should not be implemented in first step, but it can be a plan for support pooled connections
>

I've created a separate patch to address question (1), rather than
include it in the main patch, which I've adjusted accordingly. I'll
leave question (2) until another time, as you suggest.
See the attached patches.

I see two possible questions?

1. when you introduce this event, then the new hook is useless ?

2. what is a performance impact for users that want not to use this feature. What is a overhead of EventTriggerOnConnect and is possible to skip this step if database has not any event trigger

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

st 9. 12. 2020 v 14:28 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 09.12.2020 15:34, Pavel Stehule wrote:

Hi

st 9. 12. 2020 v 13:17 odesílatel Greg Nancarrow <gregn4422@gmail.com> napsal:

On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> There are two maybe generic questions?
>
> 1. Maybe we can introduce more generic GUC for all event triggers like disable_event_triggers? This GUC can be checked only by the database owner or super user. It can be an alternative ALTER TABLE DISABLE TRIGGER ALL. It can be protection against necessity to restart to single mode to repair the event trigger. I think so more generic solution is better than special disable_client_connection_trigger GUC.
>
> 2. I have no objection against client_connection. It is probably better for the mentioned purpose - possibility to block connection to database. Can be interesting, and I am not sure how much work it is to introduce the second event - session_start. This event should be started after connecting - so the exception there doesn't block connect, and should be started also after the new statement "DISCARD SESSION", that will be started automatically after DISCARD ALL.  This feature should not be implemented in first step, but it can be a plan for support pooled connections
>

I've created a separate patch to address question (1), rather than
include it in the main patch, which I've adjusted accordingly. I'll
leave question (2) until another time, as you suggest.
See the attached patches.

I see two possible questions?

1. when you introduce this event, then the new hook is useless ?

2. what is a performance impact for users that want not to use this feature. What is a overhead of EventTriggerOnConnect and is possible to skip this step if database has not any event trigger

As far as I understand this are questions to me rather than to Greg.
1. Do you mean client_connection_hook? It is used to implement this new event type. It can be also used for other purposes.

ok. I don't like it, but there are redundant hooks (against event triggers) already. 

2. Connection overhead is quite large. Supporting on connect hook requires traversal of event trigger relation. But this overhead is negligible comparing with overhead of establishing connection. In any case I did the following test (with local connection):

pgbench -C -S -T 100 -P 1 -M prepared postgres

without this patch:
tps = 424.287889 (including connections establishing)
tps = 952.911068 (excluding connections establishing)

with this patch (but without any connection trigger defined):
tps = 434.642947 (including connections establishing)
tps = 995.525383 (excluding connections establishing)

As you can see - there is almost now different (patched version is even faster, but it seems to be just "white noise".

This is not the worst case probably. In this patch the StartTransactionCommand is executed on every connection, although it is not necessary - and for most use cases it will not be used.

I did more tests - see attachments and I see a 5% slowdown - I don't think so it is acceptable for this case. This feature is nice, and for some users important, but only really few users can use it.

┌────────────────┬─────────┬────────────┬─────────────┐
│      test      │ WITH LT │ LT ENABLED │ LT DISABLED │
╞════════════════╪═════════╪════════════╪═════════════╡
│ ro_constant_10 │     539 │        877 │         905 │
│ ro_index_10    │     562 │        808 │         855 │
│ ro_constant_50 │     527 │        843 │         863 │
│ ro_index_50    │     544 │        731 │         742 │
└────────────────┴─────────┴────────────┴─────────────┘
(4 rows)

I tested a performance of trigger (results of first column in table):

CREATE OR REPLACE FUNCTION public.foo()
 RETURNS event_trigger
 LANGUAGE plpgsql
AS $function$
begin
  if not pg_has_role(session_user, 'postgres', 'member') then
    raise exception 'you are not super user';
  end if;
end;
$function$;

There is an available snapshot in InitPostgres, and then there is possible to check if for the current database some connect event trigger exists.This can reduce an overhead of this patch, when there are no logon triggers.

I think so implemented and used names are ok, but for this feature the performance impact should be really very minimal.

There is other small issue - missing tab-complete support for CREATE TRIGGER statement in psql

Regards

Pavel

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

My idea was a little bit different. Inside postinit initialize some global variables with info if there are event triggers or not. And later you can use this variable to start transactions and  other things.

There will be two access to pg_event_trigger, but it can eliminate useless and probably more expensive start_transaction and end_transaction.

čt 10. 12. 2020 v 16:48 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 10.12.2020 18:12, Pavel Stehule wrote:

My idea was a little bit different. Inside postinit initialize some global variables with info if there are event triggers or not. And later you can use this variable to start transactions and  other things.

There will be two access to pg_event_trigger, but it can eliminate useless and probably more expensive start_transaction and end_transaction.

Do you mean some variable in shared memory or GUCs?
It was my first idea - to use some flag in shared memory to make it possible fast check that there are not event triggers.
But this flag should be sent per database. May be I missed something, but there is no any per-database shared memory  data structure in Postgres.
Certainly it is possible to organize some hash db->event_info, but it makes this patch several times more complex.

My idea was a little bit different - just inside process initialization, checking existence of event triggers, and later when it is necessary to start a transaction for trigger execution. This should to reduce useless empty transaction,

I am sure this is a problem on computers with slower CPU s, although I have I7, but because this feature is usually unused, then the performance impact should be minimal every time.

 

From my point of view it is better to have separate GUC disabling just client connection events and switch it on by default.
So only those who need this events with switch it on, other users will not pay any extra cost for it.

It can work, but this design is not user friendly.  The significant bottleneck should be forking new processes, and check of content some usually very small tables should be invisible. So if there is a possible way to implement some feature that can be enabled by default, then we should go this way. It can be pretty unfriendly if somebody writes a logon trigger and it will not work by default without any warning.

When I tested last patch I found a problem (I have disabled assertions due performance testing)

create role xx login;

alter system set disable_event_triggers to on; -- better should be positive form - enable_event_triggers to on, off

select pg_reload_conf();

psql -U xx

Thread 2.1 "postmaster" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f2bff438ec0 (LWP 827497)]
ResourceOwnerEnlargeCatCacheRefs (owner=0x0) at resowner.c:1025
1025 ResourceArrayEnlarge(&(owner->catrefarr));
(gdb) bt
#0  ResourceOwnerEnlargeCatCacheRefs (owner=0x0) at resowner.c:1025
#1  0x00000000008a70f8 in SearchCatCacheInternal (cache=<optimized out>, nkeys=nkeys@entry=1, v1=v1@entry=13836, v2=v2@entry=0,
    v3=v3@entry=0, v4=v4@entry=0) at catcache.c:1273
#2  0x00000000008a7575 in SearchCatCache1 (cache=<optimized out>, v1=v1@entry=13836) at catcache.c:1167
#3  0x00000000008b7f80 in SearchSysCache1 (cacheId=cacheId@entry=21, key1=key1@entry=13836) at syscache.c:1122
#4  0x00000000005939cd in pg_database_ownercheck (db_oid=13836, roleid=16387) at aclchk.c:5114
#5  0x0000000000605b42 in EventTriggersDisabled () at event_trigger.c:650
#6  EventTriggerOnConnect () at event_trigger.c:839
#7  0x00000000007b46d7 in PostgresMain (argc=argc@entry=1, argv=argv@entry=0x7ffdd6256080, dbname=<optimized out>,
    username=0xf82508 "tom") at postgres.c:4021
#8  0x0000000000741afd in BackendRun (port=<optimized out>, port=<optimized out>) at postmaster.c:4490
#9  BackendStartup (port=<optimized out>) at postmaster.c:4212
#10 ServerLoop () at postmaster.c:1729
#11 0x0000000000742881 in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0xf44d00) at postmaster.c:1401
#12 0x00000000004f1ff7 in main (argc=3, argv=0xf44d00) at main.c:209

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

st 9. 12. 2020 v 13:17 odesílatel Greg Nancarrow <gregn4422@gmail.com> napsal:

On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> There are two maybe generic questions?
>
> 1. Maybe we can introduce more generic GUC for all event triggers like disable_event_triggers? This GUC can be checked only by the database owner or super user. It can be an alternative ALTER TABLE DISABLE TRIGGER ALL. It can be protection against necessity to restart to single mode to repair the event trigger. I think so more generic solution is better than special disable_client_connection_trigger GUC.
>
> 2. I have no objection against client_connection. It is probably better for the mentioned purpose - possibility to block connection to database. Can be interesting, and I am not sure how much work it is to introduce the second event - session_start. This event should be started after connecting - so the exception there doesn't block connect, and should be started also after the new statement "DISCARD SESSION", that will be started automatically after DISCARD ALL.  This feature should not be implemented in first step, but it can be a plan for support pooled connections
>

PGC_SU_BACKEND is too strong, there should be PGC_BACKEND if this option can be used by database owner

Pavel

I've created a separate patch to address question (1), rather than
include it in the main patch, which I've adjusted accordingly. I'll
leave question (2) until another time, as you suggest.
See the attached patches.

Regards,
Greg Nancarrow
Fujitsu Australia

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

is not correct. It makes it not possible to superuser to disable triggers for all users.

pg_database_ownercheck returns true for superuser always.

Also GUCs are not associated with any database. So I do not understand why  this check of database ownership is relevant at all?

What kind of protection violation we want to prevent?

It seems to be obvious that normal user should not be able to prevent trigger execution because this triggers may be used to enforce some security policies.
If trigger was created by user itself, then it can drop or disable it using ALTER statement. GUC is not needed for it.

when you cannot connect to the database, then you cannot do ALTER. In DBaaS environments lot of users has not superuser rights.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

pá 11. 12. 2020 v 17:05 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 11.12.2020 18:40, Pavel Stehule wrote:

is not correct. It makes it not possible to superuser to disable triggers for all users.

pg_database_ownercheck returns true for superuser always.

Sorry, but I consider different case: when normal user is connected to the database.
In this case pg_database_ownercheck returns false and trigger is not disabled, isn't it?

My idea was to reduce necessary rights to database owners.  But you have a true, so only superuser can create event trigger, so this feature cannot be used in DBaaS environments, and then my original idea was wrong.

Also GUCs are not associated with any database. So I do not understand why  this check of database ownership is relevant at all?

What kind of protection violation we want to prevent?

It seems to be obvious that normal user should not be able to prevent trigger execution because this triggers may be used to enforce some security policies.
If trigger was created by user itself, then it can drop or disable it using ALTER statement. GUC is not needed for it.

when you cannot connect to the database, then you cannot do ALTER. In DBaaS environments lot of users has not superuser rights.

But only superusers can set login triggers, right?
So only superuser can make a mistake in this trigger. But he have enough rights to recover this error. Normal users are not able to define on connection triggers and
should not have rights to disable them.

yes, it is true

Pavel

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

út 15. 12. 2020 v 14:12 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 11.12.2020 19:27, Pavel Stehule wrote:

pá 11. 12. 2020 v 17:05 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 11.12.2020 18:40, Pavel Stehule wrote:

is not correct. It makes it not possible to superuser to disable triggers for all users.

pg_database_ownercheck returns true for superuser always.

Sorry, but I consider different case: when normal user is connected to the database.
In this case pg_database_ownercheck returns false and trigger is not disabled, isn't it?

My idea was to reduce necessary rights to database owners.  But you have a true, so only superuser can create event trigger, so this feature cannot be used in DBaaS environments, and then my original idea was wrong.

Also GUCs are not associated with any database. So I do not understand why  this check of database ownership is relevant at all?

What kind of protection violation we want to prevent?

It seems to be obvious that normal user should not be able to prevent trigger execution because this triggers may be used to enforce some security policies.
If trigger was created by user itself, then it can drop or disable it using ALTER statement. GUC is not needed for it.

when you cannot connect to the database, then you cannot do ALTER. In DBaaS environments lot of users has not superuser rights.

But only superusers can set login triggers, right?
So only superuser can make a mistake in this trigger. But he have enough rights to recover this error. Normal users are not able to define on connection triggers and
should not have rights to disable them.

yes, it is true

Pavel

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

So what's next?
I see three options:

1. Do not introduce GUC for disabling all event triggers (especially taken in account that them are  disabled by default).
Return back to the patch on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch with "disable_client_connection_trigger"
and make it true by default (to eliminate any overhead for users which are not using on logintriggers).

2. Have two GUCS: "disable_client_connection_trigger" and "disable_event_triggers".

3. Implement some mechanism for caching presence of event triggers in shared memory.

@3 is the best design (the things should work well by default), @2 is a little bit chaotic and @1 looks like a workaround.

Regards

Pavel

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

út 15. 12. 2020 v 15:06 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 15.12.2020 16:18, Pavel Stehule wrote:

út 15. 12. 2020 v 14:12 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 11.12.2020 19:27, Pavel Stehule wrote:

pá 11. 12. 2020 v 17:05 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 11.12.2020 18:40, Pavel Stehule wrote:

is not correct. It makes it not possible to superuser to disable triggers for all users.

pg_database_ownercheck returns true for superuser always.

Sorry, but I consider different case: when normal user is connected to the database.
In this case pg_database_ownercheck returns false and trigger is not disabled, isn't it?

My idea was to reduce necessary rights to database owners.  But you have a true, so only superuser can create event trigger, so this feature cannot be used in DBaaS environments, and then my original idea was wrong.

Also GUCs are not associated with any database. So I do not understand why  this check of database ownership is relevant at all?

What kind of protection violation we want to prevent?

It seems to be obvious that normal user should not be able to prevent trigger execution because this triggers may be used to enforce some security policies.
If trigger was created by user itself, then it can drop or disable it using ALTER statement. GUC is not needed for it.

when you cannot connect to the database, then you cannot do ALTER. In DBaaS environments lot of users has not superuser rights.

But only superusers can set login triggers, right?
So only superuser can make a mistake in this trigger. But he have enough rights to recover this error. Normal users are not able to define on connection triggers and
should not have rights to disable them.

yes, it is true

Pavel

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

So what's next?
I see three options:

1. Do not introduce GUC for disabling all event triggers (especially taken in account that them are  disabled by default).
Return back to the patch on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch with "disable_client_connection_trigger"
and make it true by default (to eliminate any overhead for users which are not using on logintriggers).

2. Have two GUCS: "disable_client_connection_trigger" and "disable_event_triggers".

3. Implement some mechanism for caching presence of event triggers in shared memory.

@3 is the best design (the things should work well by default), @2 is a little bit chaotic and @1 looks like a workaround.

Please notice that we still need GUC to disable on-login triggers: to make it possible for superuser who did mistake and defined incorrect on-login trigger to
login to the system.
Do we need GUC to disable all other event triggers? May be I am wrong, but I do not see much need in such GUC: error in any of such event triggers is non fatal
and can be easily reverted.
So the only question is whether "disable_client_connection_trigger" should be true by default or not...

I agree with you that @2 is a little bit chaotic and @1 looks like a workaround.
But from my point of view @3 is not the best solution but overkill: maintaining yet another shared hash just to save few milliseconds on login seems to be too high price.
Actually there are many things which are loaded by new backend from the database on start: for example - catalog.
This is why launch of new backend is an expensive operation.
Certainly if we execute "select 1", then system catalog is not needed...
But does anybody start new backend just to execute "select 1" and exit?

I understand so the implementation of a new shared cache can be a lot of work. The best way is enhancing pg_database about one column with information about the login triggers (dathaslogontriggers). In init time these data are in syscache, and can be easily checked. Some like pg_attribute have an atthasdef column.  Same fields has pg_class - relhasrules, relhastriggers, ... Then the overhead of this design should be really zero.

What do you think about it?

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

Please notice that we still need GUC to disable on-login triggers: to make it possible for superuser who did mistake and defined incorrect on-login trigger to
login to the system.
Do we need GUC to disable all other event triggers? May be I am wrong, but I do not see much need in such GUC: error in any of such event triggers is non fatal
and can be easily reverted.
So the only question is whether "disable_client_connection_trigger" should be true by default or not...

I agree with you that @2 is a little bit chaotic and @1 looks like a workaround.
But from my point of view @3 is not the best solution but overkill: maintaining yet another shared hash just to save few milliseconds on login seems to be too high price.
Actually there are many things which are loaded by new backend from the database on start: for example - catalog.
This is why launch of new backend is an expensive operation.
Certainly if we execute "select 1", then system catalog is not needed...
But does anybody start new backend just to execute "select 1" and exit?

I understand so the implementation of a new shared cache can be a lot of work. The best way is enhancing pg_database about one column with information about the login triggers (dathaslogontriggers). In init time these data are in syscache, and can be easily checked. Some like pg_attribute have an atthasdef column.  Same fields has pg_class - relhasrules, relhastriggers, ... Then the overhead of this design should be really zero.

What do you think about it?

I like this approach more than implementation of shared hash.
But still I have some concerns:

1. pg_database table format has to be changed. Certainly it is not something  completely unacceptable. But IMHO we should try to avoid
modification of such commonly used catalog tables as much as possible.

yes, I know. Unfortunately I  don't see any other and correct solution. There should be more wide discussion before this work about this topic. On second hand, this change should not break anything (if this new field will be placed on as the last field). The logon trigger really looks like a database trigger - so I think so this semantic is correct. I have no idea if it is acceptable for committers :-/. I hope so. 

The fact that the existence of a logon trigger can be visible from a shared database object can be an interesting feature too.

2. It is not so easy to maintain this flag. There can be multiple on-login triggers defined. If such trigger is dropped, we can not just clear this flag.
We should check if other triggers exist. Now assume that there are two triggers and two concurrent transactions dropping each one.
According to their snapshots them do not see changes made by other transaction. So them remove both triggers but didn't clear the flag.
Certainly we can use counter instead of flag. But I am not sure that their will be not other problems with maintaining counter.

I don't think it is necessary.  My opinion is not too strong, but if pg_class doesn't need to hold a number of triggers, then I think so pg_database doesn't need to hold this number too.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

st 16. 12. 2020 v 20:38 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

Attached please find new versoin of the patch based on on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch

So there is still only  "disable_client_connection_trigger" GUC? because we need possibility to disable client connect triggers and there is no such need for other event types.

As you suggested  have added "dathaslogontriggers" flag which is set when client connection trigger is created.
This flag is never cleaned (to avoid visibility issues mentioned in my previous mail).

This is much better - I don't see any slowdown when logon trigger is not defined

I did some benchmarks and looks so starting language handler is relatively expensive - it is about 25% and starting handler like event trigger has about 35%. But this problem can be solved later and elsewhere.

I prefer the inverse form of disable_connection_trigger. Almost all GUC are in positive form - so enable_connection_triggger is better

I don't think so current handling dathaslogontriggers is good for production usage. The protection against race condition can be solved by lock on pg_event_trigger

I thought about it, and probably the counter of connect triggers will be better there. The implementation will be simpler and more robust.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

Hi,

On 17.12.2020 3:31, Zhihong Yu wrote:
> Hi,
> For EventTriggerOnConnect():
>
> +           PG_CATCH();
> +           {
> ...
> +               AbortCurrentTransaction();
> +               return;
>
> Should runlist be freed in the catch block ?

No need: it is allocated in transaction memory context and removed on 
transaction abort.

>
> +           gettext_noop("In case of errors in the ON 
> client_connection EVENT TRIGGER procedure, this parameter can be used 
> to disable trigger activation and provide access to the database."),
>
> I think the text should be on two lines (current line too long).

Thank you, fixed.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

čt 17. 12. 2020 v 19:30 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

čt 17. 12. 2020 v 14:04 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 17.12.2020 9:31, Pavel Stehule wrote:

st 16. 12. 2020 v 20:38 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

Attached please find new versoin of the patch based on on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch

So there is still only  "disable_client_connection_trigger" GUC? because we need possibility to disable client connect triggers and there is no such need for other event types.

As you suggested  have added "dathaslogontriggers" flag which is set when client connection trigger is created.
This flag is never cleaned (to avoid visibility issues mentioned in my previous mail).

This is much better - I don't see any slowdown when logon trigger is not defined

I did some benchmarks and looks so starting language handler is relatively expensive - it is about 25% and starting handler like event trigger has about 35%. But this problem can be solved later and elsewhere.

I prefer the inverse form of disable_connection_trigger. Almost all GUC are in positive form - so enable_connection_triggger is better

I don't think so current handling dathaslogontriggers is good for production usage. The protection against race condition can be solved by lock on pg_event_trigger

I thought about it, and probably the counter of connect triggers will be better there. The implementation will be simpler and more robust.

I prefer to implement different approach: unset dathaslogontriggers  flag in event trigger itself when no triggers are returned by EventTriggerCommonSetup.
I am using double checking to prevent race condition.
The main reason for this approach is that dropping of triggers is not done in event_trigger.c: it is done by generic code for all resources.
It seems to be there is no right place where decrementing of trigger counters can be inserted.
Also I prefer to keep all logon-trigger specific code in one file.

Also, as you suggested, I renamed disable_connection_trigger to enable_connection_trigger.


New version of the patch is attached.

yes, it can work

for complete review the new field in pg_doc should be documented

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

regress tests fails

     sysviews                     ... FAILED      112 ms

test event_trigger                ... FAILED (test process exited with exit code 2)      447 ms
test fast_default                 ... FAILED      392 ms
test stats                        ... FAILED      626 ms
============== shutting down postmaster               ==============

--

Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

út 22. 12. 2020 v 12:42 odesílatel Konstantin Knizhnik <k.knizhnik@postgrespro.ru> napsal:

On 22.12.2020 12:25, Pavel Stehule wrote:

regress tests fails

     sysviews                     ... FAILED      112 ms

test event_trigger                ... FAILED (test process exited with exit code 2)      447 ms
test fast_default                 ... FAILED      392 ms
test stats                        ... FAILED      626 ms
============== shutting down postmaster               ==============

Sorry, fixed.

no problem

I had to fix doc

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6ff783273f..7aded1848f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1008,7 +1008,7 @@ include_dir 'conf.d'
         trigger when a client connects. This parameter is switched on by default.
         Errors in trigger code can prevent user to login to the system.
         I this case disabling this parameter in connection string can solve the problem:
-        <literal>psql "dbname=postgres options='-c enable_client_connection_trigger=false'".
+        <literal>psql "dbname=postgres options='-c enable_client_connection_trigger=false'"</literal>.
        </para>
       </listitem>
      </varlistentry>

I am thinking again about enable_client_connection_trigger, and although it can look useless (because error is ignored for superuser), it can be useful for some debugging and administration purposes. Probably we don't want to start the client_connection trigger from backup tools, maybe from some monitoring tools. Maybe the possibility to set this GUC can be dedicated to some special role (like pg_signal_backend).

+     <varlistentry id="guc-enable-client-connection-trigger" xreflabel="enable_client_connection_trigger">
+      <term><varname>enable_client_connection_trigger</varname> (<type>boolean</type>)
+      <indexterm>
+       <primary><varname>enable_client_connection_trigger</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Enables firing the <literal>client_connection</literal>
+        trigger when a client connects. This parameter is switched on by default.
+        Errors in trigger code can prevent user to login to the system.
+        I this case disabling this parameter in connection string can solve the problem:
+        <literal>psql "dbname=postgres options='-c enable_client_connection_trigger=false'"</literal>.
+       </para>
+      </listitem>
+     </varlistentry>

There should be note, so only superuser can change this value

There is should be tab-complete support

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 3a43c09bf6..08f00d8fc4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2970,7 +2970,8 @@ psql_completion(const char *text, int start, int end)
        COMPLETE_WITH("ON");
    /* Complete CREATE EVENT TRIGGER <name> ON with event_type */
    else if (Matches("CREATE", "EVENT", "TRIGGER", MatchAny, "ON"))
-       COMPLETE_WITH("ddl_command_start", "ddl_command_end", "sql_drop");
+       COMPLETE_WITH("ddl_command_start", "ddl_command_end",
+                     "client_connection", "sql_drop");
 
    /*
     * Complete CREATE EVENT TRIGGER <name> ON <event_type>.  EXECUTE FUNCTION

Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

On Sat, Dec 26, 2020 at 4:04 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> so 26. 12. 2020 v 8:00 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
>>
>> Hi
>>
>>
>>> Thank you.
>>> I have applied all your fixes in on_connect_event_trigger-12.patch.
>>>
>>> Concerning enable_client_connection_trigger GUC, I think that it is really useful: it is the fastest and simplest
wayto disable login triggers in case 
>>> of some problems with them (not only for superuser itself, but for all users). Yes, it can be also done using
"ALTEREVENT TRIGGER DISABLE". 
>>> But assume that you have a lot of databases with different login policies enforced by on-login event triggers. And
youwant temporary disable them all, for example for testing purposes. 
>>> In this case GUC is most convenient way to do it.
>>>
>>
>> There was typo in patch
>>
>> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
>> index f810789..8861f1b 100644
>> --- a/doc/src/sgml/config.sgml
>> +++ b/doc/src/sgml/config.sgml
>> @@ -1,4 +1,4 @@
>> -<!-- doc/src/sgml/config.sgml -->
>> +\<!-- doc/src/sgml/config.sgml -->
>>
>> I have not any objections against functionality or design. I tested the performance, and there are no negative
impactswhen this feature is not used. There is significant overhead related to plpgsql runtime initialization, but when
thistrigger will be used, then probably some other PLpgSQL procedures and functions will be used too, and then this
overheadcan be ignored. 
>>
>> * make without warnings
>> * make check-world passed
>> * doc build passed
>>
>> Possible ToDo:
>>
>> The documentation can contain a note so usage connect triggers in environments with short life sessions and very
shortfast queries without usage PLpgSQL functions or procedures can have negative impact on performance due overhead of
initializationof PLpgSQL engine. 
>>
>> I'll mark this patch as ready for committers
>
>
> looks so this patch has not entry in commitfestapp 2021-01
>

Yeah, please register this patch before the next CommitFest[1] starts,
2021-01-01 AoE[2].

Regards,

[1] https://commitfest.postgresql.org/31/
[2] https://en.wikipedia.org/wiki/Anywhere_on_Earth

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

On Mon, Dec 28, 2020 at 5:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sat, Dec 26, 2020 at 4:04 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> >
> >
> >
> > so 26. 12. 2020 v 8:00 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
> >>
> >> Hi
> >>
> >>
> >>> Thank you.
> >>> I have applied all your fixes in on_connect_event_trigger-12.patch.
> >>>
> >>> Concerning enable_client_connection_trigger GUC, I think that it is really useful: it is the fastest and simplest
wayto disable login triggers in case 
> >>> of some problems with them (not only for superuser itself, but for all users). Yes, it can be also done using
"ALTEREVENT TRIGGER DISABLE". 
> >>> But assume that you have a lot of databases with different login policies enforced by on-login event triggers.
Andyou want temporary disable them all, for example for testing purposes. 
> >>> In this case GUC is most convenient way to do it.
> >>>
> >>
> >> There was typo in patch
> >>
> >> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> >> index f810789..8861f1b 100644
> >> --- a/doc/src/sgml/config.sgml
> >> +++ b/doc/src/sgml/config.sgml
> >> @@ -1,4 +1,4 @@
> >> -<!-- doc/src/sgml/config.sgml -->
> >> +\<!-- doc/src/sgml/config.sgml -->
> >>
> >> I have not any objections against functionality or design. I tested the performance, and there are no negative
impactswhen this feature is not used. There is significant overhead related to plpgsql runtime initialization, but when
thistrigger will be used, then probably some other PLpgSQL procedures and functions will be used too, and then this
overheadcan be ignored. 
> >>
> >> * make without warnings
> >> * make check-world passed
> >> * doc build passed
> >>
> >> Possible ToDo:
> >>
> >> The documentation can contain a note so usage connect triggers in environments with short life sessions and very
shortfast queries without usage PLpgSQL functions or procedures can have negative impact on performance due overhead of
initializationof PLpgSQL engine. 
> >>
> >> I'll mark this patch as ready for committers
> >
> >
> > looks so this patch has not entry in commitfestapp 2021-01
> >
>
> Yeah, please register this patch before the next CommitFest[1] starts,
> 2021-01-01 AoE[2].
>

Konstantin, did you register this patch in any CF? Even though the
reviewer seems to be happy with the patch, I am afraid that we might
lose track of this unless we register it.

--
With Regards,
Amit Kapila.

On Thu, Jan 28, 2021 at 8:17 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 28, 2020 at 5:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Sat, Dec 26, 2020 at 4:04 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > >
> > >
> > >
> > > so 26. 12. 2020 v 8:00 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
> > >>
> > >> Hi
> > >>
> > >>
> > >>> Thank you.
> > >>> I have applied all your fixes in on_connect_event_trigger-12.patch.
> > >>>
> > >>> Concerning enable_client_connection_trigger GUC, I think that it is really useful: it is the fastest and
simplestway to disable login triggers in case 
> > >>> of some problems with them (not only for superuser itself, but for all users). Yes, it can be also done using
"ALTEREVENT TRIGGER DISABLE". 
> > >>> But assume that you have a lot of databases with different login policies enforced by on-login event triggers.
Andyou want temporary disable them all, for example for testing purposes. 
> > >>> In this case GUC is most convenient way to do it.
> > >>>
> > >>
> > >> There was typo in patch
> > >>
> > >> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> > >> index f810789..8861f1b 100644
> > >> --- a/doc/src/sgml/config.sgml
> > >> +++ b/doc/src/sgml/config.sgml
> > >> @@ -1,4 +1,4 @@
> > >> -<!-- doc/src/sgml/config.sgml -->
> > >> +\<!-- doc/src/sgml/config.sgml -->
> > >>
> > >> I have not any objections against functionality or design. I tested the performance, and there are no negative
impactswhen this feature is not used. There is significant overhead related to plpgsql runtime initialization, but when
thistrigger will be used, then probably some other PLpgSQL procedures and functions will be used too, and then this
overheadcan be ignored. 
> > >>
> > >> * make without warnings
> > >> * make check-world passed
> > >> * doc build passed
> > >>
> > >> Possible ToDo:
> > >>
> > >> The documentation can contain a note so usage connect triggers in environments with short life sessions and very
shortfast queries without usage PLpgSQL functions or procedures can have negative impact on performance due overhead of
initializationof PLpgSQL engine. 
> > >>
> > >> I'll mark this patch as ready for committers
> > >
> > >
> > > looks so this patch has not entry in commitfestapp 2021-01
> > >
> >
> > Yeah, please register this patch before the next CommitFest[1] starts,
> > 2021-01-01 AoE[2].
> >
>
> Konstantin, did you register this patch in any CF? Even though the
> reviewer seems to be happy with the patch, I am afraid that we might
> lose track of this unless we register it.
>

I see the CF entry (https://commitfest.postgresql.org/31/2900/) for
this work. Sorry for the noise.

--
With Regards,
Amit Kapila.

On 28.01.2021 5:47, Amit Kapila wrote:
> On Mon, Dec 28, 2020 at 5:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> On Sat, Dec 26, 2020 at 4:04 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>>
>>>
>>> so 26. 12. 2020 v 8:00 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
>>>> Hi
>>>>
>>>>
>>>>> Thank you.
>>>>> I have applied all your fixes in on_connect_event_trigger-12.patch.
>>>>>
>>>>> Concerning enable_client_connection_trigger GUC, I think that it is really useful: it is the fastest and simplest
wayto disable login triggers in case
 
>>>>> of some problems with them (not only for superuser itself, but for all users). Yes, it can be also done using
"ALTEREVENT TRIGGER DISABLE".
 
>>>>> But assume that you have a lot of databases with different login policies enforced by on-login event triggers.
Andyou want temporary disable them all, for example for testing purposes.
 
>>>>> In this case GUC is most convenient way to do it.
>>>>>
>>>> There was typo in patch
>>>>
>>>> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
>>>> index f810789..8861f1b 100644
>>>> --- a/doc/src/sgml/config.sgml
>>>> +++ b/doc/src/sgml/config.sgml
>>>> @@ -1,4 +1,4 @@
>>>> -<!-- doc/src/sgml/config.sgml -->
>>>> +\<!-- doc/src/sgml/config.sgml -->
>>>>
>>>> I have not any objections against functionality or design. I tested the performance, and there are no negative
impactswhen this feature is not used. There is significant overhead related to plpgsql runtime initialization, but when
thistrigger will be used, then probably some other PLpgSQL procedures and functions will be used too, and then this
overheadcan be ignored.
 
>>>>
>>>> * make without warnings
>>>> * make check-world passed
>>>> * doc build passed
>>>>
>>>> Possible ToDo:
>>>>
>>>> The documentation can contain a note so usage connect triggers in environments with short life sessions and very
shortfast queries without usage PLpgSQL functions or procedures can have negative impact on performance due overhead of
initializationof PLpgSQL engine.
 
>>>>
>>>> I'll mark this patch as ready for committers
>>>
>>> looks so this patch has not entry in commitfestapp 2021-01
>>>
>> Yeah, please register this patch before the next CommitFest[1] starts,
>> 2021-01-01 AoE[2].
>>
> Konstantin, did you register this patch in any CF? Even though the
> reviewer seems to be happy with the patch, I am afraid that we might
> lose track of this unless we register it.
>
Yes, certainly:
https://commitfest.postgresql.org/31/2900/

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, May 20, 2021 at 2:45 PM Ivan Panchenko <wao@mail.ru> wrote:
>
> I have upgraded the patch for the 14th version.
>

I have some feedback on the patch:

(1) The patch adds 3 whitespace errors ("git apply <patch-file>"
reports 3 warnings)


(2) doc/src/sgml/catalogs.sgml

CURRENTLY:
This flag is used to avoid extra lookup of pg_event_trigger table on
each backend startup.

SUGGEST:
This flag is used to avoid extra lookups on the pg_event_trigger table
during each backend startup.


(3) doc/src/sgml/config.sgml

CURRENTLY:
Errors in trigger code can prevent user to login to the system.In this
case disabling this parameter in connection string can solve the
problem:

SUGGEST:
Errors in the trigger code can prevent a user from logging in to the
system. In this case, the parameter can be disabled in the connection
string, to allow the user to login:


(4) doc/src/sgml/event-trigger.sgml

(i)

CURRENTLY:
An event trigger fires whenever the event with which it is associated
occurs in the database in which it is defined. Currently, the only

SUGGEST:
An event trigger fires whenever an associated event occurs in the
database in which it is defined. Currently, the only


(ii)

CURRENTLY:
can be useful for client connections logging,

SUGGEST:
can be useful for logging client connections,


(5) src/backend/commands/event_trigger.c

(i) There are two instances of code blocks like:

  xxxx = table_open(...);
  tuple = SearchSysCacheCopy1(...);
  table_close(...);

These should end with: "heap_freetuple(tuple);"

(ii) Typo "nuder" and grammar:

CURRENTLY:
There can be race condition: event trigger may be added after we have
scanned pg_event_trigger table. Repeat this test nuder  pg_database
table lock.

SUGGEST:
There can be a race condition: the event trigger may be added after we
have scanned the pg_event_trigger table. Repeat this test under the
pg_database table lock.


(6) src/backend/utils/misc/postgresql.conf.sample

CURRENTLY:
+#enable_client_connection_trigger = true # enables firing the
client_connection trigger when a client connect

SUGGEST:
+#enable_client_connection_trigger = true # enables firing the
client_connection trigger when a client connects


Regards,
Greg Nancarrow
Fujitsu Australia

On Fri, May 21, 2021 at 2:46 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 2:45 PM Ivan Panchenko <wao@mail.ru> wrote:
> >
> > I have upgraded the patch for the 14th version.
> >
>
> I have some feedback on the patch:
>

I've attached an updated version of the patch.
I've applied my review comments and done a bit more tidying up of
documentation and comments.
Also, I found that the previously-posted patch was broken by
snapshot-handling changes in commit 84f5c290 (with patch applied,
resulting in a coredump during regression tests) so I've added a fix
for that too.

Regards,
Greg Nancarrow
Fujitsu Australia

On Thu, Jun 3, 2021 at 8:36 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Fri, May 21, 2021 at 2:46 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> >
> > On Thu, May 20, 2021 at 2:45 PM Ivan Panchenko <wao@mail.ru> wrote:
> > >
> > > I have upgraded the patch for the 14th version.
> > >
> >
> > I have some feedback on the patch:
> >
>
> I've attached an updated version of the patch.
> I've applied my review comments and done a bit more tidying up of
> documentation and comments.
> Also, I found that the previously-posted patch was broken by
> snapshot-handling changes in commit 84f5c290 (with patch applied,
> resulting in a coredump during regression tests) so I've added a fix
> for that too.

CFBot shows the following failure:
# poll_query_until timed out executing this query:
# SELECT '0/3046250' <= replay_lsn AND state = 'streaming' FROM
pg_catalog.pg_stat_replication WHERE application_name = 'standby_1';
# expecting this output:
# t
# last actual query output:
# t
# with stderr:
# NOTICE: You are welcome!
# Looks like your test exited with 29 before it could output anything.
t/001_stream_rep.pl ..................
Dubious, test returned 29 (wstat 7424, 0x1d00)

Regards,
Vignesh

On Sun, Jul 4, 2021 at 1:21 PM vignesh C <vignesh21@gmail.com> wrote:
>
> CFBot shows the following failure:
> # poll_query_until timed out executing this query:
> # SELECT '0/3046250' <= replay_lsn AND state = 'streaming' FROM
> pg_catalog.pg_stat_replication WHERE application_name = 'standby_1';
> # expecting this output:
> # t
> # last actual query output:
> # t
> # with stderr:
> # NOTICE: You are welcome!
> # Looks like your test exited with 29 before it could output anything.
> t/001_stream_rep.pl ..................
> Dubious, test returned 29 (wstat 7424, 0x1d00)
>

Thanks.
I found that the patch was broken by commit f452aaf7d (the part
"adjust poll_query_until to insist on empty stderr as well as a stdout
match").
So I had to remove a "RAISE NOTICE" (which was just an informational
message) from the login trigger function, to satisfy the new
poll_query_until expectations.
Also, I updated a PG14 version check (now must check PG15 version).

Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, Jul 19, 2021 at 8:18 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>
> The patch does not apply, and rebase is required.
>

Attached a rebased patch (minor updates to the test code).

Regards,
Greg Nancarrow
Fujitsu Australia

Среда, 7 июля 2021, 3:55 +03:00 от Greg Nancarrow <gregn4422@gmail.com>:
 

On Sun, Jul 4, 2021 at 1:21 PM vignesh C <vignesh21@gmail.com> wrote:

>
> CFBot shows the following failure:
> # poll_query_until timed out executing this query:
> # SELECT '0/3046250' <= replay_lsn AND state = 'streaming' FROM
> pg_catalog.pg_stat_replication WHERE application_name = 'standby_1';
> # expecting this output:
> # t
> # last actual query output:
> # t
> # with stderr:
> # NOTICE: You are welcome!
> # Looks like your test exited with 29 before it could output anything.
> t/001_stream_rep.pl ..................
> Dubious, test returned 29 (wstat 7424, 0x1d00)
>

Thanks.
I found that the patch was broken by commit f452aaf7d (the part
"adjust poll_query_until to insist on empty stderr as well as a stdout
match").
So I had to remove a "RAISE NOTICE" (which was just an informational
message) from the login trigger function, to satisfy the new
poll_query_until expectations.

Also, I updated a PG14 version check (now must check PG15 version).

Regards,
Greg Nancarrow
Fujitsu Australia

On Tue, Aug 17, 2021 at 1:11 AM Ivan Panchenko <wao@mail.ru> wrote:
>
> Does it mean that "RAISE NOTICE" should’t be used, or behaves unexpectedly in logon triggers? Should we mention this
inthe docs? 
>

No I don't believe so, it was just that that part of the test
framework (sub poll_query_until) had been changed to regard anything
output to stderr as an error (so now for the test to succeed, whatever
is printed to stdout must match the expected test output, and stderr
must be empty).

Regards,
Greg Nancarrow
Fujitsu Australia

> On 19 Jul 2021, at 15:25, Greg Nancarrow <gregn4422@gmail.com> wrote:

> Attached a rebased patch (minor updates to the test code).

I took a look at this, and while I like the proposed feature I think the patch
has a bit more work required.


+    END IF;
+
+-- 2) Initialize some user session data
+   CREATE TEMP TABLE session_storage (x float, y integer);
The example in the documentation use a mix of indentations, neither of which is
the 2-space indentation used elsewhere in the docs.


+    /* Get the command tag. */
+    tag = parsetree ? CreateCommandTag(parsetree) : CMDTAG_CONNECT;
This is hardcoding knowledge that currently only this trigger wont have a
parsetree, and setting the tag accordingly.  This should instead check the
event and set CMDTAG_UNKNOWN if it isn't the expected one.


+    /* database has on-login triggers */
+    bool        dathaslogontriggers;
This patch uses three different names for the same thing: client connection,
logontrigger and login trigger.  Since this trigger only fires after successful
authentication it’s not accurate to name it client connection, as that implies
it running on connections rather than logins.  The nomenclature used in the
tree is "login" so the patch should be adjusted everywhere to use that.


+/* Hook for plugins to get control at start of session */
+client_connection_hook_type client_connection_hook = EventTriggerOnConnect;
I don't see the reason for adding core functionality by hooks.  Having a hook
might be a useful thing on its own (to be discussed in a new thread, not hidden
here), but core functionality should not depend on it IMO.


+    {"enable_client_connection_trigger", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
+        gettext_noop("Enables the client_connection event trigger."),
+        gettext_noop("In case of errors in the ON client_connection EVENT TRIGGER procedure, "
 ..and..
+    /*
+     * Try to ignore error for superuser to make it possible to login even in case of errors
+     * during trigger execution
+     */
+    if (!is_superuser)
+        PG_RE_THROW();
This patch adds two ways for superusers to bypass this event trigger in case of
it being faulty, but for every other event trigger we've documented to restart
in single-user mode and fixing it there.  Why does this need to be different?
This clearly has a bigger chance of being a footgun but I don't see that as a
reason to add a GUC and a bypass that other footguns lack.


+    elog(NOTICE, "client_connection trigger failed with message: %s", error->message);
Calling elog() in a PG_CATCH() block isn't allowed is it?


+    /* Runtlist is empty: clear dathaslogontriggers flag
+     */
s/Runtlist/Runlist/ and also commenting style.


The logic for resetting the pg_database flag when firing event trigger in case
the trigger has gone away is messy and a problem waiting to happen IMO.  For
normal triggers we don't bother with that on the grounds of it being racy, and
instead perform relhastrigger removal during VACUUM.  Another approach is using
a counter as propose upthread, since updating that can be made safer.  The
current coding also isn't instructing concurrent backends to perform relcache
rebuild.

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

> On 8 Sep 2021, at 16:02, Pavel Stehule <pavel.stehule@gmail.com> wrote:

> In the time when event triggers were introduced, managed services were not too widely used like now. When we
discussedthis feature we thought about environments when users have no superuser rights and have no possibility to go
tosingle mode. 

In situations where you don't have superuser access and cannot restart in
single user mode, none of the bypasses in this patch would help anyways.

I understand the motivation, but continuing on even in the face of an
ereport(ERROR..  ) in the hopes of being able to turn off buggy code seems
pretty unsafe at best.

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

On Wed, Sep 8, 2021 at 10:56 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> I took a look at this, and while I like the proposed feature I think the patch
> has a bit more work required.
>

Thanks for reviewing the patch.
I am not the original patch author (who no longer seems active) but
I've been contributing a bit and keeping the patch alive because I
think it's a worthwhile feature.

>
> +
> +-- 2) Initialize some user session data
> +   CREATE TEMP TABLE session_storage (x float, y integer);
> The example in the documentation use a mix of indentations, neither of which is
> the 2-space indentation used elsewhere in the docs.
>

Fixed, using 2-space indentation.
(to be honest, the indentation seems inconsistent elsewhere in the
docs e.g. I'm seeing a nearby case of 2-space on 1st indent, 6-space
on 2nd indent)

>
> +       /* Get the command tag. */
> +       tag = parsetree ? CreateCommandTag(parsetree) : CMDTAG_CONNECT;
> This is hardcoding knowledge that currently only this trigger wont have a
> parsetree, and setting the tag accordingly.  This should instead check the
> event and set CMDTAG_UNKNOWN if it isn't the expected one.
>

I updated that, but maybe not exactly how you expected?

>
> +       /* database has on-login triggers */
> +       bool            dathaslogontriggers;
> This patch uses three different names for the same thing: client connection,
> logontrigger and login trigger.  Since this trigger only fires after successful
> authentication it’s not accurate to name it client connection, as that implies
> it running on connections rather than logins.  The nomenclature used in the
> tree is "login" so the patch should be adjusted everywhere to use that.
>

Fixed.

>
> +/* Hook for plugins to get control at start of session */
> +client_connection_hook_type client_connection_hook = EventTriggerOnConnect;
> I don't see the reason for adding core functionality by hooks.  Having a hook
> might be a useful thing on its own (to be discussed in a new thread, not hidden
> here), but core functionality should not depend on it IMO.
>

Fair enough, I removed the hook.

>
> +       {"enable_client_connection_trigger", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
> +               gettext_noop("Enables the client_connection event trigger."),
> +               gettext_noop("In case of errors in the ON client_connection EVENT TRIGGER procedure, "
>  ..and..
> +       /*
> +        * Try to ignore error for superuser to make it possible to login even in case of errors
> +        * during trigger execution
> +        */
> +       if (!is_superuser)
> +               PG_RE_THROW();
> This patch adds two ways for superusers to bypass this event trigger in case of
> it being faulty, but for every other event trigger we've documented to restart
> in single-user mode and fixing it there.  Why does this need to be different?
> This clearly has a bigger chance of being a footgun but I don't see that as a
> reason to add a GUC and a bypass that other footguns lack.
>

OK, I removed those bypasses. We'll see what others think.

>
> +       elog(NOTICE, "client_connection trigger failed with message: %s", error->message);
> Calling elog() in a PG_CATCH() block isn't allowed is it?
>

I believe it is allowed (e.g. there's a case in libpq), but I removed
this anyway as part of the superuser bypass.

>
> +       /* Runtlist is empty: clear dathaslogontriggers flag
> +        */
> s/Runtlist/Runlist/ and also commenting style.
>

Fixed.

>
> The logic for resetting the pg_database flag when firing event trigger in case
> the trigger has gone away is messy and a problem waiting to happen IMO.  For
> normal triggers we don't bother with that on the grounds of it being racy, and
> instead perform relhastrigger removal during VACUUM.  Another approach is using
> a counter as propose upthread, since updating that can be made safer.  The
> current coding also isn't instructing concurrent backends to perform relcache
> rebuild.
>

I think there are pros and cons of each possible approach, but I think
I prefer the current way (which I have tweaked a bit) for similar
reasons to those explained by the original patch author when debating
whether to use reference-counting instead, in the discussion upthread
(e.g. it keeps it all in one place). Also, it seems to be more inline
with the documented reason why that pg_database flag was added in the
first place. I have debugged a few concurrent scenarios with the
current mechanism in place. If you still dislike the logic for
resetting the flag, please elaborate on the issues you foresee and one
of the alternative approaches can be tried.

I've attached the updated patch.

Regards,
Greg Nancarrow
Fujitsu Australia

Hi!

Nice feature, but, sorry, I see some design problem in suggested feature. AFAIK, 
there is two use cases for this feature:
1 A permission / prohibition to login some users
2 Just a logging of facts of user's login

Suggested patch proposes prohibition of login only by failing of login trigger 
and it has at least two issues:
1 In case of prohibition to login, there is no clean way to store information 
about unsuccessful  login. Ok, it could be solved by dblink module but it seems 
to scary.
2 For logging purpose failing of trigger will cause impossibility to login, it 
could be workarounded by catching error in trigger function, but it's not so 
obvious for DBA.

some other issues:
3 It's easy to create security hole, see attachment where non-privileged user 
can close access to database even for superuser.
4 Feature is not applicable for logging unsuccessful login with wrong 
username/password or not matched by pg_hba.conf. Oracle could operate with such 
cases. But I don't think that this issue is a stopper for the patch.

May be, to solve that issues we could introduce return value of trigger and/or 
add something like IGNORE ERROR to CREATE EVENT TRIGGER command.

On 15.09.2021 16:32, Greg Nancarrow wrote:
> On Wed, Sep 8, 2021 at 10:56 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>> I took a look at this, and while I like the proposed feature I think the patch
>> has a bit more work required.
>>
> 
> Thanks for reviewing the patch.
> I am not the original patch author (who no longer seems active) but
> I've been contributing a bit and keeping the patch alive because I
> think it's a worthwhile feature.
> 
>>
>> +
>> +-- 2) Initialize some user session data
>> +   CREATE TEMP TABLE session_storage (x float, y integer);
>> The example in the documentation use a mix of indentations, neither of which is
>> the 2-space indentation used elsewhere in the docs.
>>
> 
> Fixed, using 2-space indentation.
> (to be honest, the indentation seems inconsistent elsewhere in the
> docs e.g. I'm seeing a nearby case of 2-space on 1st indent, 6-space
> on 2nd indent)
> 
>>
>> +       /* Get the command tag. */
>> +       tag = parsetree ? CreateCommandTag(parsetree) : CMDTAG_CONNECT;
>> This is hardcoding knowledge that currently only this trigger wont have a
>> parsetree, and setting the tag accordingly.  This should instead check the
>> event and set CMDTAG_UNKNOWN if it isn't the expected one.
>>
> 
> I updated that, but maybe not exactly how you expected?
> 
>>
>> +       /* database has on-login triggers */
>> +       bool            dathaslogontriggers;
>> This patch uses three different names for the same thing: client connection,
>> logontrigger and login trigger.  Since this trigger only fires after successful
>> authentication it’s not accurate to name it client connection, as that implies
>> it running on connections rather than logins.  The nomenclature used in the
>> tree is "login" so the patch should be adjusted everywhere to use that.
>>
> 
> Fixed.
> 
>>
>> +/* Hook for plugins to get control at start of session */
>> +client_connection_hook_type client_connection_hook = EventTriggerOnConnect;
>> I don't see the reason for adding core functionality by hooks.  Having a hook
>> might be a useful thing on its own (to be discussed in a new thread, not hidden
>> here), but core functionality should not depend on it IMO.
>>
> 
> Fair enough, I removed the hook.
> 
>>
>> +       {"enable_client_connection_trigger", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
>> +               gettext_noop("Enables the client_connection event trigger."),
>> +               gettext_noop("In case of errors in the ON client_connection EVENT TRIGGER procedure, "
>>   ..and..
>> +       /*
>> +        * Try to ignore error for superuser to make it possible to login even in case of errors
>> +        * during trigger execution
>> +        */
>> +       if (!is_superuser)
>> +               PG_RE_THROW();
>> This patch adds two ways for superusers to bypass this event trigger in case of
>> it being faulty, but for every other event trigger we've documented to restart
>> in single-user mode and fixing it there.  Why does this need to be different?
>> This clearly has a bigger chance of being a footgun but I don't see that as a
>> reason to add a GUC and a bypass that other footguns lack.
>>
> 
> OK, I removed those bypasses. We'll see what others think.
> 
>>
>> +       elog(NOTICE, "client_connection trigger failed with message: %s", error->message);
>> Calling elog() in a PG_CATCH() block isn't allowed is it?
>>
> 
> I believe it is allowed (e.g. there's a case in libpq), but I removed
> this anyway as part of the superuser bypass.
> 
>>
>> +       /* Runtlist is empty: clear dathaslogontriggers flag
>> +        */
>> s/Runtlist/Runlist/ and also commenting style.
>>
> 
> Fixed.
> 
>>
>> The logic for resetting the pg_database flag when firing event trigger in case
>> the trigger has gone away is messy and a problem waiting to happen IMO.  For
>> normal triggers we don't bother with that on the grounds of it being racy, and
>> instead perform relhastrigger removal during VACUUM.  Another approach is using
>> a counter as propose upthread, since updating that can be made safer.  The
>> current coding also isn't instructing concurrent backends to perform relcache
>> rebuild.
>>
> 
> I think there are pros and cons of each possible approach, but I think
> I prefer the current way (which I have tweaked a bit) for similar
> reasons to those explained by the original patch author when debating
> whether to use reference-counting instead, in the discussion upthread
> (e.g. it keeps it all in one place). Also, it seems to be more inline
> with the documented reason why that pg_database flag was added in the
> first place. I have debugged a few concurrent scenarios with the
> current mechanism in place. If you still dislike the logic for
> resetting the flag, please elaborate on the issues you foresee and one
> of the alternative approaches can be tried.
> 
> I've attached the updated patch.
> 
> Regards,
> Greg Nancarrow
> Fujitsu Australia
> 

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

On Wed, Sep 29, 2021 at 10:14 PM Teodor Sigaev <teodor@sigaev.ru> wrote:
>
> Nice feature, but, sorry, I see some design problem in suggested feature. AFAIK,
> there is two use cases for this feature:
> 1 A permission / prohibition to login some users
> 2 Just a logging of facts of user's login
>
> Suggested patch proposes prohibition of login only by failing of login trigger
> and it has at least two issues:
> 1 In case of prohibition to login, there is no clean way to store information
> about unsuccessful  login. Ok, it could be solved by dblink module but it seems
> to scary.

It's an area that could be improved, but the patch is more intended to
allow additional actions on successful login, as described by the
following (taken from the doc updates in the patch):

+   <para>
+    The event trigger on the <literal>login</literal> event can be
+    useful for logging user logins, for verifying the connection and
+    assigning roles according to current circumstances, or for some
session data
+    initialization.
+   </para>

> 2 For logging purpose failing of trigger will cause impossibility to login, it
> could be workarounded by catching error in trigger function, but it's not so
> obvious for DBA.
>

If you look back on the past discussions on this thread, you'll see
that originally the patch added a GUC for the purpose of allowing
superusers to disable the event trigger, to allow login in this case.
However it was argued that there was already a documented way of
bypassing buggy event triggers (i.e. restart in single-user mode), so
that GUC was removed.
Also previously in the patch, login trigger errors for superusers were
ignored in order to allow them to login in this case, but it was
argued that it could well be unsafe to continue on after an error, so
that too was removed.
See below:

>
> +       {"enable_client_connection_trigger", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
> +               gettext_noop("Enables the client_connection event trigger."),
> +               gettext_noop("In case of errors in the ON client_connection EVENT TRIGGER procedure, "
>  ..and..
> +       /*
> +        * Try to ignore error for superuser to make it possible to login even in case of errors
> +        * during trigger execution
> +        */
> +       if (!is_superuser)
> +               PG_RE_THROW();
> This patch adds two ways for superusers to bypass this event trigger in case of
> it being faulty, but for every other event trigger we've documented to restart
> in single-user mode and fixing it there.  Why does this need to be different?
> This clearly has a bigger chance of being a footgun but I don't see that as a
> reason to add a GUC and a bypass that other footguns lack.
>

So I really don't think that a failing event trigger will cause an
"impossibility to login".
The patch updates the documentation to explain the use of single-user
mode to fix buggy login event triggers. See below:

+   <para>
+     The <literal>login</literal> 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 <xref linkend="app-postgres"/> reference page for details about
+     using single-user mode.
+   </para>
+

Regards,
Greg Nancarrow
Fujitsu Australia

> On 30 Sep 2021, at 04:15, Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Wed, Sep 29, 2021 at 10:14 PM Teodor Sigaev <teodor@sigaev.ru> wrote:
>>
>> Nice feature, but, sorry, I see some design problem in suggested feature. AFAIK,
>> there is two use cases for this feature:
>> 1 A permission / prohibition to login some users
>> 2 Just a logging of facts of user's login
>>
>> Suggested patch proposes prohibition of login only by failing of login trigger
>> and it has at least two issues:
>> 1 In case of prohibition to login, there is no clean way to store information
>> about unsuccessful  login. Ok, it could be solved by dblink module but it seems
>> to scary.
>
> It's an area that could be improved, but the patch is more intended to
> allow additional actions on successful login, as described by the
> following (taken from the doc updates in the patch):
>
> +   <para>
> +    The event trigger on the <literal>login</literal> event can be
> +    useful for logging user logins, for verifying the connection and
> +    assigning roles according to current circumstances, or for some
> session data
> +    initialization.
> +   </para>

Running user code with potential side effects on unsuccessful logins also open
up the risk for (D)DoS attacks.

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

On Wed, Sep 15, 2021 at 11:32 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> I've attached the updated patch.
>

Attached a rebased version of the patch, as it was broken by fairly
recent changes (only very minor change to the previous version).

Regards,
Greg Nancarrow
Fujitsu Australia

> On 3 Nov 2021, at 17:15, Ivan Panchenko <wao@mail.ru> wrote:
> Среда, 29 сентября 2021, 15:14 +03:00 от Teodor Sigaev <teodor@sigaev.ru <x-msg://33/compose?To=teodor@sigaev.ru>>:
> 2 For logging purpose failing of trigger will cause impossibility to login, it
> could be workarounded by catching error in trigger function, but it's not so
> obvious for DBA.
> If the trigger contains an error, nobody can login. The database is bricked.
> Previous variant of the patch proposed to fix this with going to single-user mode.
> For faster recovery I propose to have also a GUC variable to turn on/off the login triggers.
> It should be 'on' by default.

As voiced earlier, I disagree with this and I dislike the idea of punching a
hole for circumventing infrastructure intended for auditing.

Use-cases for a login-trigger commonly involve audit trail logging, session
initialization etc.  If the login trigger bricks the production database to the
extent that it needs to be restarted with the magic GUC, then it's highly
likely that you *don't* want regular connections to the database for the
duration of this.  Any such connection won't be subject to what the trigger
does which seem counter to having the trigger in the first place.  This means
that it's likely that the superuser fixing it will have to disable logins for
everyone else while fixing, and it quicly becomes messy.

With that in mind, I think single-user mode actually *helps* the users in this
case, and we avoid a hole punched which in worst case can be a vector for an
attack.

Maybe I'm overly paranoid, but adding a backdoor of sorts for a situation which
really shouldn't happen doesn't seem like a good idea.
> some other issues:
> 3 It's easy to create security hole, see attachment where non-privileged user
> can close access to database even for superuser.
> Such cases can be avoided by careful design of the login triggers and related permissions. Added such note to the
documentation.

If all login triggers are written carefully like that, we don't need the GUC to
disable them?

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

On Fri, Nov 5, 2021 at 3:03 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> +1
> The arguments given are pretty convincing IMHO, and I agree that the additions made in the v20 patch are not a good
idea,and are not needed.
 
>

If there are no objections, I plan to reinstate the previous v19 patch
(as v21), perhaps with a few minor improvements and cleanups (e.g. SQL
capitalization) in the tests, as hinted at in the v20 patch, but no
new functionality.

Regards,
Greg Nancarrow
Fujitsu Australia

> On 10 Nov 2021, at 08:12, Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Fri, Nov 5, 2021 at 3:03 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>>
>> +1
>> The arguments given are pretty convincing IMHO, and I agree that the additions made in the v20 patch are not a good
idea,and are not needed. 
>
> If there are no objections, I plan to reinstate the previous v19 patch
> (as v21), perhaps with a few minor improvements and cleanups (e.g. SQL
> capitalization) in the tests, as hinted at in the v20 patch, but no
> new functionality.

No objections from me. Small nitpicks from the v19 patch:

+        This flag is used internally by Postgres and should not be manually changed by DBA or application.
This should be <productname>PostgreSQL</productname>.

+     * There can be a race condition: a login event trigger may have
..
+    /* Fire any defined login triggers, if appropriate */
The patch say "login trigger" in most places, and "login event trigger" in a
few places.  We should settle for a single nomenclature, and I think "login
event trigger" is the best option.

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

On Wed, Nov 10, 2021 at 8:11 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > If there are no objections, I plan to reinstate the previous v19 patch
> > (as v21), perhaps with a few minor improvements and cleanups (e.g. SQL
> > capitalization) in the tests, as hinted at in the v20 patch, but no
> > new functionality.
>
> No objections from me. Small nitpicks from the v19 patch:
>
> +        This flag is used internally by Postgres and should not be manually changed by DBA or application.
> This should be <productname>PostgreSQL</productname>.
>
> +        * There can be a race condition: a login event trigger may have
> ..
> +       /* Fire any defined login triggers, if appropriate */
> The patch say "login trigger" in most places, and "login event trigger" in a
> few places.  We should settle for a single nomenclature, and I think "login
> event trigger" is the best option.
>

I've attached an updated patch, that essentially reinstates the v19
patch, but with a few improvements such as:
- Updates to address nitpicks (Daniel Gustafsson)
- dathaslogintriggers -> dathasloginevttriggers flag rename (too
long?) and remove its restoration in pg_dump output, since it's not
needed (as in v20 patch)
- Some tidying of the updates to the event_trigger tests and
capitalization of the test SQL

Regards,
Greg Nancarrow
Fujitsu Australia

> On 11 Nov 2021, at 07:37, Greg Nancarrow <gregn4422@gmail.com> wrote:

> I've attached an updated patch, that essentially reinstates the v19
> patch

Thanks!  I've only skimmed it so far but it looks good, will do a more thorough
review soon.

+        This flag is used to avoid extra lookups on the pg_event_trigger table during each backend startup.
This should be <structname>pg_event_trigger</structname>.  Sorry, missed that
one at that last read-through.

> - dathaslogintriggers -> dathasloginevttriggers flag rename (too
> long?)

I'm not crazy about this name, "evt" is commonly the abbreviation of "event
trigger" (used in evtcache.c etc) so "dathasloginevt" would IMO be better.
That being said, that's still not a very readable name, maybe someone else has
an even better suggestion?

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

On Thu, Nov 11, 2021 at 8:56 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> +        This flag is used to avoid extra lookups on the pg_event_trigger table during each backend startup.
> This should be <structname>pg_event_trigger</structname>.  Sorry, missed that
> one at that last read-through.
>
Thanks, noted.

> > - dathaslogintriggers -> dathasloginevttriggers flag rename (too
> > long?)
>
> I'm not crazy about this name, "evt" is commonly the abbreviation of "event
> trigger" (used in evtcache.c etc) so "dathasloginevt" would IMO be better.
> That being said, that's still not a very readable name, maybe someone else has
> an even better suggestion?
>
Yes you're right, in this case I was wrongly treating "evt" as an
abbreviation for "event".
I agree "dathasloginevt" would be better.


Regards,
Greg Nancarrow
Fujitsu Australia

On Thu, Nov 11, 2021 at 8:56 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> +        This flag is used to avoid extra lookups on the pg_event_trigger table during each backend startup.
> This should be <structname>pg_event_trigger</structname>.  Sorry, missed that
> one at that last read-through.
>
Fixed.

> > - dathaslogintriggers -> dathasloginevttriggers flag rename (too
> > long?)
>
> I'm not crazy about this name, "evt" is commonly the abbreviation of "event
> trigger" (used in evtcache.c etc) so "dathasloginevt" would IMO be better.
> That being said, that's still not a very readable name, maybe someone else has
> an even better suggestion?
>
Changed to "dathasloginevt", as suggested.

I've attached an updated patch with these changes.
I also noticed one of the Windows-based cfbots was failing with an
"SSPI authentication failed for user" error, so I updated the test
code for that.

Regards,
Greg Nancarrow
Fujitsu Australia

On Tue, Nov 16, 2021 at 4:38 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> I've attached an updated patch with these changes.
>

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

Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, Dec 6, 2021 at 12:10 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> I've attached a re-based version (no functional changes from the
> previous) to fix cfbot failures.
>

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

Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, Jan 24, 2022 at 1:59 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> I've attached a re-based version (no functional changes from the
> previous) to fix cfbot failures.
>

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

Regards,
Greg Nancarrow
Fujitsu Australia

> On 15 Feb 2022, at 11:07, Greg Nancarrow <gregn4422@gmail.com> 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 <productname>PostgreSQL</productname> and should not be manually changed by
DBAor application. 
I think we should reword this to something like "This flag is used internally
by <productname>PostgreSQL</productname> 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, &tuple->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",
+                          &trigdata);
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/

On Tue, Feb 15, 2022 at 5:07 AM Greg Nancarrow <gregn4422@gmail.com> 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(10000000); 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

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.


> +   <para>
> +     The <literal>login</literal> 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 <xref linkend="app-postgres"/> reference page for details about
> +     using single-user mode.
> +   </para>

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;
> +        Relation    pg_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, &tuple->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",
> +                                          &trigdata);
> +
> +        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
> +             */
> +            Relation    pg_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
> +                 * table lock.
> +                 */
> +                AcceptInvalidationMessages();
> +                runlist = EventTriggerCommonSetup(NULL,
> +                                                  EVT_Login, "login",
> +                                                  &trigdata);

This doesn't work. RowExclusiveLock doesn't conflict with another
RowExclusiveLock.

What is the AcceptInvalidationMessages() intending to do here?


> +                if (runlist == NULL)    /* list is still empty, so clear the
> +                                         * flag */
> +                {
> +                    db->dathasloginevt = false;
> +                    CatalogTupleUpdate(pg_db, &tuple->t_self, tuple);
> +                }
> +                else
> +                    CacheInvalidateRelcacheByTuple(tuple);

Copy of the bogus relcache stuff.


Greetings,

Andres Freund

> On 12 Mar 2022, at 03:46, Andres Freund <andres@anarazel.de> wrote:

>> +   <para>
>> +     The <literal>login</literal> 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 <xref linkend="app-postgres"/> reference page for details about
>> +     using single-user mode.
>> +   </para>
>
> 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=<event|all>' GUC perhaps?

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

Hi,

On 2022-03-13 23:31:03 +0100, Daniel Gustafsson wrote:
> > On 12 Mar 2022, at 03:46, Andres Freund <andres@anarazel.de> wrote:
> 
> >> +   <para>
> >> +     The <literal>login</literal> 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 <xref linkend="app-postgres"/> reference page for details about
> >> +     using single-user mode.
> >> +   </para>
> > 
> > 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=<event|all>' 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

Andres Freund <andres@anarazel.de> writes:
> On 2022-03-13 23:31:03 +0100, Daniel Gustafsson wrote:
>> Something like a '-c ignore_event_trigger=<event|all>' 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

Hi,

On 2022-03-13 19:57:08 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-03-13 23:31:03 +0100, Daniel Gustafsson wrote:
> >> Something like a '-c ignore_event_trigger=<event|all>' 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

Andres Freund <andres@anarazel.de> 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

On 2022-03-13 20:35:44 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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.

> On 14 Mar 2022, at 00:33, Andres Freund <andres@anarazel.de> 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/

On Sun, Mar 13, 2022 at 7:34 PM Andres Freund <andres@anarazel.de> 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

> On 14 Mar 2022, at 01:08, Andres Freund <andres@anarazel.de> 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/

Hi,

On 2022-03-14 14:47:09 +0100, Daniel Gustafsson wrote:
> > On 14 Mar 2022, at 01:08, Andres Freund <andres@anarazel.de> 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

> On 14 Mar 2022, at 17:10, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-03-14 14:47:09 +0100, Daniel Gustafsson wrote:
>>> On 14 Mar 2022, at 01:08, Andres Freund <andres@anarazel.de> 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/

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

> On 18 Mar 2022, at 08:24, a.sokolov@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/

Hi,

On 2022-03-18 17:03:39 +0100, Daniel Gustafsson wrote:
> > On 18 Mar 2022, at 08:24, a.sokolov@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

> On 22 Mar 2022, at 04:48, Andres Freund <andres@anarazel.de> 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/

Daniel Gustafsson писал 2022-03-22 11:43:
>> On 22 Mar 2022, at 04:48, Andres Freund <andres@anarazel.de> 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
+    <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? 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

Hi,

On 2022-03-28 15:57:37 +0300, a.sokolov@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.


> 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

> On 28 Mar 2022, at 19:10, Andres Freund <andres@anarazel.de> wrote:
> On 2022-03-28 15:57:37 +0300, a.sokolov@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.

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

Hi,

On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
> > On 28 Mar 2022, at 19:10, Andres Freund <andres@anarazel.de> wrote:
> > On 2022-03-28 15:57:37 +0300, a.sokolov@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.


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

> On 28 Mar 2022, at 23:31, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
>>> On 28 Mar 2022, at 19:10, Andres Freund <andres@anarazel.de> wrote:
>>> On 2022-03-28 15:57:37 +0300, a.sokolov@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.

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/

Andres Freund <andres@anarazel.de> 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

Tue, March 29, 2022, 0:31 +03:00 from Andres Freund <andres@anarazel.de>:
 

Hi,

On 2022-03-28 23:27:56 +0200, Daniel Gustafsson wrote:
> > On 28 Mar 2022, at 19:10, Andres Freund <andres@anarazel.de> wrote:
> > On 2022-03-28 15:57:37 +0300, a.sokolov@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.

- 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

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;
$$;

\c 
SELECT dathasloginevt FROM pg_database WHERE datname= :'DBNAME';

 dathasloginevt 
---------------- f
(1 row)

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

> On 30 Mar 2022, at 13:21, Ivan Panchenko <wao@mail.ru> 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
throwingthe 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/

> On 29 Mar 2022, at 00:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andres Freund <andres@anarazel.de> 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/

Daniel Gustafsson писал 2022-03-30 16:48:
>> On 30 Mar 2022, at 13:21, Ivan Panchenko <wao@mail.ru> 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

> On 1 Apr 2022, at 09:16, a.sokolov@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/

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/

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 Shinderuk        https://postgrespro.com/

> On 20 Sep 2022, at 15:43, Sergey Shinderuk <s.shinderuk@postgrespro.ru> 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/

On 20.09.2022 17:10, Daniel Gustafsson wrote:
>> On 20 Sep 2022, at 15:43, Sergey Shinderuk <s.shinderuk@postgrespro.ru> 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 Shinderuk        https://postgrespro.com/

> On 20 Sep 2022, at 16:10, Daniel Gustafsson <daniel@yesql.se> 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/

2022年11月4日(金) 5:23 Daniel Gustafsson <daniel@yesql.se>:
>
> > On 20 Sep 2022, at 16:10, Daniel Gustafsson <daniel@yesql.se> 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