Re: WIP: Triggers on VIEWs

Here is a WIP patch implementing triggers on VIEWs, as outlined in the
proof-of-concept here:
http://archives.postgresql.org/pgsql-hackers/2010-08/msg00160.php

The new triggers allowed on a VIEW are:
1). Statement-level BEFORE INSERT/UPDATE/DELETE
2). Row-level INSTEAD OF INSERT/UPDATE/DELETE
3). Statement-level AFTER INSERT/UPDATE/DELETE

The new INSTEAD OF trigger type may only be used with VIEWs, and may
only be row-level. It does not support the WHEN or FOR UPDATE OF
column_list options.

There are still a number of things left todo:
- extend ALTER VIEW with enable/disable trigger commands
- much more testing
- documentation

and then there's the question of what to do about the concurrency
issues raised by Marko Tiikkaja. Currently it works like Oracle (i.e.,
no locking).

Regards,
Dean

On 15 August 2010 18:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> There are still a number of things left todo:
>  - extend ALTER VIEW with enable/disable trigger commands

On further reflection, I wonder if the ability to disable VIEW
triggers is needed/wanted at all. I just noticed that while it is
possible to disable a RULE on a TABLE, it is not possible to do so on
VIEW. This certainly makes sense for the _RETURN rule, although
possibly some people might have a use for disabling other rules on
views. The situation with triggers is similar - disabling an INSTEAD
OF trigger would be pointless, and could only lead to errors when
updating the view. Some people may have a use case for disabling
BEFORE and AFTER statement triggers on views, but I suspect that the
number of such cases is small, and I'm tempted to omit this, for now
at least.

Thoughts?

- Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 15 August 2010 18:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> There are still a number of things left todo:
>> - extend ALTER VIEW with enable/disable trigger commands

> On further reflection, I wonder if the ability to disable VIEW
> triggers is needed/wanted at all.

AFAIK the only real use for disabling triggers is in connection with
trigger-based replication systems. A view wouldn't be carrying
replication-related triggers anyway, so I think this could certainly
be left out of a first cut.

You aren't saying that you can see some reason why this couldn't be
added later, if wanted, correct?

regards, tom lane

On 16 August 2010 18:50, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> On 15 August 2010 18:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>> There are still a number of things left todo:
>>>  - extend ALTER VIEW with enable/disable trigger commands
>
>> On further reflection, I wonder if the ability to disable VIEW
>> triggers is needed/wanted at all.
>
> AFAIK the only real use for disabling triggers is in connection with
> trigger-based replication systems.  A view wouldn't be carrying
> replication-related triggers anyway, so I think this could certainly
> be left out of a first cut.
>
> You aren't saying that you can see some reason why this couldn't be
> added later, if wanted, correct?
>

Yes. It should be easy to add later if wanted. I just don't see much
use for it, and I don't want to add more to an already quite big
patch, if it's not really needed.

Regards,
Dean

On 15 August 2010 18:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Here is a WIP patch implementing triggers on VIEWs ... <snip>
>
> There are still a number of things left todo:
>  - extend ALTER VIEW with enable/disable trigger commands
>  - much more testing
>  - documentation
>

Attached is an updated patch with more tests and docs, and a few minor
code tidy ups. I think that the INSTEAD OF triggers part of the patch
is compliant with Feature T213 of the SQL 2008 standard. As discussed,
I don't plan to add the syntax to allow triggers on views to be
disabled at this time, but that should be easy to implement, if there
is a use case for it.

Comments welcome.

Regards,
Dean

On Sep 5, 2010, at 3:09 AM, Dean Rasheed wrote:

> On 15 August 2010 18:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> Here is a WIP patch implementing triggers on VIEWs ... <snip>
>>
>> There are still a number of things left todo:
>> - extend ALTER VIEW with enable/disable trigger commands
>> - much more testing
>> - documentation
>>
>
> Attached is an updated patch with more tests and docs, and a few minor

At least for me, there are some portions of the docs which could use some formatting changes in order to not be confusing grammatically; e.g., this was confusing to me on the first read:

- trigger name. In the case of before triggers, the
+ trigger name. In the case of before and instead of triggers, the

I realize this lack of formatting was inherited from the existing docs, but this would make more sense to me if this (and presumably the other related instances of "before" and "after") were set apart with <literal></> or similar. This is already in use in some places in this patch, so seems like the correct markup.

Regards,

David
--
David Christensen
End Point Corporation
david(at)endpoint(dot)com

On 7 September 2010 02:03, David Christensen <david(at)endpoint(dot)com> wrote:
>
> On Sep 5, 2010, at 3:09 AM, Dean Rasheed wrote:
>
>> On 15 August 2010 18:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>> Here is a WIP patch implementing triggers on VIEWs ... <snip>
>>>
>>> There are still a number of things left todo:
>>>  - extend ALTER VIEW with enable/disable trigger commands
>>>  - much more testing
>>>  - documentation
>>>
>>
>> Attached is an updated patch with more tests and docs, and a few minor
>
>
> At least for me, there are some portions of the docs which could use some formatting changes in order to not be confusing grammatically; e.g., this was confusing to me on the first read:
>
> -    trigger name.  In the case of before triggers, the
> +    trigger name.  In the case of before and instead of triggers, the
>
> I realize this lack of formatting was inherited from the existing docs, but this would make more sense to me if this (and presumably the other related instances of "before" and "after") were set apart with <literal></> or similar.  This is already in use in some places in this patch, so seems like the correct markup.
>

Yeah, I think you're right. That chapter is the first place in the
manual where the concepts of before/after/instead of are introduced.
The first time it mentions them it uses <firstterm>, but then it
doesn't use any markup for them for the remainder of the chapter. It
looks like the rest of the manual uses <literal>+uppercase wherever
they're mentioned, which does help readability, so it would make sense
to bring the rest of that chapter into line with that convention.

Thanks for looking at this.

Cheers,
Dean

On 7 September 2010 08:43, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 7 September 2010 02:03, David Christensen <david(at)endpoint(dot)com> wrote:
>>
>> On Sep 5, 2010, at 3:09 AM, Dean Rasheed wrote:
>>
>>> On 15 August 2010 18:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>>> Here is a WIP patch implementing triggers on VIEWs ... <snip>
>>>>
>>>> There are still a number of things left todo:
>>>>  - extend ALTER VIEW with enable/disable trigger commands
>>>>  - much more testing
>>>>  - documentation
>>>>
>>>
>>> Attached is an updated patch with more tests and docs, and a few minor
>>
>>
>> At least for me, there are some portions of the docs which could use some formatting changes in order to not be confusing grammatically; e.g., this was confusing to me on the first read:
>>
>> -    trigger name.  In the case of before triggers, the
>> +    trigger name.  In the case of before and instead of triggers, the
>>
>> I realize this lack of formatting was inherited from the existing docs, but this would make more sense to me if this (and presumably the other related instances of "before" and "after") were set apart with <literal></> or similar.  This is already in use in some places in this patch, so seems like the correct markup.
>>
>
> Yeah, I think you're right. That chapter is the first place in the
> manual where the concepts of before/after/instead of are introduced.
> The first time it mentions them it uses <firstterm>, but then it
> doesn't use any markup for them for the remainder of the chapter. It
> looks like the rest of the manual uses <literal>+uppercase wherever
> they're mentioned, which does help readability, so it would make sense
> to bring the rest of that chapter into line with that convention.
>
> Thanks for looking at this.
>
> Cheers,
> Dean
>

Here's an updated version with improved formatting and a few minor
wording changes to the triggers chapter.

Regards,
Dean

--On 5. September 2010 09:09:55 +0100 Dean Rasheed
<dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

I had a first look on your patch, great work!

> Attached is an updated patch with more tests and docs, and a few minor
> code tidy ups. I think that the INSTEAD OF triggers part of the patch
> is compliant with Feature T213 of the SQL 2008 standard. As discussed,

Reading the past discussions, there was some mention about the RETURNING
clause.
I see Oracle doesn't allow its RETURNING INTO clause with INSTEAD OF
triggers (at least my 10g XE instance here doesn't allow it, not sure about
newer versions). I assume the following example might have some surprising
effects on users:

CREATE TABLE foo(id integer);
CREATE VIEW vfoo AS SELECT 'bernd', * FROM foo;

CREATE OR REPLACE FUNCTION insert_foo() RETURNS trigger
LANGUAGE plpgsql
AS $$
BEGIN INSERT INTO foo VALUES(NEW.id);
RETURN NEW;
END; $$;

CREATE TRIGGER insert_vfoo INSTEAD OF INSERT ON vfoo
FOR EACH ROW EXECUTE PROCEDURE insert_foo();

INSERT INTO vfoo VALUES('helmle', 2) RETURNING *;
text | id
--------+----
helmle | 2
(1 row)

SELECT * FROM vfoo;
text | id
-------+----
bernd | 2
(1 row)

This is solvable by a properly designed trigger function, but maybe we need
to do something about this?

> I don't plan to add the syntax to allow triggers on views to be
> disabled at this time, but that should be easy to implement, if there
> is a use case for it.

I really don't see a need for this at the moment. We don't have DISABLE
RULE either. I'm going to post some additional comments once i've
understand all things.

--
Thanks

Bernd

On 2010-09-23 1:16 AM, Bernd Helmle wrote:
> INSERT INTO vfoo VALUES('helmle', 2) RETURNING *;
> text | id
> --------+----
> helmle | 2
> (1 row)
>
> SELECT * FROM vfoo;
> text | id
> -------+----
> bernd | 2
> (1 row)
>
> This is solvable by a properly designed trigger function, but maybe we need
> to do something about this?

I really don't think we should limit what people are allowed to do in
the trigger function.

Besides, even if the RETURNING clause returned 'bernd' in the above
case, I think it would be even *more* surprising. The trigger function
explicitly returns NEW which has 'helmle' as the first field.

Regards,
Marko Tiikkaja

On 23 September 2010 00:26, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> On 2010-09-23 1:16 AM, Bernd Helmle wrote:
>>
>> INSERT INTO vfoo VALUES('helmle', 2) RETURNING *;
>>   text  | id
>> --------+----
>>  helmle |  2
>> (1 row)
>>
>> SELECT * FROM vfoo;
>>  text  | id
>> -------+----
>>  bernd |  2
>> (1 row)
>>
>> This is solvable by a properly designed trigger function, but maybe we
>> need
>> to do something about this?
>
> I really don't think we should limit what people are allowed to do in the
> trigger function.
>
> Besides, even if the RETURNING clause returned 'bernd' in the above case, I
> think it would be even *more* surprising.  The trigger function explicitly
> returns NEW which has 'helmle' as the first field.
>

Yes, I agree. To me this is the least surprising behaviour. I think a
more common case would be where the trigger computed a value (such as
the 'last updated' example). The executor doesn't have any kind of a
handle on the row inserted by the trigger, so it has to rely on the
function return value to support RETURNING.

I can confirm the latest Oracle (11g R2 Enterprise Edition) does not
support RETURNING INTO with INSTEAD OF triggers (although it does work
with its auto-updatable views), presumably because it's triggers don't
return values, but I think it would be a shame for us to not support
it.

Regards,
Dean

--On 23. September 2010 08:59:32 +0100 Dean Rasheed
<dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> Yes, I agree. To me this is the least surprising behaviour. I think a
> more common case would be where the trigger computed a value (such as
> the 'last updated' example). The executor doesn't have any kind of a
> handle on the row inserted by the trigger, so it has to rely on the
> function return value to support RETURNING.

I didn't mean to forbid it altogether, but at least to document
explicitely, that the trigger returns a VIEW's NEW tuple, not the one of
the base table (and may modify it). But you've already adressed this in
your doc patches, so nothing to worry about further.

--
Thanks

Bernd

--On 8. September 2010 09:00:33 +0100 Dean Rasheed
<dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> Here's an updated version with improved formatting and a few minor
> wording changes to the triggers chapter.

This version doesn't apply clean anymore due to some rejects in
plainmain.c. Corrected version attached.

--
Thanks

Bernd

On 29 September 2010 20:18, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 8. September 2010 09:00:33 +0100 Dean Rasheed
> <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
>> Here's an updated version with improved formatting and a few minor
>> wording changes to the triggers chapter.
>
> This version doesn't apply clean anymore due to some rejects in plainmain.c.
> Corrected version attached.
>

Ah yes, those pesky bits have been rotting while I wasn't looking.
Thanks for fixing them!

Regards,
Dean

Bernd Helmle <mailings(at)oopsware(dot)de> writes:
> --On 8. September 2010 09:00:33 +0100 Dean Rasheed
> <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> Here's an updated version with improved formatting and a few minor
>> wording changes to the triggers chapter.

> This version doesn't apply clean anymore due to some rejects in
> plainmain.c. Corrected version attached.

Applied with revisions. A couple of points worth remarking:

* I took out this change in planmain.c:

+ /*
+ * If the query target is a VIEW, it won't be in the jointree, but we
+ * need a dummy RelOptInfo node for it. This need not have any stats in
+ * it because it always just goes at the top of the plan tree.
+ */
+ if (parse->resultRelation &&
+ root->simple_rel_array[parse->resultRelation] == NULL)
+ build_simple_rel(root, parse->resultRelation, RELOPT_OTHER_MEMBER_REL);

AFAICT that's just dead code: the only reason to build such a rel would
be if there were Vars referencing it in the main part of the plan tree.
But there aren't. Perhaps this was left over from some early iteration
of the patch before you had the Var numbering done right? Do you know
of any cases where it's still needed?

* I also took out the changes in preprocess_targetlist() that tried to
prevent equivalent wholerow vars from getting entered in the targetlist.
This would not work as intended since the executor has specific
expectations for the names of those junk TLEs; it'd fail if it ever
actually tried to do an EvalPlanQual recheck that needed those TLEs.
Now I believe that an EPQ recheck is impossible so far as the update or
delete itself is concerned, when the target is a view. So if you were
really concerned about the extra vars, the non-kluge route to a solution
would be to avoid generating RowMarks in the first place. You'd have to
think a bit about the possibility of SELECT FOR UPDATE in sub-selects
though; the query as a whole might need some rowmarks even if the top
level Modify node doesn't. On the whole I couldn't get excited about
this issue, so I just left it alone.

regards, tom lane

On 10 October 2010 19:06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Applied with revisions.

Brilliant! Thank you very much.

> * I took out this change in planmain.c:
>
> +       /*
> +        * If the query target is a VIEW, it won't be in the jointree, but we
> +        * need a dummy RelOptInfo node for it. This need not have any stats in
> +        * it because it always just goes at the top of the plan tree.
> +        */
> +       if (parse->resultRelation &&
> +               root->simple_rel_array[parse->resultRelation] == NULL)
> +               build_simple_rel(root, parse->resultRelation, RELOPT_OTHER_MEMBER_REL);
>
> AFAICT that's just dead code: the only reason to build such a rel would
> be if there were Vars referencing it in the main part of the plan tree.
> But there aren't.  Perhaps this was left over from some early iteration
> of the patch before you had the Var numbering done right?  Do you know
> of any cases where it's still needed?

No, I think you're right. It was just the leftovers of an early
attempt to get the rewriter changes right.

> * I also took out the changes in preprocess_targetlist() that tried to
> prevent equivalent wholerow vars from getting entered in the targetlist.
> This would not work as intended since the executor has specific
> expectations for the names of those junk TLEs; it'd fail if it ever
> actually tried to do an EvalPlanQual recheck that needed those TLEs.

Ah yes, I missed that. I still don't see where it uses those TLEs by
name though. It looks as though it's using wholeAttNo, so perhaps my
code would have worked if I had set wholeAttNo on the RowMark? Anyway,
I don't think it's likely that this extra Var is going to be present
often in practice, so I don't think it's a problem worth worrying
about.

Thanks very much for looking at this.

Regards,
Dean

> Now I believe that an EPQ recheck is impossible so far as the update or
> delete itself is concerned, when the target is a view.  So if you were
> really concerned about the extra vars, the non-kluge route to a solution
> would be to avoid generating RowMarks in the first place.  You'd have to
> think a bit about the possibility of SELECT FOR UPDATE in sub-selects
> though; the query as a whole might need some rowmarks even if the top
> level Modify node doesn't.  On the whole I couldn't get excited about
> this issue, so I just left it alone.
>
>                        regards, tom lane
>