On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <michael.paqu...@gmail.com> 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 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 0000000..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 0000000..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 0000000..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 100644 index 0000000..be1b949 --- /dev/null +++ b/src/test/modules/test_session_hooks/expected/test_session_hooks.out @@ -0,0 +1,31 @@ +CREATE ROLE regress_sess_hook_usr1 SUPERUSER LOGIN; +CREATE ROLE regress_sess_hook_usr2 SUPERUSER LOGIN; +\set prevdb :DBNAME +\set prevusr :USER +CREATE TABLE session_hook_log(id SERIAL, dbname TEXT, username TEXT, hook_at TEXT); +SELECT * FROM session_hook_log ORDER BY id; + id | dbname | username | hook_at +----+--------+----------+--------- +(0 rows) + +\c :prevdb regress_sess_hook_usr1 +SELECT * FROM session_hook_log ORDER BY id; + id | dbname | username | hook_at +----+--------+----------+--------- +(0 rows) + +\c :prevdb regress_sess_hook_usr2 +SELECT * FROM session_hook_log ORDER BY id; + id | dbname | username | hook_at +----+--------------------+------------------------+--------- + 1 | contrib_regression | regress_sess_hook_usr2 | START +(1 row) + +\c :prevdb :prevusr +SELECT * FROM session_hook_log ORDER BY id; + id | dbname | username | hook_at +----+--------------------+------------------------+--------- + 1 | contrib_regression | regress_sess_hook_usr2 | START + 2 | contrib_regression | regress_sess_hook_usr2 | END +(2 rows) + diff --git a/src/test/modules/test_session_hooks/session_hooks.conf b/src/test/modules/test_session_hooks/session_hooks.conf new file mode 100644 index 0000000..a66e60e --- /dev/null +++ b/src/test/modules/test_session_hooks/session_hooks.conf @@ -0,0 +1 @@ +shared_preload_libraries = 'test_session_hooks' diff --git a/src/test/modules/test_session_hooks/sql/test_session_hooks.sql b/src/test/modules/test_session_hooks/sql/test_session_hooks.sql new file mode 100644 index 0000000..5e08647 --- /dev/null +++ b/src/test/modules/test_session_hooks/sql/test_session_hooks.sql @@ -0,0 +1,12 @@ +CREATE ROLE regress_sess_hook_usr1 SUPERUSER LOGIN; +CREATE ROLE regress_sess_hook_usr2 SUPERUSER LOGIN; +\set prevdb :DBNAME +\set prevusr :USER +CREATE TABLE session_hook_log(id SERIAL, dbname TEXT, username TEXT, hook_at TEXT); +SELECT * FROM session_hook_log ORDER BY id; +\c :prevdb regress_sess_hook_usr1 +SELECT * FROM session_hook_log ORDER BY id; +\c :prevdb regress_sess_hook_usr2 +SELECT * FROM session_hook_log ORDER BY id; +\c :prevdb :prevusr +SELECT * FROM session_hook_log ORDER BY id; diff --git a/src/test/modules/test_session_hooks/test_session_hooks--1.0.sql b/src/test/modules/test_session_hooks/test_session_hooks--1.0.sql new file mode 100644 index 0000000..16bcee9 --- /dev/null +++ b/src/test/modules/test_session_hooks/test_session_hooks--1.0.sql @@ -0,0 +1,4 @@ +/* src/test/modules/test_hook_session/test_hook_session--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_hook_session" to load this file. \quit diff --git a/src/test/modules/test_session_hooks/test_session_hooks.c b/src/test/modules/test_session_hooks/test_session_hooks.c new file mode 100644 index 0000000..1edd171 --- /dev/null +++ b/src/test/modules/test_session_hooks/test_session_hooks.c @@ -0,0 +1,121 @@ +/* ------------------------------------------------------------------------- + * + * test_session_hooks.c + * Code for testing SESSION hooks. + * + * Copyright (c) 2010-2017, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_session_hooks/test_session_hooks.c + * + * ------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "access/xact.h" +#include "commands/dbcommands.h" +#include "executor/spi.h" +#include "lib/stringinfo.h" +#include "miscadmin.h" +#include "tcop/tcopprot.h" +#include "utils/snapmgr.h" +#include "utils/builtins.h" + +PG_MODULE_MAGIC; + +/* Entry point of library loading/unloading */ +void _PG_init(void); +void _PG_fini(void); + +/* Original Hook */ +static session_start_hook_type prev_session_start_hook = NULL; +static session_end_hook_type prev_session_end_hook = NULL; + +static void +register_session_hook(const char *hook_at) +{ + const char *username; + + StartTransactionCommand(); + SPI_connect(); + PushActiveSnapshot(GetTransactionSnapshot()); + + username = GetUserNameFromId(GetUserId(), false); + + if (!strcmp(username, "regress_sess_hook_usr2")) + { + const char *dbname; + int ret; + StringInfoData buf; + + dbname = get_database_name(MyDatabaseId); + + initStringInfo(&buf); + + appendStringInfo(&buf, "INSERT INTO session_hook_log (dbname, username, hook_at) "); + appendStringInfo(&buf, "VALUES ('%s', '%s', '%s');", + dbname, username, hook_at); + + ret = SPI_exec(buf.data, 0); + if (ret != SPI_OK_INSERT) + elog(ERROR, "SPI_execute failed: error code %d", ret); + } + + SPI_finish(); + PopActiveSnapshot(); + CommitTransactionCommand(); +} + +/* sample session start hook function */ +static void +sample_session_start_hook() +{ + /* Hook just normal backends */ + if (MyBackendId != InvalidBackendId) + { + (void) register_session_hook("START"); + + if (prev_session_start_hook) + prev_session_start_hook(); + } +} + +/* sample session end hook function */ +static void +sample_session_end_hook() +{ + /* Hook just normal backends */ + if (MyBackendId != InvalidBackendId) + { + if (prev_session_end_hook) + prev_session_end_hook(); + + (void) register_session_hook("END"); + } +} + +/* + * Module Load Callback + */ +void +_PG_init(void) +{ + /* Save Hooks for Unload */ + prev_session_start_hook = session_start_hook; + prev_session_end_hook = session_end_hook; + + /* Set New Hooks */ + session_start_hook = sample_session_start_hook; + session_end_hook = sample_session_end_hook; +} + +/* + * Module Unload Callback + */ +void +_PG_fini(void) +{ + /* Uninstall Hooks */ + session_start_hook = prev_session_start_hook; + session_end_hook = prev_session_end_hook; +} diff --git a/src/test/modules/test_session_hooks/test_session_hooks.control b/src/test/modules/test_session_hooks/test_session_hooks.control new file mode 100644 index 0000000..7d7ef9f --- /dev/null +++ b/src/test/modules/test_session_hooks/test_session_hooks.control @@ -0,0 +1,3 @@ +comment = 'Test start/end hook session with an extension' +default_version = '1.0' +relocatable = true
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers