Thread

1. Thinking about WITH CHECK OPTION for views

   Dean Rasheed <dean.a.rasheed@gmail.com> — 2013-01-14T09:29:56Z

   I've been thinking about WITH CHECK OPTION for auto-updatable views.
   Given the timing I doubt if this will be ready for 9.3, since I only
   get occasional evenings and weekends to hack on postgres, but I think
   it's probably worth kicking off a discussion, starting with a
   description of what the feature actually is.
   
   From the SQL spec:
   
       <view definition> ::=
           CREATE [ RECURSIVE ] VIEW <table name> <view specification>
           AS <query expression> [ WITH [ <levels clause> ] CHECK OPTION ]
   
       <levels clause> ::= CASCADED | LOCAL
   
       If WITH CHECK OPTION is specified and <levels clause> is not specified,
       then a <levels clause> of CASCADED is implicit.
   
   In other words there are 3 possible levels of check, which are:
   
   1). NO CHECK - i.e., what we do now.
   
   2). LOCAL CHECK - new tuples are validated against the WHERE quals
   specified locally on this view, but not against any quals from any
   underlying views (unless they also specify the WITH CHECK OPTION).
   
   3). CASCADED CHECK - new tuples are validated against the WHERE quals
   of this view and all underlying views (regardless of whether or not
   those underlying views specify the WITH CHECK OPTION).
   
   
   The SQL spec is also very specific about how and when the checks
   should be applied:
   
   * Each view's quals should be checked *after* inserting into the
   underlying base relation.
   
   * Checks should be applied starting with the innermost views and
   working out to the outermost (top-level) view.
   
   * Obviously the checks apply to INSERT and UPDATE statements only (not DELETE).
   
   In code terms, this suggests that the new check should go in
   ExecInsert/ExectUpdate after firing any AFTER ROW triggers and before
   processing the RETURNING list. The check itself is very similar to a
   regular CHECK constraint, but with a couple of differences:
   
   * if the qual evaluates to NULL it should be a failure since the new
   row won't be visible in the view. That's the opposite logic from a
   CHECK constraint where NULL is OK.
   
   * it probably wants to support sub-queries in the qual (we allow such
   views to be auto-updatable). This doesn't seem so hard to do, and
   makes more logical sense than supporting sub-queries in CHECK
   constraints, since the contents of the view are dynamic and the
   consistency of the constraint is automatically preserved when data in
   referenced tables is modified.
   
   
   Finally, the SQL spec is very strict about which kinds of view are
   allowed to specify the WITH CHECK OPTION.
   
   * WITH LOCAL CHECK OPTION can only be specified on a view that is
   auto-updatable, not one that is trigger-updatable.
   
   * WITH CASCADED CHECK OPTION can only be specified on a view if it is
   auto-updatable, and all its underlying views are also auto-updatable,
   not trigger-updatable.
   
   In order to enforce this, the SQL spec says that there should be an
   additional check in CREATE TRIGGER to ensure that an INSTEAD OF
   trigger isn't defined on a view that has the WITH CHECK OPTION, or a
   view that is
   part of a hierarchy of views where any view higher up has the CASCADED
   CHECK OPTION.
   
   These restrictions seem pretty strict, but on reflection I think that
   they make sense, because the query resulting from a trigger-updatable
   view isn't necessarily going to have access to all the data needed to
   check that view's quals (which may refer to columns in the base
   relation that aren't exposed in the view, and hence aren't returned by
   the trigger).
   
   
   There are similar problems with INSTEAD rules, except I think they are
   worse because the rule could completely rewrite the query, and so even
   a LOCAL WCO won't work in general, if any of the relations underlying
   the view have an INSTEAD rule.
   
   Attached is an initial proof-of-concept patch. It's still missing a
   number of things, in particular it currently doesn't do any of these
   checks for INSTEAD OF triggers or DO INSTEAD rules. The last few
   examples of the
   regression tests show the sorts of things that go wrong without these checks.
   
   Regards,
   Dean
   

2. Re: Thinking about WITH CHECK OPTION for views

   Stephen Frost <sfrost@snowman.net> — 2013-01-20T16:30:01Z

   Dean,
   
   * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
   > I've been thinking about WITH CHECK OPTION for auto-updatable views.
   > Given the timing I doubt if this will be ready for 9.3, since I only
   > get occasional evenings and weekends to hack on postgres, but I think
   > it's probably worth kicking off a discussion, starting with a
   > description of what the feature actually is.
   
   If this isn't intended for 9.3, I think it'd be good to move it to the
   post-9.3 commitfest.  That said, it actually looks pretty decent to me
   on first blush.  I did have a couple of comments though:
   
   Why have validateWithCheckOption instead of handling that in gram.y..?
   
   If the SQL spec says we should be disallowing WITH CHECK when there are
   INSTEAD rules or triggers, could we actually do that..?  This kind of an
   error:
   
   ERROR:  attribute number 2 exceeds number of columns 1
   
   is really pretty bad, any chance we could improve that or disallow the
   possibility of getting there by erroring earlier on?
   
   Lastly, documentation for this appears to be missing.
   
   	Thanks,
   
   		Stephen
   

3. Re: Thinking about WITH CHECK OPTION for views

   Dean Rasheed <dean.a.rasheed@gmail.com> — 2013-01-20T17:29:55Z

   On 20 January 2013 16:30, Stephen Frost <sfrost@snowman.net> wrote:
   > Dean,
   >
   > * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
   >> I've been thinking about WITH CHECK OPTION for auto-updatable views.
   >> Given the timing I doubt if this will be ready for 9.3, since I only
   >> get occasional evenings and weekends to hack on postgres, but I think
   >> it's probably worth kicking off a discussion, starting with a
   >> description of what the feature actually is.
   >
   > If this isn't intended for 9.3, I think it'd be good to move it to the
   > post-9.3 commitfest.
   
   Yes, I was thinking that myself. It doesn't look like I will get much
   time to work on postgresql during this commitfest, so I think it would
   be better spent reviewing other patches rather than trying to squeeze
   this in so late.
   
   
     That said, it actually looks pretty decent to me
   > on first blush.  I did have a couple of comments though:
   >
   > Why have validateWithCheckOption instead of handling that in gram.y..?
   >
   
   Yes gram.y does validate it if you use the standard SQL syntax. But,
   since I've used a new rel_option to store the constraint, it is also
   possible to specify it using the non-standard syntax "WITH
   (check_option=local|cascaded)", so validateWithCheckOption is needed
   to validate that.
   
   
   > If the SQL spec says we should be disallowing WITH CHECK when there are
   > INSTEAD rules or triggers, could we actually do that..?  This kind of an
   > error:
   >
   > ERROR:  attribute number 2 exceeds number of columns 1
   >
   > is really pretty bad, any chance we could improve that or disallow the
   > possibility of getting there by erroring earlier on?
   >
   
   Yes, I think that's the biggest missing piece from the patch as it stands.
   
   I put the tests in so that I could see how it goes wrong if we don't
   disallow triggers or rules on the underlying relations. AFAICT
   triggers don't actually lead to any errors, but the check will be
   silently ignored which is not good. Rules are worse, not only because
   they lead to errors like the above, but also because it looks like
   they need to also be disallowed on all underlying relations even if
   the view only has the LOCAL CHECK OPTION. Triggers are allowed
   provided they are not defined on the view with the check option, or
   one for which a check option cascades down from another view.
   
   Thanks for looking at it. I'll move it 9.4 CF-1.
   
   Regards,
   Dean
   
   
   

4. Re: Thinking about WITH CHECK OPTION for views

   Stephen Frost <sfrost@snowman.net> — 2013-01-20T18:42:56Z

   * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
   > Thanks for looking at it. I'll move it 9.4 CF-1.
   
   Awesome, thanks.
   
   	Stephen