Re: WITH CHECK OPTION for auto-updatable views

Here's a rebased version of the patch implementing WITH CHECK OPTION
for auto-updatable views.

It now includes documentation, and a clearer description of the
patch's limitations --- WITH CHECK OPTION is only supported on
auto-updatable views, not trigger-updatable or rule-updatable views. I
believe that's compatible with the following features from the SQL
standard:

F311-04 Schema definition statement CREATE VIEW: WITH CHECK OPTION
F751 View CHECK enhancements

Regards,
Dean

On 9 June 2013 11:14, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Here's a rebased version of the patch implementing WITH CHECK OPTION
> for auto-updatable views.
>
> It now includes documentation, and a clearer description of the
> patch's limitations --- WITH CHECK OPTION is only supported on
> auto-updatable views, not trigger-updatable or rule-updatable views. I
> believe that's compatible with the following features from the SQL
> standard:
>
> F311-04 Schema definition statement CREATE VIEW: WITH CHECK OPTION
> F751 View CHECK enhancements
>

Here's an updated version --- I missed the necessary update to the
check_option column of information_schema.views.

Regards,
Dean

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> Here's an updated version --- I missed the necessary update to the
> check_option column of information_schema.views.

Thanks! This is really looking quite good, but it's a bit late and I'm
going on vacation tomorrow, so I didn't quite want to commit it yet. :)
Instead, here are a few things that I'd like to see fixed up:

I could word-smith the docs all day, most likely, but at least the
following would be nice to have cleaned up:

- 'This is parameter may be either'

- I don't like "This allows an existing view's ...". The option can be
used on CREATE VIEW as well as ALTER VIEW. I'd say something like:

This parameter may be either <literal>local</> or
<literal>cascaded</>, and is equivalent to specifying <literal>WITH [
CASCADED | LOCAL ] CHECK OPTION</> (see below). This option can be
changed on existing views using <xref linkend="sql-alterview">.

- wrt what shows up in '\h create view' and '\h alter view', I think we
should go ahead and add in with the options are, ala EXPLAIN. That
avoids having to guess at it (I was trying 'with_check_option'
initially :).

- Supposedly, this option isn't available for RECURSIVE views, but it's
happily accepted:

=*# create recursive view qq (a) with (check_option = local) as select z from q;
CREATE VIEW

(same is true of ALTER VIEW on a RECURSIVE view)

- pg_dump support is there, but it outputs the definition using the PG
syntax instead of the SQL syntax; is there any particular reason for
this..? imv, we should be dumping SQL spec where we can trivially
do so.

- Why check_option_offset instead of simply check_option..? We don't
have security_barrier_offset and it seems like we should be
consistent there.

The rest looks pretty good to me. If you can fix the above, I'll review
again and would be happy to commit it. :)

Thanks!

Stephen

On 22 June 2013 07:24, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Dean,
>
> * Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
>> Here's an updated version --- I missed the necessary update to the
>> check_option column of information_schema.views.
>
> Thanks! This is really looking quite good, but it's a bit late and I'm
> going on vacation tomorrow, so I didn't quite want to commit it yet. :)

Thanks for looking at this!

> Instead, here are a few things that I'd like to see fixed up:
>
> I could word-smith the docs all day, most likely, but at least the
> following would be nice to have cleaned up:
>
> - 'This is parameter may be either'
>

Fixed.

> - I don't like "This allows an existing view's ...". The option can be
> used on CREATE VIEW as well as ALTER VIEW. I'd say something like:
>
> This parameter may be either <literal>local</> or
> <literal>cascaded</>, and is equivalent to specifying <literal>WITH [
> CASCADED | LOCAL ] CHECK OPTION</> (see below). This option can be
> changed on existing views using <xref linkend="sql-alterview">.
>

Yes, that sounds clearer. Done.

> - wrt what shows up in '\h create view' and '\h alter view', I think we
> should go ahead and add in with the options are, ala EXPLAIN. That
> avoids having to guess at it (I was trying 'with_check_option'
> initially :).
>

Done.

> - Supposedly, this option isn't available for RECURSIVE views, but it's
> happily accepted:
>
> =*# create recursive view qq (a) with (check_option = local) as select z from q;
> CREATE VIEW
>
> (same is true of ALTER VIEW on a RECURSIVE view)
>

Recursive views are just a special case of non-auto-updatable views
--- they don't support DML without triggers or rules, so they don't
support the check option. I've added checks to CREATE/ALTER VIEW to
prevent the check_option from being added to non-auto-updatable views,
which covers the recursive view case above.

> - pg_dump support is there, but it outputs the definition using the PG
> syntax instead of the SQL syntax; is there any particular reason for
> this..? imv, we should be dumping SQL spec where we can trivially
> do so.
>

The code's not pretty, but done.

> - Why check_option_offset instead of simply check_option..? We don't
> have security_barrier_offset and it seems like we should be
> consistent there.
>

It's because it's a string-valued option, with space allocated
separately, so it's the offset to the actual option text. This is
consistent with bufferingModeOffset in GiSTOptions.

> The rest looks pretty good to me. If you can fix the above, I'll review
> again and would be happy to commit it. :)
>
> Thanks!
>
> Stephen

Thanks.

Regards,
Dean

On 24 June 2013 14:39, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 22 June 2013 07:24, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Dean,
>>
>> * Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
>>> Here's an updated version --- I missed the necessary update to the
>>> check_option column of information_schema.views.
>>
>> Thanks! This is really looking quite good, but it's a bit late and I'm
>> going on vacation tomorrow, so I didn't quite want to commit it yet. :)
>
> Thanks for looking at this!
>
>
>> Instead, here are a few things that I'd like to see fixed up:
>>
>> I could word-smith the docs all day, most likely, but at least the
>> following would be nice to have cleaned up:
>>
>> - 'This is parameter may be either'
>>
>
> Fixed.
>
>
>> - I don't like "This allows an existing view's ...". The option can be
>> used on CREATE VIEW as well as ALTER VIEW. I'd say something like:
>>
>> This parameter may be either <literal>local</> or
>> <literal>cascaded</>, and is equivalent to specifying <literal>WITH [
>> CASCADED | LOCAL ] CHECK OPTION</> (see below). This option can be
>> changed on existing views using <xref linkend="sql-alterview">.
>>
>
> Yes, that sounds clearer. Done.
>
>
>> - wrt what shows up in '\h create view' and '\h alter view', I think we
>> should go ahead and add in with the options are, ala EXPLAIN. That
>> avoids having to guess at it (I was trying 'with_check_option'
>> initially :).
>>
>
> Done.
>
>
>> - Supposedly, this option isn't available for RECURSIVE views, but it's
>> happily accepted:
>>
>> =*# create recursive view qq (a) with (check_option = local) as select z from q;
>> CREATE VIEW
>>
>> (same is true of ALTER VIEW on a RECURSIVE view)
>>
>
> Recursive views are just a special case of non-auto-updatable views
> --- they don't support DML without triggers or rules, so they don't
> support the check option. I've added checks to CREATE/ALTER VIEW to
> prevent the check_option from being added to non-auto-updatable views,
> which covers the recursive view case above.
>
>
>> - pg_dump support is there, but it outputs the definition using the PG
>> syntax instead of the SQL syntax; is there any particular reason for
>> this..? imv, we should be dumping SQL spec where we can trivially
>> do so.
>>
>
> The code's not pretty, but done.
>
>
>> - Why check_option_offset instead of simply check_option..? We don't
>> have security_barrier_offset and it seems like we should be
>> consistent there.
>>
>
> It's because it's a string-valued option, with space allocated
> separately, so it's the offset to the actual option text. This is
> consistent with bufferingModeOffset in GiSTOptions.
>
>
>> The rest looks pretty good to me. If you can fix the above, I'll review
>> again and would be happy to commit it. :)
>>

Here's a rebased version that applies cleanly to git master.

Regards,
Dean

Hello

I try to check this patch

I have a problem with initdb after patching

error

initializing dependencies ... ok
creating system views ... FATAL: WITH CHECK OPTION is supported only
on auto-updatable views
STATEMENT: /*

I found missing initialization (strange, gcc doesn't raise warnings :( )

+ bool check_option;
+ bool security_barrier;

Regards

Pavel

2013/7/3 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 24 June 2013 14:39, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 22 June 2013 07:24, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>>> Dean,
>>>
>>> * Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
>>>> Here's an updated version --- I missed the necessary update to the
>>>> check_option column of information_schema.views.
>>>
>>> Thanks! This is really looking quite good, but it's a bit late and I'm
>>> going on vacation tomorrow, so I didn't quite want to commit it yet. :)
>>
>> Thanks for looking at this!
>>
>>
>>> Instead, here are a few things that I'd like to see fixed up:
>>>
>>> I could word-smith the docs all day, most likely, but at least the
>>> following would be nice to have cleaned up:
>>>
>>> - 'This is parameter may be either'
>>>
>>
>> Fixed.
>>
>>
>>> - I don't like "This allows an existing view's ...". The option can be
>>> used on CREATE VIEW as well as ALTER VIEW. I'd say something like:
>>>
>>> This parameter may be either <literal>local</> or
>>> <literal>cascaded</>, and is equivalent to specifying <literal>WITH [
>>> CASCADED | LOCAL ] CHECK OPTION</> (see below). This option can be
>>> changed on existing views using <xref linkend="sql-alterview">.
>>>
>>
>> Yes, that sounds clearer. Done.
>>
>>
>>> - wrt what shows up in '\h create view' and '\h alter view', I think we
>>> should go ahead and add in with the options are, ala EXPLAIN. That
>>> avoids having to guess at it (I was trying 'with_check_option'
>>> initially :).
>>>
>>
>> Done.
>>
>>
>>> - Supposedly, this option isn't available for RECURSIVE views, but it's
>>> happily accepted:
>>>
>>> =*# create recursive view qq (a) with (check_option = local) as select z from q;
>>> CREATE VIEW
>>>
>>> (same is true of ALTER VIEW on a RECURSIVE view)
>>>
>>
>> Recursive views are just a special case of non-auto-updatable views
>> --- they don't support DML without triggers or rules, so they don't
>> support the check option. I've added checks to CREATE/ALTER VIEW to
>> prevent the check_option from being added to non-auto-updatable views,
>> which covers the recursive view case above.
>>
>>
>>> - pg_dump support is there, but it outputs the definition using the PG
>>> syntax instead of the SQL syntax; is there any particular reason for
>>> this..? imv, we should be dumping SQL spec where we can trivially
>>> do so.
>>>
>>
>> The code's not pretty, but done.
>>
>>
>>> - Why check_option_offset instead of simply check_option..? We don't
>>> have security_barrier_offset and it seems like we should be
>>> consistent there.
>>>
>>
>> It's because it's a string-valued option, with space allocated
>> separately, so it's the offset to the actual option text. This is
>> consistent with bufferingModeOffset in GiSTOptions.
>>
>>
>>> The rest looks pretty good to me. If you can fix the above, I'll review
>>> again and would be happy to commit it. :)
>>>
>
> Here's a rebased version that applies cleanly to git master.
>
> Regards,
> Dean
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Hello

just some notes:

* autocomplete for INSERT, UPDATE, DELETE should to show updatable views too

* can you explain better in doc differences between WITH CASCADED or
WITH LOCAL OPTION - assign some simple example to doc, please

* is possible to better identify (describe) failed constraints?

postgres=# create view v1 as select * from bubu where a > 0;
CREATE VIEW
postgres=# create view v2 as select * from v1 where a < 10 with check option;
CREATE VIEW
postgres=# insert into v1 values(-10);
INSERT 0 1
postgres=# insert into v2 values(-10);
ERROR: new row violates WITH CHECK OPTION for view "v2" --- but this
constraint is related to v1
DETAIL: Failing row contains (-10).

* I found a difference against MySQL - LOCAL option ignore all other constraints

postgres=# CREATE TABLE t1 (a INT);
CREATE TABLE
postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a < 2 WITH CHECK OPTION;
CREATE VIEW
postgres=# CREATE VIEW v2 AS SELECT * FROM v1 WHERE a > 0 WITH LOCAL
CHECK OPTION;
CREATE VIEW
postgres=# INSERT INTO v2 VALUES (2);
ERROR: new row violates WITH CHECK OPTION for view "v1" -- it will be
ok on MySQL
DETAIL: Failing row contains (2).

Probably MySQL is wrong (due differet behave than in DB2) -- but who
know http://bugs.mysql.com/bug.php?id=6404

What is a correct behave?

Regards

Pavel

On 5 July 2013 07:02, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> I try to check this patch
>
> I have a problem with initdb after patching
>
> error
>
> initializing dependencies ... ok
> creating system views ... FATAL: WITH CHECK OPTION is supported only
> on auto-updatable views
> STATEMENT: /*
>
>
> I found missing initialization (strange, gcc doesn't raise warnings :( )
>
> + bool check_option;
> + bool security_barrier;
>

Ah, good catch. I was being careless there.

It turns out that although I compile with -Wall which implies
-Wuninitialized and -Wmaybe-uninitialized, those warnings are only
supported in optimised builds, which is why I didn't see it. So I've
learned something new today: always do an optimised build as well
during development.

Will fix. Thanks.

Regards,
Dean

On 5 July 2013 08:22, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> just some notes:
>
> * autocomplete for INSERT, UPDATE, DELETE should to show updatable views too
>

I think that is the subject for a separate patch. It was discussed
previously and Tom suggested that tab-completion should just complete
with all views regardless of whether they are updatable or not,
because the cost of calling pg_relation_is_updatable() for all views
in a database would be too high, because it would require opening them
all to do the check.

> * can you explain better in doc differences between WITH CASCADED or
> WITH LOCAL OPTION - assign some simple example to doc, please
>

OK, I've added another couple of examples to illustrate the difference.

> * is possible to better identify (describe) failed constraints?
>
> postgres=# create view v1 as select * from bubu where a > 0;
> CREATE VIEW
> postgres=# create view v2 as select * from v1 where a < 10 with check option;
> CREATE VIEW
> postgres=# insert into v1 values(-10);
> INSERT 0 1
> postgres=# insert into v2 values(-10);
> ERROR: new row violates WITH CHECK OPTION for view "v2" --- but this
> constraint is related to v1
> DETAIL: Failing row contains (-10).
>

Hmm, I was originally checking this as a single constraint "a > 0 AND
a < 10" attached to v2, which seemed logical to me, since that's where
the constraint is specified. On closer reading of the SQL spec,
however, it appears that all constraints from inner views are meant to
be checked before the constraints from outer views, which does indeed
then allow for reporting the error corresponding to whichever view's
conditions were violated.

So I've updated the patch to do as you suggest (which makes the code
is little simpler too), and the above test now reports that v1's qual
was violated.

> * I found a difference against MySQL - LOCAL option ignore all other constraints
>
> postgres=# CREATE TABLE t1 (a INT);
> CREATE TABLE
> postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a < 2 WITH CHECK OPTION;
> CREATE VIEW
> postgres=# CREATE VIEW v2 AS SELECT * FROM v1 WHERE a > 0 WITH LOCAL
> CHECK OPTION;
> CREATE VIEW
> postgres=# INSERT INTO v2 VALUES (2);
> ERROR: new row violates WITH CHECK OPTION for view "v1" -- it will be
> ok on MySQL
> DETAIL: Failing row contains (2).
>
> Probably MySQL is wrong (due differet behave than in DB2) -- but who
> know http://bugs.mysql.com/bug.php?id=6404
>
> What is a correct behave?
>

I think MySQL is wrong here.

The SQL spec is very specific about how these constraints should be
checked (see "Effect of inserting a table into a viewed table"). The
required behaviour is defined recursively such that if a view has a
LOCAL check option, any separately defined checks on lower level views
are checked first, and then any conditions specified locally on the
view are checked. A LOCAL check shouldn't prevent lower level checks
from running.

Thanks for the review. Updated patch attached.

Regards,
Dean

Dean,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> Thanks! This is really looking quite good, but it's a bit late and I'm
> going on vacation tomorrow, so I didn't quite want to commit it yet. :)

Apologies on this taking a bit longer than I expected, but it's been
committed and pushed now. Please take a look and let me know of any
issues you see with the changes that I made.

Thanks!

Stephen

On 18 July 2013 22:27, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Dean,
>
>
> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
>> Thanks! This is really looking quite good, but it's a bit late and I'm
>> going on vacation tomorrow, so I didn't quite want to commit it yet. :)
>
> Apologies on this taking a bit longer than I expected, but it's been
> committed and pushed now. Please take a look and let me know of any
> issues you see with the changes that I made.
>

Excellent. Thank you! The changes look good to me.

Cheers,
Dean