On Tue, 30 Jan 2018 19:21:04 +1300
Thomas Munro <thomas.munro@enterprisedb.com> wrote:
> On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> >>> You need to DROP VIEW lock_view4 and lock_view5 in the regression
> >>> test as well.
> >>
> >> Thank you for reviewing the patch.
> >>
> >> I fixed this and attached a updated patch v6.
> >
> > Looks good to me. If there's no objection, especially from Thomas
> > Munro, I will mark this as "ready for committer".
>
> About the idea: it makes some kind of sense to me that we should lock
> the underlying table, in all the same cases that you could do DML on
> the view automatically. I wonder if this is a problem for the
> soundness: "Tables appearing in a subquery are ignored and not
> locked." I can imagine using this for making backwards-compatible
> schema changes, in which case the LOCK-based transaction isolation
> techniques might benefit from this behaviour. I'd be interested to
> hear about the ideal use case you have in mind.
I think the use case is almost similar to that of auto-updatable views.
There are some purpose to use views, for example 1) preventing from
modifying application due to schema changes, 2) protecting the underlying
table from users without proper privileges, 3) making a shorthand of a
query with complex WHERE condition. When these are updatable views and
users need transaction isolation during updating them, I think the lockable
views feature is benefitical because users don't need to refer to the
underlying table. Users might don't know the underlying table, or even
might not have the privilege to lock this.
> About the patch: I didn't study it in detail. It builds, has
> documentation and passes all tests. Would it be a good idea to add an
> isolation test to show that the underlying relation is actually
> locked?
Whether the underlying relation is actually locked or not is confirmed
in the regression test using pg_locks, so I don't believe that we need
to add an isolation test.
> Typo:
>
> + /* Check permissions with the view owner's priviledge. */
>
> s/priviledge/privilege/
>
> Grammar:
>
> +/*
> + * Check whether the view is lockable.
> + *
> + * Currently, only auto-updatable views can be locked, that is,
> + * views whose definition are simple and that doesn't have
> + * instead of rules or triggers are lockable.
>
> s/definition are simple and that doesn't/definition is simple and that don't/
> s/instead of/INSTEAD OF/ ?
Thank you for pointing out these. I attached the fixed patch.
Regards,
> --
> Thomas Munro
> http://www.enterprisedb.com
--
Yugo Nagata <nagata@sraoss.co.jp>