Re: WITH CHECK OPTION for auto-updatable views

On 24 June 2013 14:39, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 22 June 2013 07:24, Stephen Frost <sfrost@snowman.net> wrote:
>> Dean,
>>
>> * Dean Rasheed (dean.a.rasheed@gmail.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