Re: Updatable view columns

Hi,

Attached is a work-in-progress patch to extend auto-updatable views to
support views containing a mix of updatable and non-updatable columns.
This is basically the "columns" part of SQL Feature T111, "Updatable
joins, unions, and columns".

So specifically, views of the following form will now be auto-updatable:

SELECT <column, expression, sub-query or non-SRF>, ...
FROM <single base table or view>
WHERE <arbitrary quals>
[ORDER BY ...]

with the restriction that no window functions, aggregates or
set-returning functions may appear in the view's targetlist (because
otherwise the rewriting process may move them up into quals of the
top-level query where they are not allowed).

Hopefully this will make auto-updatable views much more useful, since
it covers a much wider class of real-world views.

INSERT and UPDATE are supported provided that they do not attempt to
assign to a non-updatable column (which currently means a column that
is not a simple reference to an updatable column of the base
relation). This also means that the view must have at least 1
updatable column for these operations, which is per-spec.

DELETE on the other hand doesn't actually require any updatable
columns, provided the view satisfies all the other updatability
requirements (single base relation, no distinction, grouping, etc...).
This is actually an extension of the spec, which says that DELETE
should only be supported on updatable views with at least 1 updatable
column, but having played around with the code, it seems it would be
an annoying amount of additional code to enforce this restriction, so
I don't see any reason to not just allow it.

Code-wise, aside from the obvious changes needed to the
xxx_is_updatable() functions, the only other code change needed to
support this is in rewriteTargetListIU(). This had code for UPDATE
which would add new dummy targetlist entries for unassigned-to
attributes in the view, similar to expand_targetlist() in preptlist.c.
It now only does this for trigger-updatable views. For auto-updatable
views, it is definitely the wrong thing to do, since it would assign
targetlist entries for all the non-updatable columns. For
rule-updatable views it was not wrong, but I think it was unnecessary,
since the rule will rewrite the query with a different target
relation, which would then get recursively processed. Basically,
adding these dummy targetlist entries is only necessary if the view
relation remains the target after rewriting. That's consistent with
the fact that this code-block was only added in 9.1 to support
triggers on views.

It still needs more testing and doc updates, but otherwise I hope it
will be in reasonable shape for the next commitfest.

Regards,
Dean

On Mon, 2013-08-12 at 15:27 +0100, Dean Rasheed wrote:
> Attached is a work-in-progress patch to extend auto-updatable views to
> support views containing a mix of updatable and non-updatable columns.
> This is basically the "columns" part of SQL Feature T111, "Updatable
> joins, unions, and columns".

This patch needs to be rebased.

On 22 August 2013 01:49, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Mon, 2013-08-12 at 15:27 +0100, Dean Rasheed wrote:
>> Attached is a work-in-progress patch to extend auto-updatable views to
>> support views containing a mix of updatable and non-updatable columns.
>> This is basically the "columns" part of SQL Feature T111, "Updatable
>> joins, unions, and columns".
>
> This patch needs to be rebased.
>

Here's a rebased version, now with updated docs.

Regards,
Dean

Hi Dean,

First of all, thanks for working on this!

The patch compiles and applies fine (though with offsets). The feature
is in the SQL standard, and further improves an existing feature.

Some stuff I've spotted and fixed in the attached patch (hope you don't
mind, thought it'd be easier to just fix these rather than describing
them in email):
- Some typos I've spotted in some of the comments
- Fixed escaping of _ in regression tests

Other than these, the patch looks good to me. Will wait for your
thoughts and an updated patch before marking ready for committer.

Regards,
Marko Tiikkaja

On 16 September 2013 21:14, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> Hi Dean,
>
> First of all, thanks for working on this!
>
> The patch compiles and applies fine (though with offsets). The feature is
> in the SQL standard, and further improves an existing feature.
>
> Some stuff I've spotted and fixed in the attached patch (hope you don't
> mind, thought it'd be easier to just fix these rather than describing them
> in email):
> - Some typos I've spotted in some of the comments
> - Fixed escaping of _ in regression tests
>
> Other than these, the patch looks good to me. Will wait for your thoughts
> and an updated patch before marking ready for committer.
>

Thanks for the review. Those changes all look sensible to me.

Here's an updated patch incorporating all your fixes, and rebased to
apply without offsets.

Regards,
Dean

On 2013-09-17 12:53, Dean Rasheed wrote:
> Thanks for the review. Those changes all look sensible to me.
>
> Here's an updated patch incorporating all your fixes, and rebased to
> apply without offsets.

Looks good to me. Marking this one ready for committer.

Regards,
Marko Tiikkaja

On Tue, Sep 17, 2013 at 6:16 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> On 2013-09-17 12:53, Dean Rasheed wrote:
>>
>> Thanks for the review. Those changes all look sensible to me.
>>
>> Here's an updated patch incorporating all your fixes, and rebased to
>> apply without offsets.
>
>
> Looks good to me. Marking this one ready for committer.

Committed.

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

On 18 October 2013 15:43, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Committed.
>

Excellent. Thank you!
And thank you Marko for your thorough review.

Regards,
Dean