Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Sat, Nov 11, 2017 at 6:48 AM, Michael Paquier 
wrote:
>
> On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
>  wrote:
> > New version attached.
>
> Thanks.
>
> +++ b/src/test/modules/Makefile
>   test_extensions \
> + test_session_hooks \
>   test_parser
> Better if that's in alphabetical order.
>

Fixed.


> That's a nit though, so I am switching the patch as ready for committer.
>

Thank you so much for your review.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..7246552 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -15,6 +15,7 @@ SUBDIRS = \
 		  test_pg_dump \
 		  test_rbtree \
 		  test_rls_hooks \
+		  test_session_hooks \
 		  test_shm_mq \
 		  worker_spi
 
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRESS = test_session_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_session_hooks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_session_hooks/README b/src/test/modules/test_session_hooks/README
new file mode 100644
index 000..9cb4202
--- /dev/null
+++ b/src/test/modules/test_session_hooks/README
@@ -0,0 +1,2 @@
+test_session_hooks is an example of how to use session start and end
+hooks.
diff --git a/src/test/modules/test_session_hooks/expected/test_session_hooks.out b/src/test/modules/test_session_hooks/expected/test_session_hooks.out
new file mode 

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
 wrote:
> New version attached.

Thanks.

+++ b/src/test/modules/Makefile
  test_extensions \
+ test_session_hooks \
  test_parser
Better if that's in alphabetical order.

That's a nit though, so I am switching the patch as ready for committer.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Nov 9, 2017 at 9:08 PM, Michael Paquier 
wrote:
>
> On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello
>  wrote:
> > On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <
michael.paqu...@gmail.com>
> > wrote:
> >> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
> >> @@ -0,0 +1 @@
> >> +shared_preload_libraries = 'test_session_hooks'
> >> Don't you think that this should be a GUC? My previous comment
> >> outlined that. I won't fight hard on that point in any case, don't
> >> worry. I just want to make things clear :)
> >
> > Ooops... my fault... fixed!
> >
> > Thanks again!!
>
> +/* GUC variables */
> +static char *session_hook_username = NULL;
> This causes the module to crash if test_session_hooks.username is not
> set. I would recommend just assigning a default value, say "postgres".
>

Fixed.

New version attached.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -11,6 +11,7 @@ SUBDIRS = \
 		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
+		  test_session_hooks \
 		  test_parser \
 		  test_pg_dump \
 		  test_rbtree \
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRESS = test_session_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_session_hooks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git 

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello
 wrote:
> On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier 
> wrote:
>> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
>> @@ -0,0 +1 @@
>> +shared_preload_libraries = 'test_session_hooks'
>> Don't you think that this should be a GUC? My previous comment
>> outlined that. I won't fight hard on that point in any case, don't
>> worry. I just want to make things clear :)
>
> Ooops... my fault... fixed!
>
> Thanks again!!

+/* GUC variables */
+static char *session_hook_username = NULL;
This causes the module to crash if test_session_hooks.username is not
set. I would recommend just assigning a default value, say "postgres".
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier 
wrote:
>
> On Thu, Nov 9, 2017 at 2:42 AM, Fabrízio de Royes Mello
>  wrote:
> > On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <
michael.paqu...@gmail.com>
> > wrote:
> >> - Let's restrict the logging to a role name instead of a database
> >> name, and let's parametrize it with a setting in the temporary
> >> configuration file. Let's not bother about multiple role support with
> >> a list, for the sake of tests and simplicity only defining one role
> >> looks fine to me. Comments in the code should be clear about the
> >> dependency.
> >
> > Makes sense and simplify the test code. Fixed.
>
> +   if (!strcmp(username, "regress_sess_hook_usr2"))
> +   {
> +   const char *dbname;
> [...]
> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
> @@ -0,0 +1 @@
> +shared_preload_libraries = 'test_session_hooks'
> Don't you think that this should be a GUC? My previous comment
> outlined that. I won't fight hard on that point in any case, don't
> worry. I just want to make things clear :)
>

Ooops... my fault... fixed!

Thanks again!!

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -11,6 +11,7 @@ SUBDIRS = \
 		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
+		  test_session_hooks \
 		  test_parser \
 		  test_pg_dump \
 		  test_rbtree \
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRESS = test_session_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) 

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Thu, Nov 9, 2017 at 2:42 AM, Fabrízio de Royes Mello
 wrote:
> On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier 
> wrote:
>> - Let's restrict the logging to a role name instead of a database
>> name, and let's parametrize it with a setting in the temporary
>> configuration file. Let's not bother about multiple role support with
>> a list, for the sake of tests and simplicity only defining one role
>> looks fine to me. Comments in the code should be clear about the
>> dependency.
>
> Makes sense and simplify the test code. Fixed.

+   if (!strcmp(username, "regress_sess_hook_usr2"))
+   {
+   const char *dbname;
[...]
+++ b/src/test/modules/test_session_hooks/session_hooks.conf
@@ -0,0 +1 @@
+shared_preload_libraries = 'test_session_hooks'
Don't you think that this should be a GUC? My previous comment
outlined that. I won't fight hard on that point in any case, don't
worry. I just want to make things clear :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier 
wrote:
>
> +   /* Hook just normal backends */
> +   if (session_end_hook && MyBackendId != InvalidBackendId)
> +   (*session_end_hook) ();
> I have been wondering about the necessity of this restriction.
> Couldn't it be useful to just let the people implementing the hook do
> any decision-making? Tracking some non-backend shutdowns may be useful
> as well for logging purposes.
>

Also makes sense... I move down this check to hook code.


> The patch is beginning to take shape (I really like the test module
> you are implementing here!), still needs a bit more work.
>

Thanks... and your review is helping a lot!!!


> +CREATE ROLE session_hook_usr1 LOGIN;
> +CREATE ROLE session_hook_usr2 LOGIN;
> Roles created during regression tests should be prefixed with regress_.
>

Fixed.


> +   if (prev_session_start_hook)
> +   prev_session_start_hook();
> +
> +   if (session_start_hook_enabled)
> +   (void) register_session_hook("START");
> Shouldn't both be reversed?
>

Fixed.


> +/* sample sessoin end hook function */
> Typo here.
>

Fixed.


> +CREATE DATABASE session_hook_db;
> [...]
> +   if (!strcmp(dbname, "session_hook_db"))
> +   {
> Creating a database is usually avoided in sql files as those can be
> run on existing servers using installcheck. I am getting that this
> restriction is in place so as it is possible to create an initial
> connection to the server so as the base table to log the activity is
> created. That's a fine assumption to me. Still, I think that the
> following changes should be done:
> - Let's restrict the logging to a role name instead of a database
> name, and let's parametrize it with a setting in the temporary
> configuration file. Let's not bother about multiple role support with
> a list, for the sake of tests and simplicity only defining one role
> looks fine to me. Comments in the code should be clear about the
> dependency.

Makes sense and simplify the test code. Fixed.


> - The GUCs test_session_hooks.start_enabled and
> test_session_hooks.end_enabled are actually redundant with the
> database restriction (well, role restriction per previous comment), so
> let's remove them. Please let's also avoid ALTER SYSTEM calls in tests
> as it would impact existing installations with installcheck.
>

Also simplify the test code. Fixed.


> Impossible to tell committer's feeling about this test suite, but my
> opinion is to keep it as that's a good template example about what can
> be done with those two hooks.
>

+1

Attached new version.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..52a9641 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT 

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Tue, Nov 7, 2017 at 9:58 PM, Fabrízio de Royes Mello
 wrote:
> On Tue, Nov 7, 2017 at 1:15 AM, Michael Paquier 
> wrote:
>> On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
>>  wrote:
>> I was going to to hack something like that. That's interesting for the
>> use case Robert has mentioned.
>>
>> Well, in the case of the end session hook, those variables are passed
>> to the hook by being directly taken from the context from MyProcPort:
>> +   (*session_end_hook) (MyProcPort->database_name,
>> MyProcPort->user_name);
>> In the case of the start hook, those are directly taken from the
>> command outer caller, but similarly MyProcPort is already set, so
>> those are strings available (your patch does so in the end session
>> hook)... Variables in hooks are useful if those are not available
>> within the memory context, and refer to a specific state where the
>> hook is called. For example, take the password hook, this uses the
>> user name and the password because those values are not available
>> within the session context. The same stands for other hooks as well.
>> Keeping the interface minimal helps in readability and maintenance.
>> See for the attached example that can be applied on top of 0003, which
>> makes use of the session context, the set regression tests does not
>> pass but this shows how I think those hooks had better be shaped.
>>
>
> Makes sense... fixed.

Thanks for considering it.

>> +   (*session_end_hook) (MyProcPort->database_name,
>> MyProcPort->user_name);
>> +
>> +   /* Make don't leave any active transactions and/or locks behind */
>> +   AbortOutOfAnyTransaction();
>> +   LockReleaseAll(USER_LOCKMETHOD, true);
>> Let's leave this work to people actually implementing the hook contents.
>>
>
> Fixed.
>
> Attached new version. I unify all three patches in just one because this is
> a small patch and it's most part is test code.

Thanks. This makes sense to me.

+   /* Hook just normal backends */
+   if (session_end_hook && MyBackendId != InvalidBackendId)
+   (*session_end_hook) ();
I have been wondering about the necessity of this restriction.
Couldn't it be useful to just let the people implementing the hook do
any decision-making? Tracking some non-backend shutdowns may be useful
as well for logging purposes.

The patch is beginning to take shape (I really like the test module
you are implementing here!), still needs a bit more work.

+CREATE ROLE session_hook_usr1 LOGIN;
+CREATE ROLE session_hook_usr2 LOGIN;
Roles created during regression tests should be prefixed with regress_.

+   if (prev_session_start_hook)
+   prev_session_start_hook();
+
+   if (session_start_hook_enabled)
+   (void) register_session_hook("START");
Shouldn't both be reversed?

+/* sample sessoin end hook function */
Typo here.

+CREATE DATABASE session_hook_db;
[...]
+   if (!strcmp(dbname, "session_hook_db"))
+   {
Creating a database is usually avoided in sql files as those can be
run on existing servers using installcheck. I am getting that this
restriction is in place so as it is possible to create an initial
connection to the server so as the base table to log the activity is
created. That's a fine assumption to me. Still, I think that the
following changes should be done:
- Let's restrict the logging to a role name instead of a database
name, and let's parametrize it with a setting in the temporary
configuration file. Let's not bother about multiple role support with
a list, for the sake of tests and simplicity only defining one role
looks fine to me. Comments in the code should be clear about the
dependency.
- The GUCs test_session_hooks.start_enabled and
test_session_hooks.end_enabled are actually redundant with the
database restriction (well, role restriction per previous comment), so
let's remove them. Please let's also avoid ALTER SYSTEM calls in tests
as it would impact existing installations with installcheck.

Impossible to tell committer's feeling about this test suite, but my
opinion is to keep it as that's a good template example about what can
be done with those two hooks.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Tue, Nov 7, 2017 at 1:15 AM, Michael Paquier 
wrote:
>
> On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
>  wrote:
> > On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier <
michael.paqu...@gmail.com>
> > wrote:
> >> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
> >>  wrote:
> >> >> Passing the database name and user name does not look much useful to
> >> >> me. You can have access to this data already with CurrentUserId and
> >> >> MyDatabaseId.
> >> >
> >> > This way we don't need to convert oid to names inside hook code.
> >>
> >> Well, arguments of hooks are useful if they are used. Now if I look at
> >> the two examples mainly proposed in this thread, be it in your set of
> >> patches or the possibility to do some SQL transaction, I see nothing
> >> using them. So I'd vote for keeping an interface minimal.
> >>
> >
> > Maybe the attached patch with improved test module can illustrate
better the
> > feature.
>
> I was going to to hack something like that. That's interesting for the
> use case Robert has mentioned.
>
> Well, in the case of the end session hook, those variables are passed
> to the hook by being directly taken from the context from MyProcPort:
> +   (*session_end_hook) (MyProcPort->database_name,
> MyProcPort->user_name);
> In the case of the start hook, those are directly taken from the
> command outer caller, but similarly MyProcPort is already set, so
> those are strings available (your patch does so in the end session
> hook)... Variables in hooks are useful if those are not available
> within the memory context, and refer to a specific state where the
> hook is called. For example, take the password hook, this uses the
> user name and the password because those values are not available
> within the session context. The same stands for other hooks as well.
> Keeping the interface minimal helps in readability and maintenance.
> See for the attached example that can be applied on top of 0003, which
> makes use of the session context, the set regression tests does not
> pass but this shows how I think those hooks had better be shaped.
>

Makes sense... fixed.


> +   (*session_end_hook) (MyProcPort->database_name,
MyProcPort->user_name);
> +
> +   /* Make don't leave any active transactions and/or locks behind */
> +   AbortOutOfAnyTransaction();
> +   LockReleaseAll(USER_LOCKMETHOD, true);
> Let's leave this work to people actually implementing the hook contents.
>

Fixed.

Attached new version. I unify all three patches in just one because this is
a small patch and it's most part is test code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..52a9641 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..ffaf51f 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook just normal backends */
+	if (session_end_hook && MyBackendId != InvalidBackendId)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
 wrote:
> On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier 
> wrote:
>> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
>>  wrote:
>> >> Passing the database name and user name does not look much useful to
>> >> me. You can have access to this data already with CurrentUserId and
>> >> MyDatabaseId.
>> >
>> > This way we don't need to convert oid to names inside hook code.
>>
>> Well, arguments of hooks are useful if they are used. Now if I look at
>> the two examples mainly proposed in this thread, be it in your set of
>> patches or the possibility to do some SQL transaction, I see nothing
>> using them. So I'd vote for keeping an interface minimal.
>>
>
> Maybe the attached patch with improved test module can illustrate better the
> feature.

I was going to to hack something like that. That's interesting for the
use case Robert has mentioned.

Well, in the case of the end session hook, those variables are passed
to the hook by being directly taken from the context from MyProcPort:
+   (*session_end_hook) (MyProcPort->database_name,
MyProcPort->user_name);
In the case of the start hook, those are directly taken from the
command outer caller, but similarly MyProcPort is already set, so
those are strings available (your patch does so in the end session
hook)... Variables in hooks are useful if those are not available
within the memory context, and refer to a specific state where the
hook is called. For example, take the password hook, this uses the
user name and the password because those values are not available
within the session context. The same stands for other hooks as well.
Keeping the interface minimal helps in readability and maintenance.
See for the attached example that can be applied on top of 0003, which
makes use of the session context, the set regression tests does not
pass but this shows how I think those hooks had better be shaped.

+   (*session_end_hook) (MyProcPort->database_name, MyProcPort->user_name);
+
+   /* Make don't leave any active transactions and/or locks behind */
+   AbortOutOfAnyTransaction();
+   LockReleaseAll(USER_LOCKMETHOD, true);
Let's leave this work to people actually implementing the hook contents.
-- 
Michael


session_hook_simplify.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier 
wrote:
>
> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
>  wrote:
> >> Passing the database name and user name does not look much useful to
> >> me. You can have access to this data already with CurrentUserId and
> >> MyDatabaseId.
> >
> > This way we don't need to convert oid to names inside hook code.
>
> Well, arguments of hooks are useful if they are used. Now if I look at
> the two examples mainly proposed in this thread, be it in your set of
> patches or the possibility to do some SQL transaction, I see nothing
> using them. So I'd vote for keeping an interface minimal.
>

Maybe the attached patch wit improved test module can illustrate better the
feature.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..24346fb 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) (dbname, username);
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..d349592 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,11 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start of session */
+typedef void (*session_start_hook_type) (const char *dbname,
+		 const char *username);
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..029eeb4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,16 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook just normal backends */
+	if (session_end_hook && MyBackendId != InvalidBackendId)
+	{
+		(*session_end_hook) (MyProcPort->database_name, MyProcPort->user_name);
+
+		/* Make don't leave any active transactions and/or locks behind */
+		AbortOutOfAnyTransaction();
+		LockReleaseAll(USER_LOCKMETHOD, true);
+	}
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index d349592..b7fb8c3 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,10 +35,14 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start and end of session */
 typedef void (*session_start_hook_type) (const char *dbname,
 		 const char *username);
+typedef void (*session_end_hook_type) (const char *dbname,
+	   const char *username);
+
 extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
 
 /* GUC-configurable parameters */
 
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -11,6 +11,7 @@ SUBDIRS = \
 		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
+		  test_session_hooks \
 		  test_parser \
 		  test_pg_dump \
 		  test_rbtree \
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile 

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
 wrote:
>> Passing the database name and user name does not look much useful to
>> me. You can have access to this data already with CurrentUserId and
>> MyDatabaseId.
>
> This way we don't need to convert oid to names inside hook code.

Well, arguments of hooks are useful if they are used. Now if I look at
the two examples mainly proposed in this thread, be it in your set of
patches or the possibility to do some SQL transaction, I see nothing
using them. So I'd vote for keeping an interface minimal.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Nov 3, 2017 at 11:43 AM, Aleksandr Parfenov <
a.parfe...@postgrespro.ru> wrote:
>
> README file in patch 0003 is a copy of README from test_pg_dump module
> without any changes.
>

Thanks, I'll fix it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Nov 3, 2017 at 11:19 AM, Michael Paquier 
wrote:
>
>  /*
> + * Setup handler to session end hook
> + */
> +if (IsUnderPostmaster)
> +on_proc_exit(do_session_end_hook, 0);
> I think that it would be better to place that in ShutdownPostgres.
> This way it is possible to take actions before any resource is shut
> down.
>

Hmmm... ok but I have some doubt... ShutdownPostgres make sure to abort any
active transaction (AbortOutOfAnyTransaction) and release all locks
(LockReleaseAll). If we hook session end at this point we'll do that after
this two steps and after that run again... something like:

...
/* Make sure we've killed any active transaction */
AbortOutOfAnyTransaction();

/*
 * User locks are not released by transaction end, so be sure to release
 * them explicitly.
 */
LockReleaseAll(USER_LOCKMETHOD, true);

if (session_end_hook) {
(*session_end_hook) (port->database_name, port->user_name);

/* Make sure session end hook doesn't leave trash behind */
AbortOutOfAnyTransaction();
LockReleaseAll(USER_LOCKMETHOD, true);
}
...


> Passing the database name and user name does not look much useful to
> me. You can have access to this data already with CurrentUserId and
> MyDatabaseId.
>

This way we don't need to convert oid to names inside hook code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Aleksandr Parfenov]

README file in patch 0003 is a copy of README from test_pg_dump module
without any changes.

-- 
Aleksandr Parfenov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Thu, Nov 2, 2017 at 11:36 PM, Fabrízio de Royes Mello
 wrote:
> On Thu, Nov 2, 2017 at 5:42 AM, Aleksandr Parfenov
>  wrote:
>> Unfortunately, patches 0001 and 0002 don't apply to current master.
>>
>> The new status of this patch is: Waiting on Author
>
> Thanks for your review. Rebased versions attached.

Looking at this thread, there are clearly arguments in favor of having
a session hook after authentication. One use case mentioned by Robert
is inserting data into a table when a user logs in. I can imagine that
something like that could be applied to a session ending.

 /*
+ * Setup handler to session end hook
+ */
+if (IsUnderPostmaster)
+on_proc_exit(do_session_end_hook, 0);
I think that it would be better to place that in ShutdownPostgres.
This way it is possible to take actions before any resource is shut
down.

Passing the database name and user name does not look much useful to
me. You can have access to this data already with CurrentUserId and
MyDatabaseId.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Nov 2, 2017 at 5:42 AM, Aleksandr Parfenov <
a.parfe...@postgrespro.ru> wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   not tested
> Spec compliant:   not tested
> Documentation:not tested
>
> Hi,
>
> Unfortunately, patches 0001 and 0002 don't apply to current master.
>
> The new status of this patch is: Waiting on Author
>

Thanks for your review. Rebased versions attached.

Regards,


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2c7260e..24346fb 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) (dbname, username);
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..d349592 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,11 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start of session */
+typedef void (*session_start_hook_type) (const char *dbname,
+		 const char *username);
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 24346fb..0dbbd7b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,9 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start or end of session */
 session_start_hook_type session_start_hook = NULL;
-
+session_end_hook_type session_end_hook = NULL;
 /* 
  *		decls for routines only used in this file
  * 
@@ -196,6 +196,7 @@ static void drop_unnamed_stmt(void);
 static void log_disconnections(int code, Datum arg);
 static void enable_statement_timeout(void);
 static void disable_statement_timeout(void);
+static void do_session_end_hook(int code, Datum arg);
 
 
 /* 
@@ -3864,6 +3865,12 @@ PostgresMain(int argc, char *argv[],
 		(*session_start_hook) (dbname, username);
 
 	/*
+	 * Setup handler to session end hook
+	 */
+	if (IsUnderPostmaster)
+		on_proc_exit(do_session_end_hook, 0);
+
+	/*
 	 * POSTGRES main processing loop begins here
 	 *
 	 * If an exception is encountered, processing resumes here so we abort the
@@ -4615,3 +4622,15 @@ disable_statement_timeout(void)
 		stmt_timeout_active = false;
 	}
 }
+
+/*
+ * on_proc_exit handler to call session end hook
+ */
+static void
+do_session_end_hook(int code, Datum arg)
+{
+	Port	   *port = MyProcPort;
+
+	if (session_end_hook)
+		(*session_end_hook) (port->database_name, port->user_name);
+}
\ No newline at end of file
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index d349592..b7fb8c3 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,10 +35,14 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start and end of session */
 typedef void (*session_start_hook_type) (const char *dbname,
 		 const char *username);
+typedef void (*session_end_hook_type) (const char *dbname,
+	   const char *username);
+
 extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
 
 /* GUC-configurable parameters */
 
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ 

Re: [HACKERS] [PATCH] A hook for session start

[Aleksandr Parfenov]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi,

Unfortunately, patches 0001 and 0002 don't apply to current master.

The new status of this patch is: Waiting on Author

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Robert Haas]

On Fri, Oct 6, 2017 at 3:36 PM, Nico Williams  wrote:
>> If global temporary tables should be effective, then you have not have
>> modify system catalogue after creating. But lot of processes requires it -
>> ANALYZE, query planning.
>
> But the nice thing about them is that you need only create them once, so
> leave them in the catalog.  Stats about them should not be gathered nor
> stored, since they could be different per-session.

The topic of this thread seems to have drifted quite far from the
subject line, but here are some links to previous discussions of
global temporary tables.

https://www.postgresql.org/message-id/flat/u2o603c8f071004231952i36642ae6u9d6a7eae6eb6ff32%40mail.gmail.com
https://www.postgresql.org/message-id/AANLkTim=5af41BKFvZ=ofvj465kxqkjdjhqzudz3k...@mail.gmail.com
https://www.postgresql.org/message-id/flat/CAFj8pRC2h6qhHsFbcE7b_7SagiS6o%3D5J2UvCwCb05Ka1XFv_Ng%40mail.gmail.com
https://www.postgresql.org/message-id/flat/CA%2B0EDdACCx8w4nK-wdj-eodbJn4juChnHoUWVMM3u3uhLVPnJw%40mail.gmail.com

I would strongly recommend reading through those carefully before
undertaking anything here.  This is not a straightforward project...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Pavel Stehule]

2017-10-07 6:49 GMT+02:00 Nico Williams :

> On Sat, Oct 07, 2017 at 05:44:00AM +0200, Pavel Stehule wrote:
> > 2017-10-06 21:36 GMT+02:00 Nico Williams :
> > > But the nice thing about them is that you need only create them once,
> so
> > > leave them in the catalog.  Stats about them should not be gathered nor
> > > stored, since they could be different per-session.
> >
> > Unfortunately one field from pg_class are not static - reltuples should
> be
> > per session.
>
> It's "only an estimate" "used by the query planner".  We could estimate
> zero for global temp tables, and the query planner can get the true
> value from an internal temp table.
>

It can be solution.


> > But it can be moved to different table
>
> That too, if it's OK.
>
> Nico
> --
>

Re: [HACKERS] [PATCH] A hook for session start

[Nico Williams]

On Sat, Oct 07, 2017 at 05:44:00AM +0200, Pavel Stehule wrote:
> 2017-10-06 21:36 GMT+02:00 Nico Williams :
> > But the nice thing about them is that you need only create them once, so
> > leave them in the catalog.  Stats about them should not be gathered nor
> > stored, since they could be different per-session.
> 
> Unfortunately one field from pg_class are not static - reltuples should be
> per session.

It's "only an estimate" "used by the query planner".  We could estimate
zero for global temp tables, and the query planner can get the true
value from an internal temp table.

> But it can be moved to different table

That too, if it's OK.

Nico
-- 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Pavel Stehule]

2017-10-06 21:36 GMT+02:00 Nico Williams :

> On Fri, Oct 06, 2017 at 08:51:53PM +0200, Pavel Stehule wrote:
> > 2017-10-06 20:39 GMT+02:00 Nico Williams :
> > > On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> > > > When we talked about this topic, there are two issues:
> > > >
> > > > a) probably not too hard issue - some internal data can be in
> session sys
> > > > cache.
> > > >
> > > > b) the session sys data should be visible on SQL level too (for some
> > > tools
> > > > and consistency) - it is hard task.
> > >
> > > Can you expand on this?
> >
> > If global temporary tables should be effective, then you have not have
> > modify system catalogue after creating. But lot of processes requires it
> -
> > ANALYZE, query planning.
>
> But the nice thing about them is that you need only create them once, so
> leave them in the catalog.  Stats about them should not be gathered nor
> stored, since they could be different per-session.
>

Unfortunately one field from pg_class are not static - reltuples should be
per session.

But it can be moved to different table

Regards

Pavel

Re: [HACKERS] [PATCH] A hook for session start

[Nico Williams]

On Fri, Oct 06, 2017 at 08:51:53PM +0200, Pavel Stehule wrote:
> 2017-10-06 20:39 GMT+02:00 Nico Williams :
> > On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> > > When we talked about this topic, there are two issues:
> > >
> > > a) probably not too hard issue - some internal data can be in session sys
> > > cache.
> > >
> > > b) the session sys data should be visible on SQL level too (for some
> > tools
> > > and consistency) - it is hard task.
> >
> > Can you expand on this?
> 
> If global temporary tables should be effective, then you have not have
> modify system catalogue after creating. But lot of processes requires it -
> ANALYZE, query planning.

But the nice thing about them is that you need only create them once, so
leave them in the catalog.  Stats about them should not be gathered nor
stored, since they could be different per-session.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Pavel Stehule]

2017-10-06 20:39 GMT+02:00 Nico Williams :

> On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> > 2017-10-06 6:48 GMT+02:00 Nico Williams :
> > > On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> > > > Current TEMP tables, if you do it for any session has pretty
> significant
> > > > overhead  - with possible risk of performance lost (system catalog
> > > bloat).
> > >
> > > Because of the DDLs for them?
> >
> > yes - pg_attribute, pg_class, pg_stats are bloating - and when these
> tables
> > are bloated, then DDL is slow.
>
> :(
>
> > > No, I want GLOBAL TEMP tables.
> >
> > me too :) - and lot of customer and users.
>
> > I though about it, but I have other on my top priority. GLOBAL TEMP TABLE
> > is on 90% unlogged table. But few fields should be session based instead
> > shared persistent - statistics, rows in pg_class, filenode.
>
> Unlogged tables don't provide isolation between sessions the way temp
> tables do, so I don't see the connection.
>
> But the necessary components (temp heaps and such) are all there, and I
> suspect a PoC could be done fairly quickly.  But there are some
> subtleties like that FKs between GLOBAL TEMP and persistent tables must
> not be allowed (in either direction), so a complete implementation will
> take significant work.
>
> The work looks like:
>
>  - add syntax (trivial)
>
>  - add new kind of persistence (lots of places to touch, but it's mostly
>mechanical)
>
>  - redirect all references to global temp table contents to temp
>heaps/indexes/whatever
>
>  - add logic to prevent FKs between persistent and global temp tables
>
>  - what else?
>
> > When we talked about this topic, there are two issues:
> >
> > a) probably not too hard issue - some internal data can be in session sys
> > cache.
> >
> > b) the session sys data should be visible on SQL level too (for some
> tools
> > and consistency) - it is hard task.
>
> Can you expand on this?
>

If global temporary tables should be effective, then you have not have
modify system catalogue after creating. But lot of processes requires it -
ANALYZE, query planning.

>
> Nico
> --
>

Re: [HACKERS] [PATCH] A hook for session start

[Nico Williams]

On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> 2017-10-06 6:48 GMT+02:00 Nico Williams :
> > On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> > > Current TEMP tables, if you do it for any session has pretty significant
> > > overhead  - with possible risk of performance lost (system catalog
> > bloat).
> >
> > Because of the DDLs for them?
> 
> yes - pg_attribute, pg_class, pg_stats are bloating - and when these tables
> are bloated, then DDL is slow.

:(

> > No, I want GLOBAL TEMP tables.
> 
> me too :) - and lot of customer and users.

> I though about it, but I have other on my top priority. GLOBAL TEMP TABLE
> is on 90% unlogged table. But few fields should be session based instead
> shared persistent - statistics, rows in pg_class, filenode.

Unlogged tables don't provide isolation between sessions the way temp
tables do, so I don't see the connection.

But the necessary components (temp heaps and such) are all there, and I
suspect a PoC could be done fairly quickly.  But there are some
subtleties like that FKs between GLOBAL TEMP and persistent tables must
not be allowed (in either direction), so a complete implementation will
take significant work.

The work looks like:

 - add syntax (trivial)

 - add new kind of persistence (lots of places to touch, but it's mostly
   mechanical)

 - redirect all references to global temp table contents to temp
   heaps/indexes/whatever

 - add logic to prevent FKs between persistent and global temp tables

 - what else?

> When we talked about this topic, there are two issues:
> 
> a) probably not too hard issue - some internal data can be in session sys
> cache.
> 
> b) the session sys data should be visible on SQL level too (for some tools
> and consistency) - it is hard task.

Can you expand on this?

Nico
-- 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Pavel Stehule]

2017-10-06 6:48 GMT+02:00 Nico Williams :

> On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> > 2017-10-05 22:31 GMT+02:00 Nico Williams :
> > > On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> > > > On 7/21/17 13:14, Jim Mlodgenski wrote:
> > > > > When I first saw this thread, my initial thought of a use case is
> to
> > > > > prepare some key application queries so they are there and ready
> to go.
> > > > > That would need to be before the ExecutorStart_hook or
> > > > > ProcessUtility_hook if an app would just want to execute the
> prepared
> > > > > statement.
> > > >
> > > > Isn't that what the preprepare extension does already?
> > >
> > > more generic facility -> more useful
> > >
> > > My use case is to pre-create TEMP schema elements that VIEWs,
> FUNCTIONs,
> > > and TRIGGERs, might need.
> >
> > It is better to work on GLOBAL TEMP tables.
>
> I don't disagree.
>
> In fact, I was scoping out what it might take to do that just yesterday.
>
> I've too thoughts on that: either a new relpersistence kind that is very
> similar to persistent, but which always uses temp heaps, or a modifier
> for the persistent kind that says to use temp heaps.  Either way it
> looks like it should be fairly straightforward (but then, i've only
> ever written one thing for PG, earlier this week, the ALWAYS DEFERRED
> thing).
>
> > Current TEMP tables, if you do it for any session has pretty significant
> > overhead  - with possible risk of performance lost (system catalog
> bloat).
>
> Because of the DDLs for them?
>

yes - pg_attribute, pg_class, pg_stats are bloating - and when these tables
are bloated, then DDL is slow.



> > So often creating local temp tables is antipattern (in Postgres)
> > unfortunately.
>
> I do it plenty, but sometimes I use an UNLOGGED table with a txid column
> in the PK set to txid_current(), then I clean up where I can.  It'd be
> nice to have COMMIT triggers for cleaning up such rows, among other
> things.  I've implemented that using DDL event triggers, but to perform
> well it needs to be a native feature.
>
> > I am not sure, if we should to support this case more :( Probably is
> > better, so it is hard to use local TEMP tables.
>
> No, I want GLOBAL TEMP tables.
>

me too :) - and lot of customer and users.

There is a workaround - you can use a array instead temp tables in 50%. But
it is not a solution in other 50%.

I though about it, but I have other on my top priority. GLOBAL TEMP TABLE
is on 90% unlogged table. But few fields should be session based instead
shared persistent - statistics, rows in pg_class, filenode.

When we talked about this topic, there are two issues:

a) probably not too hard issue - some internal data can be in session sys
cache.

b) the session sys data should be visible on SQL level too (for some tools
and consistency) - it is hard task.

Regards

Pavel


> Nico
> --
>

Re: [HACKERS] [PATCH] A hook for session start

[Nico Williams]

On Fri, Oct 06, 2017 at 11:04:38AM +0800, Craig Ringer wrote:
> On 6 October 2017 at 10:52, Pavel Stehule  wrote:
> 
> > It is better to work on GLOBAL TEMP tables.
> >
> > Current TEMP tables, if you do it for any session has pretty significant
> > overhead  - with possible risk of performance lost (system catalog bloat).
> >
> > pretty significant performance issue of my customers are related to temp
> > tables usage (under high load)
> 
> I've seen the same thing too. Especially when combined with logical
> decoding, where IIRC we mark transactions as having catalog changes
> due to temp tables.
> 
> Sometimes the catalog bloat can be truly horrible when a user has
> hundreds of plpgsql functions that all like to make temp tables.

I agree that we should have GLOBAL TEMP tables, but also we should have
a pg_temp_catalog where all TEMP schema elements go...  (That, I'm sure,
would be a lot of work.)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Nico Williams]

On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> 2017-10-05 22:31 GMT+02:00 Nico Williams :
> > On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> > > On 7/21/17 13:14, Jim Mlodgenski wrote:
> > > > When I first saw this thread, my initial thought of a use case is to
> > > > prepare some key application queries so they are there and ready to go.
> > > > That would need to be before the ExecutorStart_hook or
> > > > ProcessUtility_hook if an app would just want to execute the prepared
> > > > statement.
> > >
> > > Isn't that what the preprepare extension does already?
> >
> > more generic facility -> more useful
> >
> > My use case is to pre-create TEMP schema elements that VIEWs, FUNCTIONs,
> > and TRIGGERs, might need.
> 
> It is better to work on GLOBAL TEMP tables.

I don't disagree.

In fact, I was scoping out what it might take to do that just yesterday.

I've too thoughts on that: either a new relpersistence kind that is very
similar to persistent, but which always uses temp heaps, or a modifier
for the persistent kind that says to use temp heaps.  Either way it
looks like it should be fairly straightforward (but then, i've only
ever written one thing for PG, earlier this week, the ALWAYS DEFERRED
thing).

> Current TEMP tables, if you do it for any session has pretty significant
> overhead  - with possible risk of performance lost (system catalog bloat).

Because of the DDLs for them?

> So often creating local temp tables is antipattern (in Postgres)
> unfortunately.

I do it plenty, but sometimes I use an UNLOGGED table with a txid column
in the PK set to txid_current(), then I clean up where I can.  It'd be
nice to have COMMIT triggers for cleaning up such rows, among other
things.  I've implemented that using DDL event triggers, but to perform
well it needs to be a native feature.

> I am not sure, if we should to support this case more :( Probably is
> better, so it is hard to use local TEMP tables.

No, I want GLOBAL TEMP tables.

Nico
-- 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Craig Ringer]

On 6 October 2017 at 10:52, Pavel Stehule  wrote:

> It is better to work on GLOBAL TEMP tables.
>
> Current TEMP tables, if you do it for any session has pretty significant
> overhead  - with possible risk of performance lost (system catalog bloat).
>
> pretty significant performance issue of my customers are related to temp
> tables usage (under high load)

I've seen the same thing too. Especially when combined with logical
decoding, where IIRC we mark transactions as having catalog changes
due to temp tables.

Sometimes the catalog bloat can be truly horrible when a user has
hundreds of plpgsql functions that all like to make temp tables.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Pavel Stehule]

2017-10-05 22:31 GMT+02:00 Nico Williams :

> On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> > On 7/21/17 13:14, Jim Mlodgenski wrote:
> > > When I first saw this thread, my initial thought of a use case is to
> > > prepare some key application queries so they are there and ready to go.
> > > That would need to be before the ExecutorStart_hook or
> > > ProcessUtility_hook if an app would just want to execute the prepared
> > > statement.
> >
> > Isn't that what the preprepare extension does already?
>
> more generic facility -> more useful
>
> My use case is to pre-create TEMP schema elements that VIEWs, FUNCTIONs,
> and TRIGGERs, might need.
>

It is better to work on GLOBAL TEMP tables.

Current TEMP tables, if you do it for any session has pretty significant
overhead  - with possible risk of performance lost (system catalog bloat).

pretty significant performance issue of my customers are related to temp
tables usage (under high load)

So often creating local temp tables is antipattern (in Postgres)
unfortunately.

I am not sure, if we should to support this case more :( Probably is
better, so it is hard to use local TEMP tables.

Regards

Pavel

>
> Nico
> --
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] [PATCH] A hook for session start

[Nico Williams]

On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> On 7/21/17 13:14, Jim Mlodgenski wrote:
> > When I first saw this thread, my initial thought of a use case is to
> > prepare some key application queries so they are there and ready to go.
> > That would need to be before the ExecutorStart_hook or
> > ProcessUtility_hook if an app would just want to execute the prepared
> > statement.
> 
> Isn't that what the preprepare extension does already?

more generic facility -> more useful

My use case is to pre-create TEMP schema elements that VIEWs, FUNCTIONs,
and TRIGGERs, might need.

Nico
-- 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Nico Williams]

On Fri, Jul 21, 2017 at 11:10:52PM +0800, Craig Ringer wrote:
> What practical use cases are there for acting post-auth but that can't wait
> until the user tries to do something?

Creating TEMP schema that triggers and functions might need.

Doing CREATE TEMP TABLE IF NOT EXISTS in triggers slows things down.

It'd be super nice if PostgreSQL had some sort of persistent TEMP
schema option, where you can have schema elements that are persistent
in that they're always there, but where the data is all TEMP.  Oracle
has this and they call it GLOBAL TEMP IIRC.  There would be some
caveats, such as not being able to have FKs between these sorts of
persistent temp tables and persistent tables.

In the absence of such a feature, a session hook/trigger is a great
workaround.

> Can a user do anything remotely interesting or useful without hitting
> either ExecutorStart_hook or ProcessUtility_hook? They can parse queries I
> guess but you could just set your hook up in the parser instead. If you
> hook the parser all they can do is open an idle session and sit there...

In any other hook you'd have to check whether the session setup work you
wanted to do has been done.  That could be potentially slow.

I actually have an all SQL implementation of session/begin/commit
triggers.  The session triggers in that implementation only run on the
first DML statement, which could be too late for OP's purpose.

Nico
-- 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Oct 5, 2017 at 4:14 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
>
> On Tue, Aug 1, 2017 at 4:55 PM, Robert Haas  wrote:
> >
> > On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
> >  wrote:
> > > On 7/21/17 12:59, Robert Haas wrote:
> > >> That's an exceedingly-weak argument for rejecting this patch.  The
> > >> fact that you can probably hack around the lack of a hook for most
> > >> reasonable use cases is not an argument for having a hook that does
> > >> what people actually want to do.
> > >
> > > Still nobody has presented a concrete use case so far.
> >
> > I've been asked for this at EDB, too.  Inserting a row into some table
> > on each logon, for example.
> >
> > A quick Google search found 6 previous requests for this feature, some
> > of which describe intended use cases:
> >
> > https://www.postgresql.org/message-id/4ebc6852.5030...@fuzzy.cz (2011)
> >
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
> > (2011)
> >
https://www.postgresql.org/message-id/bay104-w513cf26c0046c9d28747b8d1...@phx.gbl
> > (2009, in Spanish)
> >
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
> > (2008)
> >
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
> > (2004)
> >
https://www.postgresql.org/message-id/f96sgcorblsaqv6updv0...@hotmail.com
> > (2000)
> >
>
> Hi all,
>
> I'm sending a new rebased patches and added tests to src/tests/modules as
suggested before.
>

Also added for the next commitfest:

https://commitfest.postgresql.org/15/1318/

Att,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Tue, Aug 1, 2017 at 4:55 PM, Robert Haas  wrote:
>
> On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
>  wrote:
> > On 7/21/17 12:59, Robert Haas wrote:
> >> That's an exceedingly-weak argument for rejecting this patch.  The
> >> fact that you can probably hack around the lack of a hook for most
> >> reasonable use cases is not an argument for having a hook that does
> >> what people actually want to do.
> >
> > Still nobody has presented a concrete use case so far.
>
> I've been asked for this at EDB, too.  Inserting a row into some table
> on each logon, for example.
>
> A quick Google search found 6 previous requests for this feature, some
> of which describe intended use cases:
>
> https://www.postgresql.org/message-id/4ebc6852.5030...@fuzzy.cz (2011)
>
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
> (2011)
>
https://www.postgresql.org/message-id/bay104-w513cf26c0046c9d28747b8d1...@phx.gbl
> (2009, in Spanish)
>
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
> (2008)
>
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
> (2004)
>
https://www.postgresql.org/message-id/f96sgcorblsaqv6updv0...@hotmail.com
> (2000)
>

Hi all,

I'm sending a new rebased patches and added tests to src/tests/modules as
suggested before.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index c807b00..5c22f2d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -165,6 +165,9 @@ static bool RecoveryConflictPending = false;
 static bool RecoveryConflictRetryable = true;
 static ProcSignalReason RecoveryConflictReason;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3838,6 +3841,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) (dbname, username);
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..d349592 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,11 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start of session */
+typedef void (*session_start_hook_type) (const char *dbname,
+		 const char *username);
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 5c22f2d..79f3099 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -165,9 +165,9 @@ static bool RecoveryConflictPending = false;
 static bool RecoveryConflictRetryable = true;
 static ProcSignalReason RecoveryConflictReason;
 
-/* Hook for plugins to get control at start of session */
+/* Hook for plugins to get control at start or end of session */
 session_start_hook_type session_start_hook = NULL;
-
+session_end_hook_type session_end_hook = NULL;
 /* 
  *		decls for routines only used in this file
  * 
@@ -192,6 +192,7 @@ static void drop_unnamed_stmt(void);
 static void log_disconnections(int code, Datum arg);
 static void enable_statement_timeout(void);
 static void disable_statement_timeout(void);
+static void do_session_end_hook(int code, Datum arg);
 
 
 /* 
@@ -3845,6 +3846,12 @@ PostgresMain(int argc, char *argv[],
 		(*session_start_hook) (dbname, username);
 
 	/*
+	 * Setup handler to session end hook
+	 */
+	if (IsUnderPostmaster)
+		on_proc_exit(do_session_end_hook, 0);
+
+	/*
 	 * POSTGRES main processing loop begins here
 	 *
 	 * If an exception is encountered, processing resumes here so we abort the
@@ -4596,3 +4603,15 @@ disable_statement_timeout(void)
 		stmt_timeout_active = false;
 	}
 }
+
+/*
+ * on_proc_exit handler to call session end hook
+ */
+static void
+do_session_end_hook(int code, Datum arg)
+{
+	Port	   *port = MyProcPort;
+
+	if (session_end_hook)
+		(*session_end_hook) (port->database_name, port->user_name);
+}
\ No newline at end of file

Re: [HACKERS] [PATCH] A hook for session start

[Robert Haas]

On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
 wrote:
> On 7/21/17 12:59, Robert Haas wrote:
>> That's an exceedingly-weak argument for rejecting this patch.  The
>> fact that you can probably hack around the lack of a hook for most
>> reasonable use cases is not an argument for having a hook that does
>> what people actually want to do.
>
> Still nobody has presented a concrete use case so far.

I've been asked for this at EDB, too.  Inserting a row into some table
on each logon, for example.

A quick Google search found 6 previous requests for this feature, some
of which describe intended use cases:

https://www.postgresql.org/message-id/4ebc6852.5030...@fuzzy.cz (2011)
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
(2011)
https://www.postgresql.org/message-id/bay104-w513cf26c0046c9d28747b8d1...@phx.gbl
(2009, in Spanish)
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
(2008)
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
(2004)
https://www.postgresql.org/message-id/f96sgcorblsaqv6updv0...@hotmail.com
(2000)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Andres Freund]

On 2017-08-01 15:37:40 -0400, Peter Eisentraut wrote:
> On 7/21/17 12:59, Robert Haas wrote:
> > That's an exceedingly-weak argument for rejecting this patch.  The
> > fact that you can probably hack around the lack of a hook for most
> > reasonable use cases is not an argument for having a hook that does
> > what people actually want to do.
> 
> Still nobody has presented a concrete use case so far.

Citus for example starts a background worker (performing
e.g. distributed deadlock detection) if the citus extension exists. We
atm need annoying hacks to do so when the first query is executed.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Peter Eisentraut]

On 7/21/17 12:59, Robert Haas wrote:
> That's an exceedingly-weak argument for rejecting this patch.  The
> fact that you can probably hack around the lack of a hook for most
> reasonable use cases is not an argument for having a hook that does
> what people actually want to do.

Still nobody has presented a concrete use case so far.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Peter Eisentraut]

On 7/20/17 07:47, Yugo Nagata wrote:
> Another patch, session_start_sample.patch, is a very simple
> example of this hook that changes work_mem values for sessions
> of a specific database. 

I think test modules should go into src/test/modules/ instead of contrib.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Peter Eisentraut]

On 7/21/17 13:14, Jim Mlodgenski wrote:
> When I first saw this thread, my initial thought of a use case is to
> prepare some key application queries so they are there and ready to go.
> That would need to be before the ExecutorStart_hook or
> ProcessUtility_hook if an app would just want to execute the prepared
> statement.

Isn't that what the preprepare extension does already?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Jul 21, 2017 at 12:19 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
> On Fri, Jul 21, 2017 at 10:58 AM, Yugo Nagata  wrote:
> >
> > On Fri, 21 Jul 2017 10:31:57 -0300
> > Fabrízio de Royes Mello  wrote:
> >
> > > On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata 
wrote:
> > > >
> > > > On Fri, 21 Jul 2017 09:53:19 +0800
> > > > Craig Ringer  wrote:
> > > >
> > > > > On 21 July 2017 at 08:42, Robert Haas 
wrote:
> > > > >
> > > > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > > >  wrote:
> > > > > > > I'm not sure your real needs but doesn't it material for
improve
> > > Event
> > > > > > > Triggers???
> > > > > >
> > > > > > I've thought about that, too.  One problem is what to do if the
user
> > > > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > > > to that by killing the event trigger and letting the user issue
> > > > > > commands, then the event trigger can't be used for security or
> > > > > > auditing purposes because the user might prevent it from doing
> > > > > > whatever it's intended to do with a well-timed interrupt.  If
you
> > > > > > ignore ^C or make it turn into FATAL, then a poorly-crafted
trigger
> > > > > > can lock users out of the database.  Maybe that's OK.  We could
say
> > > > > > "well, if you lock yourself out of the database with your logon
> > > > > > trigger, you get to shut down the database and restart in
single user
> > > > > > mode to recover".
> > > > > >
> > > > > > A hook, as proposed here, is a lot simpler and lacks these
concerns.
> > > > > > Installing code in C into the database is intrinsically risky
> > > > > > anywhere, and not any moreso here than elsewhere.  But it's
also less
> > > > > > accessible to the average user.
> > > > > > 
> > > > >
> > > > >
> > > > > I'd favour the c hook personally. It's a lot more flexible, and
can be
> > > used
> > > > > by an extension to implement trigger-like behaviour if anyone
wants it,
> > > > > including the extension's choice of error handling decisions.
> > > > >
> > > > > It's also a lot simpler and less intrusive for core. Which is nice
> > > where we
> > > > > don't have something that we don't have anything compelling
destined for
> > > > > core that needs it. (I want to add a bunch of hooks in the logical
> > > > > replication code in pg11 for similar reasons, and so features
like DDL
> > > > > replication can be prototyped as extensions more practically).
> > > > >
> > >
> > > I agree with you both...
> > >
> > >
> > > > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> > > the
> > > > > same job as a session-start hook, albeit at slightly higher
overhead?
> > > You
> > > > > can just test to see if your initial tasks have run yet.
> > > >
> > > > Thank you for your suggestion. Certainly, we can do the similar job
of a
> > > > session-start hook using these existing hooks, although these hooks
are
> > > > triggered when the first query is executed not when the session is
> > > started.
> > > > Now I come to think that an additional hook is not need.
> > > >
> > >
> > > As Nagata said hooks proposed by Craing will happens only when the
first
> > > query is called so I don't know how it works for session start... are
we
> > > missing something?
> >
> > Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
> > session_start hook. If a query is issued a long time since the session
start,
> > the timing the hook happens is largely deviated. It is no problem if we
only
> > want do something once at the session start, but it might be problem if
> > we want to record the timestamp of the session start, for example.
> >
> > >
> > > If we're going to add this hook what about add a session end hook
also?
> >
> > If someone want the session-start hook, he might want this too.
> >
>
> Well if someone wants here are the patches... I just did a minor fix and
cleanup in your previous session_start sample and provide both samples into
the same patch.
>

I made a mistake on previous patch... now the attached three patches in
their correct orders.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b8d860e..7a1fa3b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -160,6 +160,9 @@ static bool RecoveryConflictPending = false;
 static bool RecoveryConflictRetryable = true;
 static ProcSignalReason RecoveryConflictReason;
 
+/* Hook for plugins to get control at start of session */

Re: [HACKERS] [PATCH] A hook for session start

[Jim Mlodgenski]

>
> > Can a user do anything remotely interesting or useful without hitting
> either
> > ExecutorStart_hook or ProcessUtility_hook? They can parse queries I guess
> > but you could just set your hook up in the parser instead. If you hook
> the
> > parser all they can do is open an idle session and sit there...
>
> That's an exceedingly-weak argument for rejecting this patch.  The
> fact that you can probably hack around the lack of a hook for most
> reasonable use cases is not an argument for having a hook that does
> what people actually want to do.
>
>
When I first saw this thread, my initial thought of a use case is to
prepare some key application queries so they are there and ready to go.
That would need to be before the ExecutorStart_hook or ProcessUtility_hook
if an app would just want to execute the prepared statement.

Re: [HACKERS] [PATCH] A hook for session start

[Robert Haas]

On Fri, Jul 21, 2017 at 11:10 AM, Craig Ringer  wrote:
> Don't we have that timestamp already?
>
> What practical use cases are there for acting post-auth but that can't wait
> until the user tries to do something?

Have, yes; record, no.

> Can a user do anything remotely interesting or useful without hitting either
> ExecutorStart_hook or ProcessUtility_hook? They can parse queries I guess
> but you could just set your hook up in the parser instead. If you hook the
> parser all they can do is open an idle session and sit there...

That's an exceedingly-weak argument for rejecting this patch.  The
fact that you can probably hack around the lack of a hook for most
reasonable use cases is not an argument for having a hook that does
what people actually want to do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Jul 21, 2017 at 10:58 AM, Yugo Nagata  wrote:
>
> On Fri, 21 Jul 2017 10:31:57 -0300
> Fabrízio de Royes Mello  wrote:
>
> > On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata 
wrote:
> > >
> > > On Fri, 21 Jul 2017 09:53:19 +0800
> > > Craig Ringer  wrote:
> > >
> > > > On 21 July 2017 at 08:42, Robert Haas  wrote:
> > > >
> > > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > >  wrote:
> > > > > > I'm not sure your real needs but doesn't it material for improve
> > Event
> > > > > > Triggers???
> > > > >
> > > > > I've thought about that, too.  One problem is what to do if the
user
> > > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > > to that by killing the event trigger and letting the user issue
> > > > > commands, then the event trigger can't be used for security or
> > > > > auditing purposes because the user might prevent it from doing
> > > > > whatever it's intended to do with a well-timed interrupt.  If you
> > > > > ignore ^C or make it turn into FATAL, then a poorly-crafted
trigger
> > > > > can lock users out of the database.  Maybe that's OK.  We could
say
> > > > > "well, if you lock yourself out of the database with your logon
> > > > > trigger, you get to shut down the database and restart in single
user
> > > > > mode to recover".
> > > > >
> > > > > A hook, as proposed here, is a lot simpler and lacks these
concerns.
> > > > > Installing code in C into the database is intrinsically risky
> > > > > anywhere, and not any moreso here than elsewhere.  But it's also
less
> > > > > accessible to the average user.
> > > > > 
> > > >
> > > >
> > > > I'd favour the c hook personally. It's a lot more flexible, and can
be
> > used
> > > > by an extension to implement trigger-like behaviour if anyone wants
it,
> > > > including the extension's choice of error handling decisions.
> > > >
> > > > It's also a lot simpler and less intrusive for core. Which is nice
> > where we
> > > > don't have something that we don't have anything compelling
destined for
> > > > core that needs it. (I want to add a bunch of hooks in the logical
> > > > replication code in pg11 for similar reasons, and so features like
DDL
> > > > replication can be prototyped as extensions more practically).
> > > >
> >
> > I agree with you both...
> >
> >
> > > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> > the
> > > > same job as a session-start hook, albeit at slightly higher
overhead?
> > You
> > > > can just test to see if your initial tasks have run yet.
> > >
> > > Thank you for your suggestion. Certainly, we can do the similar job
of a
> > > session-start hook using these existing hooks, although these hooks
are
> > > triggered when the first query is executed not when the session is
> > started.
> > > Now I come to think that an additional hook is not need.
> > >
> >
> > As Nagata said hooks proposed by Craing will happens only when the first
> > query is called so I don't know how it works for session start... are we
> > missing something?
>
> Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
> session_start hook. If a query is issued a long time since the session
start,
> the timing the hook happens is largely deviated. It is no problem if we
only
> want do something once at the session start, but it might be problem if
> we want to record the timestamp of the session start, for example.
>
> >
> > If we're going to add this hook what about add a session end hook also?
>
> If someone want the session-start hook, he might want this too.
>

Well if someone wants here are the patches... I just did a minor fix and
cleanup in your previous session_start sample and provide both samples into
the same patch.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b8d860e..7a1fa3b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -160,6 +160,9 @@ static bool RecoveryConflictPending = false;
 static bool RecoveryConflictRetryable = true;
 static ProcSignalReason RecoveryConflictReason;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3808,6 +3811,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if 

Re: [HACKERS] [PATCH] A hook for session start

[Craig Ringer]

On 21 Jul. 2017 21:58, "Yugo Nagata"  wrote:

On Fri, 21 Jul 2017 10:31:57 -0300
Fabrízio de Royes Mello  wrote:

> On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata  wrote:
> >
> > On Fri, 21 Jul 2017 09:53:19 +0800
> > Craig Ringer  wrote:
> >
> > > On 21 July 2017 at 08:42, Robert Haas  wrote:
> > >
> > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > >  wrote:
> > > > > I'm not sure your real needs but doesn't it material for improve
> Event
> > > > > Triggers???
> > > >
> > > > I've thought about that, too.  One problem is what to do if the user
> > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > to that by killing the event trigger and letting the user issue
> > > > commands, then the event trigger can't be used for security or
> > > > auditing purposes because the user might prevent it from doing
> > > > whatever it's intended to do with a well-timed interrupt.  If you
> > > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > > > can lock users out of the database.  Maybe that's OK.  We could say
> > > > "well, if you lock yourself out of the database with your logon
> > > > trigger, you get to shut down the database and restart in single
user
> > > > mode to recover".
> > > >
> > > > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > > > Installing code in C into the database is intrinsically risky
> > > > anywhere, and not any moreso here than elsewhere.  But it's also
less
> > > > accessible to the average user.
> > > > 
> > >
> > >
> > > I'd favour the c hook personally. It's a lot more flexible, and can be
> used
> > > by an extension to implement trigger-like behaviour if anyone wants
it,
> > > including the extension's choice of error handling decisions.
> > >
> > > It's also a lot simpler and less intrusive for core. Which is nice
> where we
> > > don't have something that we don't have anything compelling destined
for
> > > core that needs it. (I want to add a bunch of hooks in the logical
> > > replication code in pg11 for similar reasons, and so features like DDL
> > > replication can be prototyped as extensions more practically).
> > >
>
> I agree with you both...
>
>
> > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> the
> > > same job as a session-start hook, albeit at slightly higher overhead?
> You
> > > can just test to see if your initial tasks have run yet.
> >
> > Thank you for your suggestion. Certainly, we can do the similar job of a
> > session-start hook using these existing hooks, although these hooks are
> > triggered when the first query is executed not when the session is
> started.
> > Now I come to think that an additional hook is not need.
> >
>
> As Nagata said hooks proposed by Craing will happens only when the first
> query is called so I don't know how it works for session start... are we
> missing something?

Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
session_start hook. If a query is issued a long time since the session
start,
the timing the hook happens is largely deviated. It is no problem if we only
want do something once at the session start, but it might be problem if
we want to record the timestamp of the session start, for example.


Don't we have that timestamp already?

What practical use cases are there for acting post-auth but that can't wait
until the user tries to do something?

Can a user do anything remotely interesting or useful without hitting
either ExecutorStart_hook or ProcessUtility_hook? They can parse queries I
guess but you could just set your hook up in the parser instead. If you
hook the parser all they can do is open an idle session and sit there...

So given that you can effectively do it already at the C hook level, if
you're going to do it at all I guess it it'd be more interesting to expose
a convenient event trigger for session start. As others suggested upthread.
So it's easy for DBAs and devs who won't have any idea where to start
writing extensions that register hooks.

But... I think you need a good use case. Such a trigger would have no way
to receive parameters from the user (except custom GUCs) or report any sort
of result other than an error/warning/notice. So what's it going to do that
can't already be decided by pg_hba.cond, pg_authid etc?

Re: [HACKERS] [PATCH] A hook for session start

[Yugo Nagata]

On Fri, 21 Jul 2017 10:31:57 -0300
Fabrízio de Royes Mello  wrote:

> On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata  wrote:
> >
> > On Fri, 21 Jul 2017 09:53:19 +0800
> > Craig Ringer  wrote:
> >
> > > On 21 July 2017 at 08:42, Robert Haas  wrote:
> > >
> > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > >  wrote:
> > > > > I'm not sure your real needs but doesn't it material for improve
> Event
> > > > > Triggers???
> > > >
> > > > I've thought about that, too.  One problem is what to do if the user
> > > > hits ^C while the event trigger procedure is running.  If you respond
> > > > to that by killing the event trigger and letting the user issue
> > > > commands, then the event trigger can't be used for security or
> > > > auditing purposes because the user might prevent it from doing
> > > > whatever it's intended to do with a well-timed interrupt.  If you
> > > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > > > can lock users out of the database.  Maybe that's OK.  We could say
> > > > "well, if you lock yourself out of the database with your logon
> > > > trigger, you get to shut down the database and restart in single user
> > > > mode to recover".
> > > >
> > > > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > > > Installing code in C into the database is intrinsically risky
> > > > anywhere, and not any moreso here than elsewhere.  But it's also less
> > > > accessible to the average user.
> > > > 
> > >
> > >
> > > I'd favour the c hook personally. It's a lot more flexible, and can be
> used
> > > by an extension to implement trigger-like behaviour if anyone wants it,
> > > including the extension's choice of error handling decisions.
> > >
> > > It's also a lot simpler and less intrusive for core. Which is nice
> where we
> > > don't have something that we don't have anything compelling destined for
> > > core that needs it. (I want to add a bunch of hooks in the logical
> > > replication code in pg11 for similar reasons, and so features like DDL
> > > replication can be prototyped as extensions more practically).
> > >
> 
> I agree with you both...
> 
> 
> > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve
> the
> > > same job as a session-start hook, albeit at slightly higher overhead?
> You
> > > can just test to see if your initial tasks have run yet.
> >
> > Thank you for your suggestion. Certainly, we can do the similar job of a
> > session-start hook using these existing hooks, although these hooks are
> > triggered when the first query is executed not when the session is
> started.
> > Now I come to think that an additional hook is not need.
> >
> 
> As Nagata said hooks proposed by Craing will happens only when the first
> query is called so I don't know how it works for session start... are we
> missing something?

Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
session_start hook. If a query is issued a long time since the session start,
the timing the hook happens is largely deviated. It is no problem if we only
want do something once at the session start, but it might be problem if
we want to record the timestamp of the session start, for example.

> 
> If we're going to add this hook what about add a session end hook also?

If someone want the session-start hook, he might want this too.

> 
> Regards,
> 
> --
> Fabrízio de Royes Mello
> Consultoria/Coaching PostgreSQL
> >> Timbira: http://www.timbira.com.br
> >> Blog: http://fabriziomello.github.io
> >> Linkedin: http://br.linkedin.com/in/fabriziomello
> >> Twitter: http://twitter.com/fabriziomello
> >> Github: http://github.com/fabriziomello


-- 
Yugo Nagata 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata  wrote:
>
> On Fri, 21 Jul 2017 09:53:19 +0800
> Craig Ringer  wrote:
>
> > On 21 July 2017 at 08:42, Robert Haas  wrote:
> >
> > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > >  wrote:
> > > > I'm not sure your real needs but doesn't it material for improve
Event
> > > > Triggers???
> > >
> > > I've thought about that, too.  One problem is what to do if the user
> > > hits ^C while the event trigger procedure is running.  If you respond
> > > to that by killing the event trigger and letting the user issue
> > > commands, then the event trigger can't be used for security or
> > > auditing purposes because the user might prevent it from doing
> > > whatever it's intended to do with a well-timed interrupt.  If you
> > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > > can lock users out of the database.  Maybe that's OK.  We could say
> > > "well, if you lock yourself out of the database with your logon
> > > trigger, you get to shut down the database and restart in single user
> > > mode to recover".
> > >
> > > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > > Installing code in C into the database is intrinsically risky
> > > anywhere, and not any moreso here than elsewhere.  But it's also less
> > > accessible to the average user.
> > > 
> >
> >
> > I'd favour the c hook personally. It's a lot more flexible, and can be
used
> > by an extension to implement trigger-like behaviour if anyone wants it,
> > including the extension's choice of error handling decisions.
> >
> > It's also a lot simpler and less intrusive for core. Which is nice
where we
> > don't have something that we don't have anything compelling destined for
> > core that needs it. (I want to add a bunch of hooks in the logical
> > replication code in pg11 for similar reasons, and so features like DDL
> > replication can be prototyped as extensions more practically).
> >

I agree with you both...


> > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve
the
> > same job as a session-start hook, albeit at slightly higher overhead?
You
> > can just test to see if your initial tasks have run yet.
>
> Thank you for your suggestion. Certainly, we can do the similar job of a
> session-start hook using these existing hooks, although these hooks are
> triggered when the first query is executed not when the session is
started.
> Now I come to think that an additional hook is not need.
>

As Nagata said hooks proposed by Craing will happens only when the first
query is called so I don't know how it works for session start... are we
missing something?

If we're going to add this hook what about add a session end hook also?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Yugo Nagata]

On Fri, 21 Jul 2017 09:53:19 +0800
Craig Ringer  wrote:

> On 21 July 2017 at 08:42, Robert Haas  wrote:
> 
> > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> >  wrote:
> > > I'm not sure your real needs but doesn't it material for improve Event
> > > Triggers???
> >
> > I've thought about that, too.  One problem is what to do if the user
> > hits ^C while the event trigger procedure is running.  If you respond
> > to that by killing the event trigger and letting the user issue
> > commands, then the event trigger can't be used for security or
> > auditing purposes because the user might prevent it from doing
> > whatever it's intended to do with a well-timed interrupt.  If you
> > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > can lock users out of the database.  Maybe that's OK.  We could say
> > "well, if you lock yourself out of the database with your logon
> > trigger, you get to shut down the database and restart in single user
> > mode to recover".
> >
> > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > Installing code in C into the database is intrinsically risky
> > anywhere, and not any moreso here than elsewhere.  But it's also less
> > accessible to the average user.
> > 
> 
> 
> I'd favour the c hook personally. It's a lot more flexible, and can be used
> by an extension to implement trigger-like behaviour if anyone wants it,
> including the extension's choice of error handling decisions.
> 
> It's also a lot simpler and less intrusive for core. Which is nice where we
> don't have something that we don't have anything compelling destined for
> core that needs it. (I want to add a bunch of hooks in the logical
> replication code in pg11 for similar reasons, and so features like DDL
> replication can be prototyped as extensions more practically).
> 
> That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve the
> same job as a session-start hook, albeit at slightly higher overhead? You
> can just test to see if your initial tasks have run yet.

Thank you for your suggestion. Certainly, we can do the similar job of a
session-start hook using these existing hooks, although these hooks are
triggered when the first query is executed not when the session is started.
Now I come to think that an additional hook is not need.

Thanks,

> 
> -- 
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Yugo Nagata 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Craig Ringer]

On 21 July 2017 at 08:42, Robert Haas  wrote:

> On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
>  wrote:
> > I'm not sure your real needs but doesn't it material for improve Event
> > Triggers???
>
> I've thought about that, too.  One problem is what to do if the user
> hits ^C while the event trigger procedure is running.  If you respond
> to that by killing the event trigger and letting the user issue
> commands, then the event trigger can't be used for security or
> auditing purposes because the user might prevent it from doing
> whatever it's intended to do with a well-timed interrupt.  If you
> ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> can lock users out of the database.  Maybe that's OK.  We could say
> "well, if you lock yourself out of the database with your logon
> trigger, you get to shut down the database and restart in single user
> mode to recover".
>
> A hook, as proposed here, is a lot simpler and lacks these concerns.
> Installing code in C into the database is intrinsically risky
> anywhere, and not any moreso here than elsewhere.  But it's also less
> accessible to the average user.
> 


I'd favour the c hook personally. It's a lot more flexible, and can be used
by an extension to implement trigger-like behaviour if anyone wants it,
including the extension's choice of error handling decisions.

It's also a lot simpler and less intrusive for core. Which is nice where we
don't have something that we don't have anything compelling destined for
core that needs it. (I want to add a bunch of hooks in the logical
replication code in pg11 for similar reasons, and so features like DDL
replication can be prototyped as extensions more practically).

That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve the
same job as a session-start hook, albeit at slightly higher overhead? You
can just test to see if your initial tasks have run yet.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] [PATCH] A hook for session start

[Robert Haas]

On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
 wrote:
> I'm not sure your real needs but doesn't it material for improve Event
> Triggers???

I've thought about that, too.  One problem is what to do if the user
hits ^C while the event trigger procedure is running.  If you respond
to that by killing the event trigger and letting the user issue
commands, then the event trigger can't be used for security or
auditing purposes because the user might prevent it from doing
whatever it's intended to do with a well-timed interrupt.  If you
ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
can lock users out of the database.  Maybe that's OK.  We could say
"well, if you lock yourself out of the database with your logon
trigger, you get to shut down the database and restart in single user
mode to recover".

A hook, as proposed here, is a lot simpler and lacks these concerns.
Installing code in C into the database is intrinsically risky
anywhere, and not any moreso here than elsewhere.  But it's also less
accessible to the average user.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Jul 20, 2017 at 8:47 AM, Yugo Nagata  wrote:
>
> Hi,
>
> Currently, PostgreSQL doen't have a hook triggered at session
> start. Although we already have ClientAuthentication_hook,
> this is triggered during authentication, so we can not
> access the database.
>
> If we have a hook triggerd only once at session start, we may
> do something useful on the session for certain database or user.
>
> For example, one of our clients wanted such feature. He wanted
> to handle encription for specific users, though I don't know
> the detail.
>
> The attached patch (session_start_hook.patch) implements such
> hook.
>
> Another patch, session_start_sample.patch, is a very simple
> example of this hook that changes work_mem values for sessions
> of a specific database.
>
> I would appreciate hearing your opinion on this hook.
>

I'm not sure your real needs but doesn't it material for improve Event
Triggers???

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello