Обсуждение: Support TRUNCATE triggers on foreign tables
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>
Вложения
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">—</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
AFTERstatement-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">—</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 forAFTER 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 forAFTER 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
thatfor 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
thatfor 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
thatfor 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>