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

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>

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

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

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>

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

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

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>