Support TRUNCATE triggers on foreign tables

Started by Yugo NAGATAover 3 years ago9 messages

nagata@sraoss.co.jp

over 3 years ago

1 attachment(s)

Hello,

I propose supporting TRUNCATE triggers on foreign tables
because some FDW now supports TRUNCATE. I think such triggers
are useful for audit logging or for preventing undesired
truncate.

Patch attached.

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata@sraoss.co.jp>

Attachments:

truncate_trigger_on_fdw.patchtext/x-diff; name=truncate_trigger_on_fdw.patchDownload

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 44457f930c..5f2ef88cf3 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -6732,9 +6732,9 @@ BEGIN
 		TG_ARGV[0], TG_OP, TG_WHEN, TG_LEVEL;
 	RETURN NULL;
 END;$$;
-CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
-CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
 CREATE OR REPLACE FUNCTION trigger_data()  RETURNS trigger
 LANGUAGE plpgsql AS $$
@@ -6821,6 +6821,9 @@ NOTICE:  OLD: (1,update),NEW: (1,updateupdate)
 NOTICE:  trig_row_after(23, skidoo) AFTER ROW UPDATE ON rem1
 NOTICE:  OLD: (1,update),NEW: (1,updateupdate)
 NOTICE:  trigger_func(<NULL>) called: action = UPDATE, when = AFTER, level = STATEMENT
+truncate rem1;
+NOTICE:  trigger_func(<NULL>) called: action = TRUNCATE, when = BEFORE, level = STATEMENT
+NOTICE:  trigger_func(<NULL>) called: action = TRUNCATE, when = AFTER, level = STATEMENT
 -- cleanup
 DROP TRIGGER trig_row_before ON rem1;
 DROP TRIGGER trig_row_after ON rem1;
@@ -7087,7 +7090,7 @@ NOTICE:  trig_row_after(23, skidoo) AFTER ROW INSERT ON rem1
 NOTICE:  NEW: (13,"test triggered !")
   ctid  
 --------
- (0,32)
+ (0,25)
 (1 row)
 
 -- cleanup
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 92d1212027..ae1fc8f58b 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1595,9 +1595,9 @@ BEGIN
 	RETURN NULL;
 END;$$;
 
-CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
-CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
 
 CREATE OR REPLACE FUNCTION trigger_data()  RETURNS trigger
@@ -1652,6 +1652,7 @@ delete from rem1;
 insert into rem1 values(1,'insert');
 update rem1 set f2  = 'update' where f1 = 1;
 update rem1 set f2 = f2 || f2;
+truncate rem1;
 
 
 -- cleanup
diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index ee42f413e9..ff234fbe65 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -131,7 +131,7 @@ CREATE [ OR REPLACE ] [ CONSTRAINT ] TRIGGER <replaceable class="parameter">name
      <row>
       <entry align="center"><command>TRUNCATE</command></entry>
       <entry align="center">&mdash;</entry>
-      <entry align="center">Tables</entry>
+      <entry align="center">Tables and foreign tables</entry>
      </row>
      <row>
       <entry align="center" morerows="1"><literal>AFTER</literal></entry>
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 13cb516752..b8db53b66d 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -295,13 +295,6 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString,
 							RelationGetRelationName(rel)),
 					 errdetail("Foreign tables cannot have INSTEAD OF triggers.")));
 
-		if (TRIGGER_FOR_TRUNCATE(stmt->events))
-			ereport(ERROR,
-					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-					 errmsg("\"%s\" is a foreign table",
-							RelationGetRelationName(rel)),
-					 errdetail("Foreign tables cannot have TRUNCATE triggers.")));
-
 		/*
 		 * We disallow constraint triggers to protect the assumption that
 		 * triggers on FKs can't be deferred.  See notes with AfterTriggers

Fujii Masao

masao.fujii@oss.nttdata.com

over 3 years ago

In reply to: Yugo NAGATA (#1)

Re: Support TRUNCATE triggers on foreign tables

On 2022/06/30 19:38, Yugo NAGATA wrote:

Hello,

I propose supporting TRUNCATE triggers on foreign tables
because some FDW now supports TRUNCATE. I think such triggers
are useful for audit logging or for preventing undesired
truncate.

Patch attached.

Thanks for the patch! It looks good to me except the following thing.

        <entry align="center"><command>TRUNCATE</command></entry>
        <entry align="center">&mdash;</entry>
-      <entry align="center">Tables</entry>
+      <entry align="center">Tables and foreign tables</entry>
       </row>

You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Yugo NAGATA

nagata@sraoss.co.jp

over 3 years ago

In reply to: Fujii Masao (#2)

1 attachment(s)

Re: Support TRUNCATE triggers on foreign tables

Hello Fujii-san,

Thank you for reviewing the patch!

On Fri, 8 Jul 2022 00:54:37 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2022/06/30 19:38, Yugo NAGATA wrote:

Hello,

I propose supporting TRUNCATE triggers on foreign tables
because some FDW now supports TRUNCATE. I think such triggers
are useful for audit logging or for preventing undesired
truncate.

Patch attached.

Thanks for the patch! It looks good to me except the following thing.
<entry align="center"><command>TRUNCATE</command></entry>
<entry align="center">&mdash;</entry>
-      <entry align="center">Tables</entry>
+      <entry align="center">Tables and foreign tables</entry>
</row>
You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No?

Oops, I forgot it. I attached the updated patch.

Regards,
Yugo Nagata

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

--
Yugo NAGATA <nagata@sraoss.co.jp>

Attachments:

v2_truncate_trigger_on_fdw.patchtext/x-diff; name=v2_truncate_trigger_on_fdw.patchDownload

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 44457f930c..5f2ef88cf3 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -6732,9 +6732,9 @@ BEGIN
 		TG_ARGV[0], TG_OP, TG_WHEN, TG_LEVEL;
 	RETURN NULL;
 END;$$;
-CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
-CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
 CREATE OR REPLACE FUNCTION trigger_data()  RETURNS trigger
 LANGUAGE plpgsql AS $$
@@ -6821,6 +6821,9 @@ NOTICE:  OLD: (1,update),NEW: (1,updateupdate)
 NOTICE:  trig_row_after(23, skidoo) AFTER ROW UPDATE ON rem1
 NOTICE:  OLD: (1,update),NEW: (1,updateupdate)
 NOTICE:  trigger_func(<NULL>) called: action = UPDATE, when = AFTER, level = STATEMENT
+truncate rem1;
+NOTICE:  trigger_func(<NULL>) called: action = TRUNCATE, when = BEFORE, level = STATEMENT
+NOTICE:  trigger_func(<NULL>) called: action = TRUNCATE, when = AFTER, level = STATEMENT
 -- cleanup
 DROP TRIGGER trig_row_before ON rem1;
 DROP TRIGGER trig_row_after ON rem1;
@@ -7087,7 +7090,7 @@ NOTICE:  trig_row_after(23, skidoo) AFTER ROW INSERT ON rem1
 NOTICE:  NEW: (13,"test triggered !")
   ctid  
 --------
- (0,32)
+ (0,25)
 (1 row)
 
 -- cleanup
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 92d1212027..ae1fc8f58b 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1595,9 +1595,9 @@ BEGIN
 	RETURN NULL;
 END;$$;
 
-CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
-CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE ON rem1
+CREATE TRIGGER trig_stmt_after AFTER DELETE OR INSERT OR UPDATE OR TRUNCATE ON rem1
 	FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
 
 CREATE OR REPLACE FUNCTION trigger_data()  RETURNS trigger
@@ -1652,6 +1652,7 @@ delete from rem1;
 insert into rem1 values(1,'insert');
 update rem1 set f2  = 'update' where f1 = 1;
 update rem1 set f2 = f2 || f2;
+truncate rem1;
 
 
 -- cleanup
diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index ee42f413e9..982ab6f3ee 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -131,7 +131,7 @@ CREATE [ OR REPLACE ] [ CONSTRAINT ] TRIGGER <replaceable class="parameter">name
      <row>
       <entry align="center"><command>TRUNCATE</command></entry>
       <entry align="center">&mdash;</entry>
-      <entry align="center">Tables</entry>
+      <entry align="center">Tables and foreign tables</entry>
      </row>
      <row>
       <entry align="center" morerows="1"><literal>AFTER</literal></entry>
@@ -142,7 +142,7 @@ CREATE [ OR REPLACE ] [ CONSTRAINT ] TRIGGER <replaceable class="parameter">name
      <row>
       <entry align="center"><command>TRUNCATE</command></entry>
       <entry align="center">&mdash;</entry>
-      <entry align="center">Tables</entry>
+      <entry align="center">Tables and foreign tables</entry>
      </row>
      <row>
       <entry align="center" morerows="1"><literal>INSTEAD OF</literal></entry>
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 13cb516752..b8db53b66d 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -295,13 +295,6 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString,
 							RelationGetRelationName(rel)),
 					 errdetail("Foreign tables cannot have INSTEAD OF triggers.")));
 
-		if (TRIGGER_FOR_TRUNCATE(stmt->events))
-			ereport(ERROR,
-					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-					 errmsg("\"%s\" is a foreign table",
-							RelationGetRelationName(rel)),
-					 errdetail("Foreign tables cannot have TRUNCATE triggers.")));
-
 		/*
 		 * We disallow constraint triggers to protect the assumption that
 		 * triggers on FKs can't be deferred.  See notes with AfterTriggers

Fujii Masao

masao.fujii@oss.nttdata.com

over 3 years ago

In reply to: Yugo NAGATA (#3)

Re: Support TRUNCATE triggers on foreign tables

On 2022/07/08 11:19, Yugo NAGATA wrote:

You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No?

Oops, I forgot it. I attached the updated patch.

Thanks for updating the patch! LGTM.
Barring any objection, I will commit the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Ian Lawrence Barwick

barwick@gmail.com

over 3 years ago

In reply to: Fujii Masao (#4)

Re: Support TRUNCATE triggers on foreign tables

2022年7月8日(金) 14:06 Fujii Masao <masao.fujii@oss.nttdata.com>:

On 2022/07/08 11:19, Yugo NAGATA wrote:

You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No?

Oops, I forgot it. I attached the updated patch.

Thanks for updating the patch! LGTM.
Barring any objection, I will commit the patch.

An observation: as-is the patch would make it possible to create a truncate
trigger for a foreign table whose FDW doesn't support truncation, which seems
somewhat pointless, possible source of confusion etc.:

postgres=# CREATE TRIGGER ft_trigger
AFTER TRUNCATE ON fb_foo
EXECUTE FUNCTION fb_foo_trg();
CREATE TRIGGER

postgres=# TRUNCATE fb_foo;
ERROR: cannot truncate foreign table "fb_foo"

It would be easy enough to check for this, e.g.:

else if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
{
FdwRoutine *fdwroutine = GetFdwRoutineForRelation(rel, false);

if (!fdwroutine->ExecForeignTruncate)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("foreign data wrapper does not support
table truncation")));
...

which results in:

postgres=# CREATE TRIGGER ft_trigger
AFTER TRUNCATE ON fb_foo
EXECUTE FUNCTION fb_foo_trg();
ERROR: foreign data wrapper does not support table truncation

which IMO is preferable to silently accepting DDL which will never
actually do anything.

Regards

Ian Barwick

Yugo NAGATA

nagata@sraoss.co.jp

over 3 years ago

In reply to: Ian Lawrence Barwick (#5)

Re: Support TRUNCATE triggers on foreign tables

On Fri, 8 Jul 2022 16:50:10 +0900
Ian Lawrence Barwick <barwick@gmail.com> wrote:

2022年7月8日(金) 14:06 Fujii Masao <masao.fujii@oss.nttdata.com>:

On 2022/07/08 11:19, Yugo NAGATA wrote:

You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No?

Oops, I forgot it. I attached the updated patch.

Thanks for updating the patch! LGTM.
Barring any objection, I will commit the patch.

An observation: as-is the patch would make it possible to create a truncate
trigger for a foreign table whose FDW doesn't support truncation, which seems
somewhat pointless, possible source of confusion etc.:

postgres=# CREATE TRIGGER ft_trigger
AFTER TRUNCATE ON fb_foo
EXECUTE FUNCTION fb_foo_trg();
CREATE TRIGGER

postgres=# TRUNCATE fb_foo;
ERROR: cannot truncate foreign table "fb_foo"

It would be easy enough to check for this, e.g.:

else if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
{
FdwRoutine *fdwroutine = GetFdwRoutineForRelation(rel, false);

if (!fdwroutine->ExecForeignTruncate)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("foreign data wrapper does not support
table truncation")));
...

which results in:

postgres=# CREATE TRIGGER ft_trigger
AFTER TRUNCATE ON fb_foo
EXECUTE FUNCTION fb_foo_trg();
ERROR: foreign data wrapper does not support table truncation

which IMO is preferable to silently accepting DDL which will never
actually do anything.

At beginning, I also thought such check would be necessary, but I noticed that
it is already possible to create insert/delete/update triggers for a foreign
table whose FDW doesn't support such operations. So, I discarded this idea from
the proposed patch for consistency.

If we want to add such prevention, we will need similar checks for
INSERT/DELETE/UPDATE not only TRUNCATE. However, I think such fix is independent
from this and it can be proposed as another patch.

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata@sraoss.co.jp>

Ian Lawrence Barwick

barwick@gmail.com

over 3 years ago

In reply to: Yugo NAGATA (#6)

Re: Support TRUNCATE triggers on foreign tables

2022年7月8日(金) 17:10 Yugo NAGATA <nagata@sraoss.co.jp>:

On Fri, 8 Jul 2022 16:50:10 +0900
Ian Lawrence Barwick <barwick@gmail.com> wrote:

2022年7月8日(金) 14:06 Fujii Masao <masao.fujii@oss.nttdata.com>:

On 2022/07/08 11:19, Yugo NAGATA wrote:

You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No?

Oops, I forgot it. I attached the updated patch.

Thanks for updating the patch! LGTM.
Barring any objection, I will commit the patch.

An observation: as-is the patch would make it possible to create a truncate
trigger for a foreign table whose FDW doesn't support truncation, which seems
somewhat pointless, possible source of confusion etc.:

postgres=# CREATE TRIGGER ft_trigger
AFTER TRUNCATE ON fb_foo
EXECUTE FUNCTION fb_foo_trg();
CREATE TRIGGER

postgres=# TRUNCATE fb_foo;
ERROR: cannot truncate foreign table "fb_foo"

It would be easy enough to check for this, e.g.:

else if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
{
FdwRoutine *fdwroutine = GetFdwRoutineForRelation(rel, false);

if (!fdwroutine->ExecForeignTruncate)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("foreign data wrapper does not support
table truncation")));
...

which results in:

postgres=# CREATE TRIGGER ft_trigger
AFTER TRUNCATE ON fb_foo
EXECUTE FUNCTION fb_foo_trg();
ERROR: foreign data wrapper does not support table truncation

which IMO is preferable to silently accepting DDL which will never
actually do anything.

At beginning, I also thought such check would be necessary, but I noticed that
it is already possible to create insert/delete/update triggers for a foreign
table whose FDW doesn't support such operations. So, I discarded this idea from
the proposed patch for consistency.

If we want to add such prevention, we will need similar checks for
INSERT/DELETE/UPDATE not only TRUNCATE. However, I think such fix is independent
from this and it can be proposed as another patch.

Ah OK, makes sense from that point of view. Thanks for the clarification!

Regards

Ian Barwick

Fujii Masao

masao.fujii@oss.nttdata.com

over 3 years ago

In reply to: Ian Lawrence Barwick (#7)

Re: Support TRUNCATE triggers on foreign tables

On 2022/07/08 17:13, Ian Lawrence Barwick wrote:

If we want to add such prevention, we will need similar checks for
INSERT/DELETE/UPDATE not only TRUNCATE. However, I think such fix is independent
from this and it can be proposed as another patch.

Ah OK, makes sense from that point of view. Thanks for the clarification!

So I pushed the v2 patch that Yugo-san posted. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Yugo NAGATA

nagata@sraoss.co.jp

over 3 years ago

In reply to: Fujii Masao (#8)

Re: Support TRUNCATE triggers on foreign tables

On Tue, 12 Jul 2022 09:24:20 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2022/07/08 17:13, Ian Lawrence Barwick wrote:

If we want to add such prevention, we will need similar checks for
INSERT/DELETE/UPDATE not only TRUNCATE. However, I think such fix is independent
from this and it can be proposed as another patch.

Ah OK, makes sense from that point of view. Thanks for the clarification!

So I pushed the v2 patch that Yugo-san posted. Thanks!

Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

--
Yugo NAGATA <nagata@sraoss.co.jp>