On 02/10/2018 00:06, Alvaro Herrera wrote:
> Re-reading the implementation in standard_ProcessUtility, I wonder what
> is PROCESS_UTILITY_QUERY_NONATOMIC -- there seems to be a maze through
> SPI that determines whether this flag is set or not, which could affect
> whether the event trigger is useful. Are utilities executed through a
> procedure detected by event triggers? If so, then this mechanism seems
> good enough to me. But if there's a way to sneak utility commands (DROP
> TABLE) without the event trigger being invoked, then no (and in that
> case maybe it's just a bug in procedures and we can still not include
> this patch).
It looked for a moment that
isCompleteQuery = (context <= PROCESS_UTILITY_QUERY)
in ProcessUtilitySlow() might be a problem, since that omits
PROCESS_UTILITY_QUERY_NONATOMIC, but it's not actually a problem, since
the commands that run this way (CALL and SET from PL/pgSQL) don't have
event triggers. But anyway, I propose the attached patch to rephrase
that. Also some tests that show it all works as expected.
> On the TRUNCATE case it's a bit annoying that you can't do it with event
> triggers -- you have to create individual regular triggers on truncate
> for each table (so you probably need yet another event trigger that
> creates such triggers).
I don't see why we couldn't add event triggers on TRUNCATE or other
commands such as DELETE.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 7f86aabd89ce7d30873a3257f0187738d272d531 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Fri, 5 Oct 2018 15:20:01 +0200
Subject: [PATCH 1/2] Test that event triggers work in functions and procedures
---
src/test/regress/expected/event_trigger.out | 37 ++++++++++++++++++++-
src/test/regress/sql/event_trigger.sql | 21 +++++++++++-
2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/src/test/regress/expected/event_trigger.out
b/src/test/regress/expected/event_trigger.out
index 6175a10d77..466c46e7f3 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -112,10 +112,45 @@ create table event_trigger_fire5 (a int);
NOTICE: test_event_trigger: ddl_command_start CREATE TABLE
NOTICE: test_event_trigger: ddl_command_start CREATE TABLE
NOTICE: test_event_trigger: ddl_command_end CREATE TABLE
+-- non-top-level command
+create function f1() returns int
+language plpgsql
+as $$
+begin
+ create table event_trigger_fire6 (a int);
+ return 0;
+end $$;
+NOTICE: test_event_trigger: ddl_command_start CREATE FUNCTION
+NOTICE: test_event_trigger: ddl_command_start CREATE FUNCTION
+NOTICE: test_event_trigger: ddl_command_end CREATE FUNCTION
+select f1();
+NOTICE: test_event_trigger: ddl_command_start CREATE TABLE
+NOTICE: test_event_trigger: ddl_command_start CREATE TABLE
+NOTICE: test_event_trigger: ddl_command_end CREATE TABLE
+ f1
+----
+ 0
+(1 row)
+
+-- non-top-level command
+create procedure p1()
+language plpgsql
+as $$
+begin
+ create table event_trigger_fire7 (a int);
+end $$;
+NOTICE: test_event_trigger: ddl_command_start CREATE PROCEDURE
+NOTICE: test_event_trigger: ddl_command_end CREATE PROCEDURE
+call p1();
+NOTICE: test_event_trigger: ddl_command_start CREATE TABLE
+NOTICE: test_event_trigger: ddl_command_start CREATE TABLE
+NOTICE: test_event_trigger: ddl_command_end CREATE TABLE
-- clean up
alter event trigger regress_event_trigger disable;
-drop table event_trigger_fire2, event_trigger_fire3, event_trigger_fire4,
event_trigger_fire5;
+drop table event_trigger_fire2, event_trigger_fire3, event_trigger_fire4,
event_trigger_fire5, event_trigger_fire6, event_trigger_fire7;
NOTICE: test_event_trigger: ddl_command_end DROP TABLE
+drop routine f1(), p1();
+NOTICE: test_event_trigger: ddl_command_end DROP ROUTINE
-- regress_event_trigger_end should fire on these commands
grant all on table event_trigger_fire1 to public;
NOTICE: test_event_trigger: ddl_command_end GRANT
diff --git a/src/test/regress/sql/event_trigger.sql
b/src/test/regress/sql/event_trigger.sql
index 342aef6449..545a416252 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -106,9 +106,28 @@
reset session_replication_role;
-- fires all three
create table event_trigger_fire5 (a int);
+-- non-top-level command
+create function f1() returns int
+language plpgsql
+as $$
+begin
+ create table event_trigger_fire6 (a int);
+ return 0;
+end $$;
+select f1();
+-- non-top-level command
+create procedure p1()
+language plpgsql
+as $$
+begin
+ create table event_trigger_fire7 (a int);
+end $$;
+call p1();
+
-- clean up
alter event trigger regress_event_trigger disable;
-drop table event_trigger_fire2, event_trigger_fire3, event_trigger_fire4,
event_trigger_fire5;
+drop table event_trigger_fire2, event_trigger_fire3, event_trigger_fire4,
event_trigger_fire5, event_trigger_fire6, event_trigger_fire7;
+drop routine f1(), p1();
-- regress_event_trigger_end should fire on these commands
grant all on table event_trigger_fire1 to public;
base-commit: ff347f8aff04865680c19ffc818460bb2afaad5b
--
2.19.0
From 6003ddd5dd7d9b0b36af7f852f2519c8e8ee670b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Fri, 5 Oct 2018 15:20:32 +0200
Subject: [PATCH 2/2] Slightly correct context check for event triggers
The previous check for a "complete query" omitted the new
PROCESS_UTILITY_QUERY_NONATOMIC value. This didn't actually make a
difference in practice, because only CALL and SET from PL/pgSQL run in
this state, but it's more correct to include it anyway.
---
src/backend/tcop/utility.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index b5804f64ad..d0e2d84ea2 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -943,7 +943,7 @@ ProcessUtilitySlow(ParseState *pstate,
{
Node *parsetree = pstmt->utilityStmt;
bool isTopLevel = (context == PROCESS_UTILITY_TOPLEVEL);
- bool isCompleteQuery = (context <= PROCESS_UTILITY_QUERY);
+ bool isCompleteQuery = (context !=
PROCESS_UTILITY_SUBCOMMAND);
bool needCleanup;
bool commandCollected = false;
ObjectAddress address;
--
2.19.0
