On login trigger: take three

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:

/messages/by-id/1570308356720-0.post@n3.nabble.com

/messages/by-id/OSAPR01MB507373499CCCEA00EAE79875FE2D0@OSAPR01MB5073.jpnprd01.prod.outlook.com

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

Pavel

Example

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

On 03.09.2020 17:18, Pavel Stehule wrote:

Hi

čt 3. 9. 2020 v 15:43 odesílatel Konstantin Knizhnik
<k.knizhnik@postgrespro.ru <mailto: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:

/messages/by-id/1570308356720-0.post@n3.nabble.com
/messages/by-id/OSAPR01MB507373499CCCEA00EAE79875FE2D0@OSAPR01MB5073.jpnprd01.prod.outlook.com

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

I realized that on connect trigger should be implemented as EVENT TRIGGER.
So I have reimplemented my patch using event trigger and use
session_start even name to make it more consistent with other events.
Now on login triggers can be created in this way:

create table connects(id serial, who text);
create function on_login_proc() returns event_trigger as $$
begin
  insert into connects (who) values (current_user());
  raise notice 'You are welcome!';
end;
$$ language plpgsql;
create event trigger on_login_trigger on session_start execute procedure
on_login_proc();
alter event trigger on_login_trigger enable always;

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

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

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

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.

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.

Regards

Pavel

On 14.09.2020 17:34, Pavel Stehule wrote:

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.

I agree.
I can rename trigger to backend_start or process_start or whatever else.

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.

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

po 14. 9. 2020 v 17:53 odesílatel Konstantin Knizhnik <
k.knizhnik@postgrespro.ru> napsal:

On 14.09.2020 17:34, Pavel Stehule wrote:

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.

I agree.
I can rename trigger to backend_start or process_start or whatever else.

Creating a good name can be hard - it is not called for any process - so
maybe "user_backend_start" ?

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.

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 custom libpq variables can be the
stop for people that use pgAdmin.

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

út 8. 12. 2020 v 1:17 odesílatel Greg Nancarrow <gregn4422@gmail.com>
napsal:

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.

yes, it can work .. Maybe for this operation only database owner rights
should be necessary. The super user is maybe too strong.

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

Regards

Pavel

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.

Regards,
Greg Nancarrow
Fujitsu Australia

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

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

Pavel

On 09.12.2020 15:34, Pavel Stehule wrote:

Hi

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

On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule
<pavel.stehule@gmail.com <mailto: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.
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".

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

On 10.12.2020 10:45, Pavel Stehule wrote:

st 9. 12. 2020 v 14:28 odesílatel Konstantin Knizhnik
<k.knizhnik@postgrespro.ru <mailto: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 <mailto:gregn4422@gmail.com>> napsal:

On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule
<pavel.stehule@gmail.com <mailto: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

Unfortunately I was not able to reproduce your results.
I just tried the case "select 1" because for this trivial query the
overhead of session hooks should be the largest.
In my case variation of results was large enough.
Did you try to run test multiple times?

pgbench -j 2 -c 10 --connect -f test-ro.sql -T 60 -n postgres

disable_event_triggers = off
tps = 1812.250463 (including connections establishing)
tps = 2256.285712 (excluding connections establishing)

tps = 1838.107242 (including connections establishing)
tps = 2288.507668 (excluding connections establishing)

tps = 1830.879302 (including connections establishing)
tps = 2279.302553 (excluding connections establishing)

disable_event_triggers = on
tps = 1858.328717 (including connections establishing)
tps = 2313.661689 (excluding connections establishing)

tps = 1832.932960 (including connections establishing)
tps = 2282.074346 (excluding connections establishing)

tps = 1868.908521 (including connections establishing)
tps = 2326.559150 (excluding connections establishing)

I tried to increase run time to 1000 seconds.
Results are the following:

disable_event_triggers = off
tps = 1813.587876 (including connections establishing)
tps = 2257.844703 (excluding connections establishing)

disable_event_triggers = on
tps = 1838.107242 (including connections establishing)
tps = 2288.507668 (excluding connections establishing)

So the difference on this extreme case is 1%.

I tried your suggestion and move client_connection_hook invocation to
postinit.c
It slightly improve performance:

tps = 1828.446959 (including connections establishing)
tps = 2276.142972 (excluding connections establishing)

So result is somewhere in the middle, because it allows to eliminate
overhead of
starting transaction or taking snapshots but not of lookup through
pg_event_trigger table (although it it empty).

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