Re: Thinking about WITH CHECK OPTION for views

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

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)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

On 20 January 2013 16:30, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Dean,
>
> * Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)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

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

Awesome, thanks.

Stephen