From: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
---|---|
To: | Stephen Frost <sfrost(at)snowman(dot)net> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: WITH CHECK OPTION for auto-updatable views |
Date: | 2013-06-24 13:39:06 |
Message-ID: | CAEZATCVEU2sRrtN__mfN+93pgEPL4XH-+wVeqcvVpwGKvDTq_g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
with-check-option.patch | application/octet-stream | 83.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Albe Laurenz | 2013-06-24 13:41:51 | Re: Possible bug in CASE evaluation |
Previous Message | Andres Freund | 2013-06-24 12:47:12 | Re: Add more regression tests for dbcommands |