On 14.09.2020 12:44, Pavel Stehule wrote:
Hi
I am checking last patch, and there are notices
1. disable_session_start_trigger should be SU_BACKEND instead SUSET
2. The documentation should be enhanced - there is not any note about
behave when there are unhandled exceptions, about motivation for this
event trigger
3. regress tests should be enhanced - the cases with exceptions are
not tested
4. This trigger is not executed again after RESET ALL or DISCARD ALL -
it can be a problem if somebody wants to use this trigger for
initialisation of some session objects with some pooling solutions.
5. The handling errors don't work well for canceling. If event trigger
waits for some event, then cancel disallow connect although connected
user is superuser
CREATE OR REPLACE FUNCTION on_login_proc2() RETURNS EVENT_TRIGGER AS
$$ begin perform pg_sleep(10000); raise notice '%', fx1(100);raise
notice 'kuku kuku'; end $$ language plpgsql;
probably nobody will use pg_sleep in this routine, but there can be
wait on some locks ...
Regards
Pavel
Hi
Thank you very much for looking at my patch for connection triggers.
I have fixed 1-3 issues in the attached patch.
Concerning 4 and 5 I have some doubts:
4. Should I add some extra GUC to allow firing of session_start trigger
in case of RESET ALL or DISCARD ALL ?
Looks like such behavior contradicts with event name "session_start"...
And do we really need it? If some pooler is using RESET ALL/DISCARD ALL
to emulate session semantic then most likely it provides way to define
custom actions which
should be perform for session initialization. As far as I know, for
example pgbouncer allows do define own on-connect hooks.
5. I do not quite understand your concern. If I define trigger
procedure which is blocked (for example as in your example), then I can
use pg_cancel_backend to interrupt execution of login trigger and
superuser can login. What should be changed here?
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml
index 60366a9..f7c3c14 100644
--- a/doc/src/sgml/event-trigger.sgml
+++ b/doc/src/sgml/event-trigger.sgml
@@ -28,12 +28,31 @@
An event trigger fires whenever the event with which it is associated
occurs in the database in which it is defined. Currently, the only
supported events are
+ <literal>session_start</literal>,
<literal>ddl_command_start</literal>,
<literal>ddl_command_end</literal>,
<literal>table_rewrite</literal>
and <literal>sql_drop</literal>.
Support for additional events may be added in future releases.
</para>
+<literal>table_rewrite</literal>
+ <para>
+ The <literal>session_start</literal> event occurs on backend start when connection with user was established.
+ As far as bug in trigger procedure can prevent normal login to the system, there are two mechanisms
+ for preventing it:
+ <itemizedlist>
+ <listitem>
+ <para>
+ GUC <literal>disable_session_start_trigger</literal> which makes it possible to disable firing triggers on session startup.
+ </para>
+ <listitem>
+ <para>
+ Ignoring errors in trigger procedure for superuser. Error message is delivered to client as <literal>NOTICE</literal>
+ in this case
+ </para>
+ </listitem>
+ <itemizedlist>
+ </para>
<para>
The <literal>ddl_command_start</literal> event occurs just before the
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 7844880..a341fee 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -48,6 +48,8 @@
#include "utils/rel.h"
#include "utils/syscache.h"
+bool disable_session_start_trigger;
+
typedef struct EventTriggerQueryState
{
/* memory context for this state's objects */
@@ -130,6 +132,7 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
strcmp(stmt->eventname, "ddl_command_end") != 0 &&
strcmp(stmt->eventname, "sql_drop") != 0 &&
+ strcmp(stmt->eventname, "session_start") != 0 &&
strcmp(stmt->eventname, "table_rewrite") != 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -562,6 +565,9 @@ EventTriggerCommonSetup(Node *parsetree,
ListCell *lc;
List *runlist = NIL;
+ /* Get the command tag. */
+ tag = parsetree ? CreateCommandTag(parsetree) : CMDTAG_CONNECT;
+
/*
* We want the list of command tags for which this procedure is actually
* invoked to match up exactly with the list that CREATE EVENT TRIGGER
@@ -577,22 +583,18 @@ EventTriggerCommonSetup(Node *parsetree,
* relevant command tag.
*/
#ifdef USE_ASSERT_CHECKING
+ if (event == EVT_DDLCommandStart ||
+ event == EVT_DDLCommandEnd ||
+ event == EVT_SQLDrop ||
+ event == EVT_Connect)
{
- CommandTag dbgtag;
-
- dbgtag = CreateCommandTag(parsetree);
- if (event == EVT_DDLCommandStart ||
- event == EVT_DDLCommandEnd ||
- event == EVT_SQLDrop)
- {
- if (!command_tag_event_trigger_ok(dbgtag))
- elog(ERROR, "unexpected command tag \"%s\"", GetCommandTagName(dbgtag));
- }
- else if (event == EVT_TableRewrite)
- {
- if (!command_tag_table_rewrite_ok(dbgtag))
- elog(ERROR, "unexpected command tag \"%s\"", GetCommandTagName(dbgtag));
- }
+ if (!command_tag_event_trigger_ok(tag))
+ elog(ERROR, "unexpected command tag \"%s\"", GetCommandTagName(tag));
+ }
+ else if (event == EVT_TableRewrite)
+ {
+ if (!command_tag_table_rewrite_ok(tag))
+ elog(ERROR, "unexpected command tag \"%s\"", GetCommandTagName(tag));
}
#endif
@@ -601,9 +603,6 @@ EventTriggerCommonSetup(Node *parsetree,
if (cachelist == NIL)
return NIL;
- /* Get the command tag. */
- tag = CreateCommandTag(parsetree);
-
/*
* Filter list of event triggers by command tag, and copy them into our
* memory context. Once we start running the command triggers, or indeed
@@ -800,6 +799,66 @@ EventTriggerSQLDrop(Node *parsetree)
list_free(runlist);
}
+/*
+ * Fire connect triggers.
+ */
+void
+EventTriggerOnConnect(void)
+{
+ List *runlist;
+ EventTriggerData trigdata;
+
+ /*
+ * See EventTriggerDDLCommandStart for a discussion about why event
+ * triggers are disabled in single user mode.
+ */
+ if (!IsUnderPostmaster || !OidIsValid(MyDatabaseId) || disable_session_start_trigger)
+ return;
+
+ StartTransactionCommand();
+
+ runlist = EventTriggerCommonSetup(NULL,
+ EVT_Connect, "connect",
+ &trigdata);
+
+ if (runlist != NIL)
+ {
+ MemoryContext old_context = CurrentMemoryContext;
+ bool is_superuser = superuser();
+ /*
+ * Make sure anything the main command did will be visible to the event
+ * triggers.
+ */
+ CommandCounterIncrement();
+
+ /* Run the triggers. */
+ PG_TRY();
+ {
+ EventTriggerInvoke(runlist, &trigdata);
+ list_free(runlist);
+ }
+ PG_CATCH();
+ {
+ ErrorData* error;
+ /*
+ * Try to ignore error for superuser to make it possible to login even in case of errors
+ * during trigger execution
+ */
+ if (!is_superuser)
+ PG_RE_THROW();
+
+ MemoryContextSwitchTo(old_context);
+ error = CopyErrorData();
+ FlushErrorState();
+ elog(NOTICE, "start_session trigger failed with message %s", error->message);
+ AbortCurrentTransaction();
+ return;
+ }
+ PG_END_TRY();
+ }
+ CommitTransactionCommand();
+}
+
/*
* Fire table_rewrite triggers.
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index c9424f1..008e574 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -42,6 +42,7 @@
#include "catalog/pg_type.h"
#include "commands/async.h"
#include "commands/prepare.h"
+#include "commands/event_trigger.h"
#include "executor/spi.h"
#include "jit/jit.h"
#include "libpq/libpq.h"
@@ -167,6 +168,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 = EventTriggerOnConnect;
+
/* ----------------------------------------------------------------
* decls for routines only used in this file
* ----------------------------------------------------------------
@@ -4017,6 +4021,11 @@ 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/cache/evtcache.c b/src/backend/utils/cache/evtcache.c
index 73d091d..6eaac13 100644
--- a/src/backend/utils/cache/evtcache.c
+++ b/src/backend/utils/cache/evtcache.c
@@ -169,6 +169,8 @@ BuildEventTriggerCache(void)
event = EVT_SQLDrop;
else if (strcmp(evtevent, "table_rewrite") == 0)
event = EVT_TableRewrite;
+ else if (strcmp(evtevent, "session_start") == 0)
+ event = EVT_Connect;
else
continue;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 73518d9..5ced24f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -43,6 +43,7 @@
#include "commands/async.h"
#include "commands/prepare.h"
#include "commands/trigger.h"
+#include "commands/event_trigger.h"
#include "commands/user.h"
#include "commands/vacuum.h"
#include "commands/variable.h"
@@ -928,6 +929,16 @@ static const unit_conversion time_unit_conversion_table[] =
static struct config_bool ConfigureNamesBool[] =
{
{
+ {"disable_session_start_trigger", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
+ gettext_noop("Disable on session_start event trigger."),
+ gettext_noop("In case of errors in ON session_start EVENT TRIGGER procedure this GUC can be used to disable trigger activation and provide access to the database."),
+ GUC_EXPLAIN
+ },
+ &disable_session_start_trigger,
+ false,
+ NULL, NULL, NULL
+ },
+ {
{"enable_seqscan", PGC_USERSET, QUERY_TUNING_METHOD,
gettext_noop("Enables the planner's use of sequential-scan plans."),
NULL,
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index 407fd6a..61c096d 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -21,6 +21,8 @@
#include "tcop/deparse_utility.h"
#include "utils/aclchk_internal.h"
+extern bool disable_session_start_trigger; /* GUC */
+
typedef struct EventTriggerData
{
NodeTag type;
@@ -53,6 +55,7 @@ extern void EventTriggerDDLCommandStart(Node *parsetree);
extern void EventTriggerDDLCommandEnd(Node *parsetree);
extern void EventTriggerSQLDrop(Node *parsetree);
extern void EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason);
+extern void EventTriggerOnConnect(void);
extern bool EventTriggerBeginCompleteQuery(void);
extern void EventTriggerEndCompleteQuery(void);
diff --git a/src/include/tcop/cmdtaglist.h b/src/include/tcop/cmdtaglist.h
index 8ef0f55..cba70b3 100644
--- a/src/include/tcop/cmdtaglist.h
+++ b/src/include/tcop/cmdtaglist.h
@@ -80,6 +80,7 @@ PG_CMDTAG(CMDTAG_CLUSTER, "CLUSTER", false, false, false)
PG_CMDTAG(CMDTAG_COMMENT, "COMMENT", true, false, false)
PG_CMDTAG(CMDTAG_COMMIT, "COMMIT", false, false, false)
PG_CMDTAG(CMDTAG_COMMIT_PREPARED, "COMMIT PREPARED", false, false, false)
+PG_CMDTAG(CMDTAG_CONNECT, "CONNECT", true, false, false)
PG_CMDTAG(CMDTAG_COPY, "COPY", false, false, true)
PG_CMDTAG(CMDTAG_COPY_FROM, "COPY FROM", false, false, false)
PG_CMDTAG(CMDTAG_CREATE_ACCESS_METHOD, "CREATE ACCESS METHOD", true, false, false)
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index bd30607..be71020 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -30,6 +30,11 @@ 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);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
/* GUC-configurable parameters */
typedef enum
diff --git a/src/include/utils/evtcache.h b/src/include/utils/evtcache.h
index bb1e39e..ab7e8c3 100644
--- a/src/include/utils/evtcache.h
+++ b/src/include/utils/evtcache.h
@@ -22,7 +22,8 @@ typedef enum
EVT_DDLCommandStart,
EVT_DDLCommandEnd,
EVT_SQLDrop,
- EVT_TableRewrite
+ EVT_TableRewrite,
+ EVT_Connect,
} EventTriggerEvent;
typedef struct
diff --git a/src/test/recovery/t/000_session_start_trigger.pl b/src/test/recovery/t/000_session_start_trigger.pl
new file mode 100644
index 0000000..fa82e0a
--- /dev/null
+++ b/src/test/recovery/t/000_session_start_trigger.pl
@@ -0,0 +1,69 @@
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More;
+if (!$use_unix_sockets)
+{
+ plan skip_all =>
+ "authentication tests cannot run without Unix-domain sockets";
+}
+else
+{
+ plan tests => 5;
+}
+
+# Initialize master node
+my $node = get_new_node('master');
+$node->init;
+$node->start;
+$node->safe_psql('postgres', q{
+CREATE ROLE regress_user LOGIN PASSWORD 'pass';
+CREATE ROLE regress_hacker LOGIN PASSWORD 'pass';
+
+CREATE TABLE connects(id serial, who text);
+
+CREATE FUNCTION on_login_proc() RETURNS EVENT_TRIGGER AS $$
+BEGIN
+ IF NOT pg_is_in_recovery() THEN
+ INSERT INTO connects (who) VALUES (session_user);
+ END IF;
+ IF session_user = 'regress_hacker' THEN
+ RAISE EXCEPTION 'You are not welcome!';
+ END IF;
+ RAISE NOTICE 'You are welcome!';
+END;
+$$ LANGUAGE plpgsql SECURITY DEFINER;
+
+CREATE EVENT TRIGGER on_login_trigger ON session_start EXECUTE FUNCTION on_login_proc();
+ALTER EVENT TRIGGER on_login_trigger ENABLE ALWAYS;
+}
+);
+my $res;
+
+$res = $node->safe_psql('postgres', "SELECT 1");
+
+$res = $node->safe_psql('postgres', "SELECT 1",
+ extra_params => [ '-U', 'regress_user', '-w' ]);
+
+my ($ret, $stdout, $stderr) = $node->psql('postgres', "SELECT 1",
+ extra_params => [ '-U', 'regress_hacker', '-w' ]);
+ok( $ret != 0 && $stderr =~ /You are not welcome!/ );
+
+$res = $node->safe_psql('postgres', "SELECT COUNT(1) FROM connects WHERE who = 'regress_user'");
+ok($res == 1);
+
+my $tempdir = TestLib::tempdir;
+command_ok(
+ [ "pg_dumpall", '-p', $node->port, '-c', "--file=$tempdir/regression_dump.sql", ],
+ "dumpall");
+# my $dump_contents = slurp_file("$tempdir/regression_dump.sql");
+# print($dump_contents);
+
+my $node1 = get_new_node('secondary');
+$node1->init;
+$node1->start;
+command_ok(["psql", '-p', $node1->port, '-b', '-f', "$tempdir/regression_dump.sql" ] );
+$res = $node1->safe_psql('postgres', "SELECT 1", extra_params => [ '-U', 'regress_user', '-w' ]);
+$res = $node1->safe_psql('postgres', "SELECT COUNT(1) FROM connects WHERE who = 'regress_user'");
+ok($res == 2);
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 9e31a53..5e9dc5f 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -43,6 +43,27 @@ $node_standby_2->start;
$node_primary->safe_psql('postgres',
"CREATE TABLE tab_int AS SELECT generate_series(1,1002) AS a");
+$node_primary->safe_psql('postgres', q{
+CREATE ROLE regress_user LOGIN PASSWORD 'pass';
+
+CREATE TABLE connects(id serial, who text);
+
+CREATE FUNCTION on_login_proc() RETURNS EVENT_TRIGGER AS $$
+BEGIN
+ IF NOT pg_is_in_recovery() THEN
+ INSERT INTO connects (who) VALUES (session_user);
+ END IF;
+ IF session_user = 'regress_hacker' THEN
+ RAISE EXCEPTION 'You are not welcome!';
+ END IF;
+ RAISE NOTICE 'You are welcome!';
+END;
+$$ LANGUAGE plpgsql SECURITY DEFINER;
+
+CREATE EVENT TRIGGER on_login_trigger ON session_start EXECUTE FUNCTION on_login_proc();
+ALTER EVENT TRIGGER on_login_trigger ENABLE ALWAYS;
+});
+
# Wait for standbys to catch up
$node_primary->wait_for_catchup($node_standby_1, 'replay',
$node_primary->lsn('insert'));
@@ -266,6 +287,9 @@ sub replay_check
replay_check();
+$node_standby_1->safe_psql('postgres', "SELECT 1", extra_params => [ '-U', 'regress_user', '-w' ]);
+$node_standby_2->safe_psql('postgres', "SELECT 2", extra_params => [ '-U', 'regress_user', '-w' ]);
+
note "enabling hot_standby_feedback";
# Enable hs_feedback. The slot should gain an xmin. We set the status interval
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index bdd0ffc..e0b5196 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -536,3 +536,40 @@ NOTICE: DROP POLICY - ddl_command_end
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+-- On session start triggers
+create table connects(id serial, who text);
+create function on_login_proc() returns event_trigger as $$
+begin
+ insert into connects (who) values ('I am');
+ raise notice 'You are welcome!';
+end;
+$$ language plpgsql;
+create event trigger on_login_trigger on session_start execute procedure on_login_proc();
+alter event trigger on_login_trigger enable always;
+\c
+NOTICE: You are welcome!
+select * from connects;
+ id | who
+----+------
+ 1 | I am
+(1 row)
+
+\c
+NOTICE: You are welcome!
+select * from connects;
+ id | who
+----+------
+ 1 | I am
+ 2 | I am
+(2 rows)
+
+-- Test handing exeptions in session_start trigger
+drop table connects;
+-- superuser should ignore error
+\c
+NOTICE: start_session trigger failed with message relation "connects" does not exist
+-- suppress troigger firing
+\c "dbname=regression options='-c disable_session_start_trigger=true'"
+-- Cleanup
+drop event trigger on_login_trigger;
+drop function on_login_proc();
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 18b2a26..82dacdd 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -429,3 +429,31 @@ DROP POLICY p2 ON event_trigger_test;
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+
+-- On session start triggers
+create table connects(id serial, who text);
+create function on_login_proc() returns event_trigger as $$
+begin
+ insert into connects (who) values ('I am');
+ raise notice 'You are welcome!';
+end;
+$$ language plpgsql;
+create event trigger on_login_trigger on session_start execute procedure on_login_proc();
+alter event trigger on_login_trigger enable always;
+\c
+select * from connects;
+\c
+select * from connects;
+
+-- Test handing exeptions in session_start trigger
+
+drop table connects;
+-- superuser should ignore error
+\c
+-- suppress troigger firing
+\c "dbname=regression options='-c disable_session_start_trigger=true'"
+
+
+-- Cleanup
+drop event trigger on_login_trigger;
+drop function on_login_proc();
