Re: WITH CHECK OPTION for auto-updatable views

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.a.rasheed@gmail.com>:
> 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
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>