Horton

Thread

Commits

  1. Prohibit transition tables on views and foreign tables.

  1. Transition tables for triggers on foreign tables and views

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-25T23:17:05Z

    Hi hackers,

My colleague Prabhat Sahu reported off list that transition tables
don't work for views.  I probably should have thought about that when
I fixed something similar for partitioned tables, and after some
experimentation I see that this is also broken for foreign tables.

For foreign tables using postgres_fdw, I see that transition tables
capture new rows for INSERT but capture nothing for DELETE and UPDATE.

For views, aside from the question of transition tables, I noticed
that statement triggers don't seem to fire at all with updatable
views.  Surely they should -- isn't that a separate bug?

Example:

  create table my_table (i int);
  create view my_view as select * from my_table;
  create function my_trigger_function() returns trigger language plpgsql as
    $$ begin raise warning 'hello world'; return null; end; $$;
  create trigger my_trigger after insert or update or delete on my_view
    for each statement execute procedure my_trigger_function();
  insert into my_view values (42);

... and the world remains ungreeted.

As for transition tables, there are probably meaningful ways to
support those for both views and foreign tables at least in certain
cases, as future feature enhancements.  For now, do you agree that we
should reject such triggers as unsupported?  See attached.

Separately, I noticed an obsolete sentence in the trigger
documentation.  See doc.patch.

-- 
Thomas Munro
http://www.enterprisedb.com

  2. Re: Transition tables for triggers on foreign tables and views

    Noah Misch <noah@leadboat.com> — 2017-04-28T05:07:31Z

    On Wed, Apr 26, 2017 at 11:17:05AM +1200, Thomas Munro wrote:
> My colleague Prabhat Sahu reported off list that transition tables
> don't work for views.  I probably should have thought about that when
> I fixed something similar for partitioned tables, and after some
> experimentation I see that this is also broken for foreign tables.
> 
> For foreign tables using postgres_fdw, I see that transition tables
> capture new rows for INSERT but capture nothing for DELETE and UPDATE.
> 
> For views, aside from the question of transition tables, I noticed
> that statement triggers don't seem to fire at all with updatable
> views.  Surely they should -- isn't that a separate bug?
> 
> Example:
> 
>   create table my_table (i int);
>   create view my_view as select * from my_table;
>   create function my_trigger_function() returns trigger language plpgsql as
>     $$ begin raise warning 'hello world'; return null; end; $$;
>   create trigger my_trigger after insert or update or delete on my_view
>     for each statement execute procedure my_trigger_function();
>   insert into my_view values (42);
> 
> ... and the world remains ungreeted.
> 
> As for transition tables, there are probably meaningful ways to
> support those for both views and foreign tables at least in certain
> cases, as future feature enhancements.  For now, do you agree that we
> should reject such triggers as unsupported?  See attached.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Kevin,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

  3. Re: Transition tables for triggers on foreign tables and views

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-28T18:52:58Z

    On Tue, Apr 25, 2017 at 6:17 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

> My colleague Prabhat Sahu reported off list that transition tables
> don't work for views.  I probably should have thought about that when
> I fixed something similar for partitioned tables, and after some
> experimentation I see that this is also broken for foreign tables.

Good spot.  Don't know why that didn't occur to me to look at.

> For foreign tables using postgres_fdw, I see that transition tables
> capture new rows for INSERT but capture nothing for DELETE and UPDATE.

Will dig into that in a bit, but first...

> For views, aside from the question of transition tables, I noticed
> that statement triggers don't seem to fire at all with updatable
> views.  Surely they should -- isn't that a separate bug?
>
> Example:
>
>   create table my_table (i int);
>   create view my_view as select * from my_table;
>   create function my_trigger_function() returns trigger language plpgsql as
>     $$ begin raise warning 'hello world'; return null; end; $$;
>   create trigger my_trigger after insert or update or delete on my_view
>     for each statement execute procedure my_trigger_function();
>   insert into my_view values (42);
>
> ... and the world remains ungreeted.

I checked out 25dc142a (before any of my commits for $subject),
built it, and tried the above -- with no warning generated.  I then
used an UPDATE and DELETE against the view, also with no trigger
fired (nor any error during trigger creation or DML).  Does anyone
know whether such trigger ever fired at any point in the commit
history?  Should we remove the documentation that anything except
INSTEAD OF triggers work on a view, and generate errors for attempt
to do otherwise, or is there something to salvage that has worked at
some point?  If we do get these working, don't they deserve at least
one regression test?

Will post again after I have a chance to review the FDW issue.

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/

  4. Re: Transition tables for triggers on foreign tables and views

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-28T19:42:46Z

    Kevin Grittner <kgrittn@gmail.com> writes:
> On Tue, Apr 25, 2017 at 6:17 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> For views, aside from the question of transition tables, I noticed
>> that statement triggers don't seem to fire at all with updatable
>> views.  Surely they should -- isn't that a separate bug?

> I checked out 25dc142a (before any of my commits for $subject),
> built it, and tried the above -- with no warning generated.  I then
> used an UPDATE and DELETE against the view, also with no trigger
> fired (nor any error during trigger creation or DML).  Does anyone
> know whether such trigger ever fired at any point in the commit
> history?

[ experiments... ]  They did, and do, fire if you do it the old-style
way with an INSTEAD OF row trigger.  They don't fire if you're relying
on an updatable view.  It seems we fire the table's statement triggers
instead, ie, the update is fully translated into an update on the
underlying table.

I'm not sure how intentional that was, but it's not a completely
unreasonable definition on its face, and given the lack of field
complaints since 9.3, I think we should probably stick to it.
However, if you didn't understand that from the documentation,
then we have a documentation problem.

> If we do get these working, don't they deserve at least
> one regression test?

Are you sure there isn't one?

			regards, tom lane

  5. Re: Transition tables for triggers on foreign tables and views

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-28T20:54:04Z

    On Fri, Apr 28, 2017 at 2:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> On Tue, Apr 25, 2017 at 6:17 PM, Thomas Munro
>> <thomas.munro@enterprisedb.com> wrote:
>>> For views, aside from the question of transition tables, I noticed
>>> that statement triggers don't seem to fire at all with updatable
>>> views.  Surely they should -- isn't that a separate bug?
>
>> I checked out 25dc142a (before any of my commits for $subject),
>> built it, and tried the above -- with no warning generated.  I then
>> used an UPDATE and DELETE against the view, also with no trigger
>> fired (nor any error during trigger creation or DML).  Does anyone
>> know whether such trigger ever fired at any point in the commit
>> history?
>
> [ experiments... ]  They did, and do, fire if you do it the old-style
> way with an INSTEAD OF row trigger.

Here is the table from near the start of CREATE TRIGGER docs,
"folded" such that I hope it remains intelligible in a fixed-width
font after email systems get done with it:

When
  Event
    Row-level                  Statement-level

BEFORE
  INSERT/UPDATE/DELETE
    Tables and foreign tables  Tables, views, and foreign tables
  TRUNCATE
    —                          Tables

AFTER
  INSERT/UPDATE/DELETE
    Tables and foreign tables  Tables, views, and foreign tables
  TRUNCATE
    —                          Tables

INSTEAD OF
  INSERT/UPDATE/DELETE
    Views                      —
  TRUNCATE
    —                          —

The claim seems to be that you can create a { BEFORE | AFTER } {
event [ OR ... ] } ...  FOR EACH STATEMENT trigger where event is {
INSERT | UPDATE | DELETE } on an updateable view.  Well, you can
*create* it, but it will never fire.

> They don't fire if you're relying
> on an updatable view.  It seems we fire the table's statement triggers
> instead, ie, the update is fully translated into an update on the
> underlying table.

Well, certainly that should *also* happen.  Not firing a table's DML
because it was fired off a view would be crazy, or so it seems to
me.

> I'm not sure how intentional that was, but it's not a completely
> unreasonable definition on its face, and given the lack of field
> complaints since 9.3, I think we should probably stick to it.

Are you talking about being able to create INSERT, UPDATE, and
DELETE triggers on the view (which never fire), or about firing
triggers on the table when an INSTEAD OF trigger is fired.

> However, if you didn't understand that from the documentation,
> then we have a documentation problem.

I totally get that there are INSTEAD OF triggers and have no quibble
with how they behave.  I can't even argue that the above chart is
wrong in terms of what CREATE TRIGGER allows.  However, creating
triggers that can never fire seems entirely wrong.

>> If we do get these working, don't they deserve at least
>> one regression test?
>
> Are you sure there isn't one?

Well, I was sort of hoping that the triggers that can now be defined
but can never fire *did* fire at some point.  But if that were true,
and they subsequently were broken, it would mean we lacked
regression tests for that case.

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/

  6. Re: Transition tables for triggers on foreign tables and views

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-28T20:57:40Z

    On Fri, Apr 28, 2017 at 3:54 PM, Kevin Grittner <kgrittn@gmail.com> wrote:

> Not firing a table's DML because it was fired off a view would be crazy,

should read:

Not firing a table's trigger because the trigger is on DML run from a
view's INSTEAD OF trigger would be crazy,

-- 
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/

  7. Re: Transition tables for triggers on foreign tables and views

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-29T03:56:18Z

    Kevin Grittner <kgrittn@gmail.com> writes:
> Well, I was sort of hoping that the triggers that can now be defined
> but can never fire *did* fire at some point.

They will fire if you have an INSTEAD OF row-level trigger; the existence
of that trigger is what determines whether we implement DML on a view
through the view's own triggers or through translation to an action on
the underlying table.

I do not think it'd be reasonable to throw an error for creation of
a statement-level view trigger when there's no row-level trigger,
because that just imposes a hard-to-deal-with DDL ordering dependency.

You could make a case for having the updatable-view translation code
print a WARNING if it notices that there are statement-level triggers
that cannot be fired due to the translation.

			regards, tom lane

  8. Re: Transition tables for triggers on foreign tables and views

    Noah Misch <noah@leadboat.com> — 2017-05-02T04:28:06Z

    On Fri, Apr 28, 2017 at 05:07:31AM +0000, Noah Misch wrote:
> On Wed, Apr 26, 2017 at 11:17:05AM +1200, Thomas Munro wrote:
> > My colleague Prabhat Sahu reported off list that transition tables
> > don't work for views.  I probably should have thought about that when
> > I fixed something similar for partitioned tables, and after some
> > experimentation I see that this is also broken for foreign tables.
> > 
> > For foreign tables using postgres_fdw, I see that transition tables
> > capture new rows for INSERT but capture nothing for DELETE and UPDATE.
> > 
> > For views, aside from the question of transition tables, I noticed
> > that statement triggers don't seem to fire at all with updatable
> > views.  Surely they should -- isn't that a separate bug?
> > 
> > Example:
> > 
> >   create table my_table (i int);
> >   create view my_view as select * from my_table;
> >   create function my_trigger_function() returns trigger language plpgsql as
> >     $$ begin raise warning 'hello world'; return null; end; $$;
> >   create trigger my_trigger after insert or update or delete on my_view
> >     for each statement execute procedure my_trigger_function();
> >   insert into my_view values (42);
> > 
> > ... and the world remains ungreeted.
> > 
> > As for transition tables, there are probably meaningful ways to
> > support those for both views and foreign tables at least in certain
> > cases, as future feature enhancements.  For now, do you agree that we
> > should reject such triggers as unsupported?  See attached.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Kevin,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 
> [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

  9. Re: Transition tables for triggers on foreign tables and views

    Kevin Grittner <kgrittn@gmail.com> — 2017-05-02T23:06:47Z

    On Fri, Apr 28, 2017 at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> They will fire if you have an INSTEAD OF row-level trigger; the existence
> of that trigger is what determines whether we implement DML on a view
> through the view's own triggers or through translation to an action on
> the underlying table.
>
> I do not think it'd be reasonable to throw an error for creation of
> a statement-level view trigger when there's no row-level trigger,
> because that just imposes a hard-to-deal-with DDL ordering dependency.
>
> You could make a case for having the updatable-view translation code
> print a WARNING if it notices that there are statement-level triggers
> that cannot be fired due to the translation.

Oh, I see -- you can add all the AFTER ... FOR EACH STATEMENT
triggers you want for an updatable view and they will quietly sit
there without firing no matter how many statements perform the
supposedly triggering action, but as soon as you add a INSTEAD OF
... FOR EACH ROW trigger they spring to life.  On the face of it that
seems to me to violate the POLA, but I kinda see how it evolved.

I need to look at this and the rather similar odd behavior under
inheritance.  I hope to post something Friday.

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/

  10. Re: Transition tables for triggers on foreign tables and views

    Noah Misch <noah@leadboat.com> — 2017-05-06T18:58:35Z

    On Tue, May 02, 2017 at 06:06:47PM -0500, Kevin Grittner wrote:
> On Fri, Apr 28, 2017 at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > They will fire if you have an INSTEAD OF row-level trigger; the existence
> > of that trigger is what determines whether we implement DML on a view
> > through the view's own triggers or through translation to an action on
> > the underlying table.
> >
> > I do not think it'd be reasonable to throw an error for creation of
> > a statement-level view trigger when there's no row-level trigger,
> > because that just imposes a hard-to-deal-with DDL ordering dependency.
> >
> > You could make a case for having the updatable-view translation code
> > print a WARNING if it notices that there are statement-level triggers
> > that cannot be fired due to the translation.
> 
> Oh, I see -- you can add all the AFTER ... FOR EACH STATEMENT
> triggers you want for an updatable view and they will quietly sit
> there without firing no matter how many statements perform the
> supposedly triggering action, but as soon as you add a INSTEAD OF
> ... FOR EACH ROW trigger they spring to life.  On the face of it that
> seems to me to violate the POLA, but I kinda see how it evolved.
> 
> I need to look at this and the rather similar odd behavior under
> inheritance.  I hope to post something Friday.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

  11. Re: Transition tables for triggers on foreign tables and views

    Noah Misch <noah@leadboat.com> — 2017-05-08T07:00:20Z

    On Sat, May 06, 2017 at 06:58:35PM +0000, Noah Misch wrote:
> On Tue, May 02, 2017 at 06:06:47PM -0500, Kevin Grittner wrote:
> > On Fri, Apr 28, 2017 at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 
> > > They will fire if you have an INSTEAD OF row-level trigger; the existence
> > > of that trigger is what determines whether we implement DML on a view
> > > through the view's own triggers or through translation to an action on
> > > the underlying table.
> > >
> > > I do not think it'd be reasonable to throw an error for creation of
> > > a statement-level view trigger when there's no row-level trigger,
> > > because that just imposes a hard-to-deal-with DDL ordering dependency.
> > >
> > > You could make a case for having the updatable-view translation code
> > > print a WARNING if it notices that there are statement-level triggers
> > > that cannot be fired due to the translation.
> > 
> > Oh, I see -- you can add all the AFTER ... FOR EACH STATEMENT
> > triggers you want for an updatable view and they will quietly sit
> > there without firing no matter how many statements perform the
> > supposedly triggering action, but as soon as you add a INSTEAD OF
> > ... FOR EACH ROW trigger they spring to life.  On the face of it that
> > seems to me to violate the POLA, but I kinda see how it evolved.
> > 
> > I need to look at this and the rather similar odd behavior under
> > inheritance.  I hope to post something Friday.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
for your status update.  Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately.  If I do not hear from you by
2017-05-09 07:00 UTC, I will transfer this item to release management team
ownership without further notice.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

  12. Re: Transition tables for triggers on foreign tables and views

    Noah Misch <noah@leadboat.com> — 2017-05-10T02:45:05Z

    On Mon, May 08, 2017 at 12:00:20AM -0700, Noah Misch wrote:
> On Sat, May 06, 2017 at 06:58:35PM +0000, Noah Misch wrote:
> > On Tue, May 02, 2017 at 06:06:47PM -0500, Kevin Grittner wrote:
> > > On Fri, Apr 28, 2017 at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > 
> > > > They will fire if you have an INSTEAD OF row-level trigger; the existence
> > > > of that trigger is what determines whether we implement DML on a view
> > > > through the view's own triggers or through translation to an action on
> > > > the underlying table.
> > > >
> > > > I do not think it'd be reasonable to throw an error for creation of
> > > > a statement-level view trigger when there's no row-level trigger,
> > > > because that just imposes a hard-to-deal-with DDL ordering dependency.
> > > >
> > > > You could make a case for having the updatable-view translation code
> > > > print a WARNING if it notices that there are statement-level triggers
> > > > that cannot be fired due to the translation.
> > > 
> > > Oh, I see -- you can add all the AFTER ... FOR EACH STATEMENT
> > > triggers you want for an updatable view and they will quietly sit
> > > there without firing no matter how many statements perform the
> > > supposedly triggering action, but as soon as you add a INSTEAD OF
> > > ... FOR EACH ROW trigger they spring to life.  On the face of it that
> > > seems to me to violate the POLA, but I kinda see how it evolved.
> > > 
> > > I need to look at this and the rather similar odd behavior under
> > > inheritance.  I hope to post something Friday.
> > 
> > This PostgreSQL 10 open item is past due for your status update.  Kindly send
> > a status update within 24 hours, and include a date for your subsequent status
> > update.  Refer to the policy on open item ownership:
> > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
> for your status update.  Please reacquaint yourself with the policy on open
> item ownership[1] and then reply immediately.  If I do not hear from you by
> 2017-05-09 07:00 UTC, I will transfer this item to release management team
> ownership without further notice.
> 
> [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

This PostgreSQL 10 open item now needs a permanent owner.  Would any other
committer like to take ownership?  If this role interests you, please read
this thread and the policy linked above, then send an initial status update
bearing a date for your subsequent status update.  If the item does not have a
permanent owner by 2017-05-13 03:00 UTC, I will resolve the item by reverting
transition table patches.

Thanks,
nm

  13. Re: Transition tables for triggers on foreign tables and views

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-05-10T03:24:23Z

    Hi,

Since the last one conflicted with recent changes, here's a rebased
patch to prevent transition tables on views and foreign tables.

-- 
Thomas Munro
http://www.enterprisedb.com

  14. Re: Transition tables for triggers on foreign tables and views

    Robert Haas <robertmhaas@gmail.com> — 2017-05-10T03:35:42Z

    On Tue, May 9, 2017 at 11:24 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Since the last one conflicted with recent changes, here's a rebased
> patch to prevent transition tables on views and foreign tables.

Committed, but I made the new error details more like the existing one
that also checks relkind.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company