Horton

Thread

Commits

  1. Support TRUNCATE triggers on foreign tables.

  1. Support TRUNCATE triggers on foreign tables

    Yugo Nagata <nagata@sraoss.co.jp> — 2022-06-30T10:38:48Z

    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>

  2. Re: Support TRUNCATE triggers on foreign tables

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-07-07T15:54:37Z

    
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

  3. Re: Support TRUNCATE triggers on foreign tables

    Yugo Nagata <nagata@sraoss.co.jp> — 2022-07-08T02:19:59Z

    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>

  4. Re: Support TRUNCATE triggers on foreign tables

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-07-08T05:06:40Z

    
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

  5. Re: Support TRUNCATE triggers on foreign tables

    Ian Lawrence Barwick <barwick@gmail.com> — 2022-07-08T07:50:10Z

    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

  6. Re: Support TRUNCATE triggers on foreign tables

    Yugo Nagata <nagata@sraoss.co.jp> — 2022-07-08T08:10:11Z

    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>

  7. Re: Support TRUNCATE triggers on foreign tables

    Ian Lawrence Barwick <barwick@gmail.com> — 2022-07-08T08:13:32Z

    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

  8. Re: Support TRUNCATE triggers on foreign tables

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-07-12T00:24:20Z

    
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

  9. Re: Support TRUNCATE triggers on foreign tables

    Yugo Nagata <nagata@sraoss.co.jp> — 2022-07-12T07:29:51Z

    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>