From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Yotsunaga, Naoki <yotsunaga.naoki@jp.fujitsu.com>
Cc: Postgres hackers <pgsql-hackers@postgresql.org>
Subject: Re: automatic restore point
Date: Fri, 5 Oct 2018 15:26:27 +0200
Message-ID: <4566041d-2567-74d2-d135-19ff6a20fe51@2ndquadrant.com> (raw)
In-Reply-To: <20181001220632.7a4jnzph2cwntq6w@alvherre.pgsql>
References: <20181001220632.7a4jnzph2cwntq6w@alvherre.pgsql>
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
Attachments:download all (2 files, .tar.gz)0001-Test-that-event-triggers-work-in-functions-and-proce.patch (text/plain, 3.6K) download
From 7f86aabd89ce7d30873a3257f0187738d272d531 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@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.outindex 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.sqlindex 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
0002-Slightly-correct-context-check-for-event-triggers.patch (text/plain, 1.1K) download
From 6003ddd5dd7d9b0b36af7f852f2519c8e8ee670b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@2ndquadrant.com, alvherre@2ndquadrant.com, michael@paquier.xyz, yotsunaga.naoki@jp.fujitsu.com
Subject: Re: automatic restore point
In-Reply-To: <4566041d-2567-74d2-d135-19ff6a20fe51@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox