Обсуждение: WITH CHECK OPTION bug [was RLS Design]

On 20 September 2014 06:13, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:
>>>>>> "Adam" == Brightwell, Adam <adam.brightwell@crunchydatasolutions.com> writes:
>
>  Adam> At any rate, this appears to be a previously existing issue
>  Adam> with WITH CHECK OPTION.  Thoughts?
>
> It's definitely an existing issue; you can reproduce it more simply,
> no need to mess with different users.
>
> The issue as far as I can tell is that the withCheckOption exprs are
> not being processed anywhere in setrefs.c, so it only works at all by
> pure fluke: for most operators, the opfuncid is also filled in by
> eval_const_expressions, but for whatever reason SAOPs escape this
> treatment. Same goes for other similar cases:
>
> create table colors (name text);
> create view vc1 as select * from colors where name is distinct from 'grue' with check option;
> create view vc2 as select * from colors where name in ('red','green','blue') with check option;
> create view vc3 as select * from colors where nullif(name,'grue') is null with check option;
>
> insert into vc1 values ('red'); -- fails
> insert into vc2 values ('red'); -- fails
> insert into vc3 values ('red'); -- fails
>

Oh dear. I remember thinking at the time I wrote the WITH CHECK OPTION
stuff that I needed to check all the places that did returningLists
processing, because they would probably need similar processing for
withCheckOptionLists, but somehow I missed that one place.

Fortunately it looks pretty trivial though. The patch attached fixes
the above test cases.

Obviously this needs to be fixed in 9.4 and HEAD.

Regards,
Dean

On Sat, Sep 20, 2014 at 7:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Fortunately it looks pretty trivial though. The patch attached fixes
> the above test cases.
> Obviously this needs to be fixed in 9.4 and HEAD.
Wouldn't it be better if bundled with some regression tests?
-- 
Michael

On 20 September 2014 14:08, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Sat, Sep 20, 2014 at 7:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> Fortunately it looks pretty trivial though. The patch attached fixes
>> the above test cases.
>> Obviously this needs to be fixed in 9.4 and HEAD.
> Wouldn't it be better if bundled with some regression tests?

Yeah OK, fair point. Here are some tests that cover that code path.
I've also thrown in a test with prepared statements, although that
case was already working, it seemed worth checking.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 20 September 2014 14:08, Michael Paquier <michael.paquier@gmail.com> wrote:
> > On Sat, Sep 20, 2014 at 7:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >> Fortunately it looks pretty trivial though. The patch attached fixes
> >> the above test cases.
> >> Obviously this needs to be fixed in 9.4 and HEAD.
> > Wouldn't it be better if bundled with some regression tests?

Agreed.

> Yeah OK, fair point. Here are some tests that cover that code path.
> I've also thrown in a test with prepared statements, although that
> case was already working, it seemed worth checking.

Thanks!  Looks good, but will review in more depth, address the other
comments on this thread (typo, adding more documentation) and
investigate the results from the Coverity run this morning this evening
and should be able to get everything addressed in the next couple days.
Obviously, I'll be back-patching this fix to 9.4 too.
Thanks again!
    Stephen

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> Yeah OK, fair point. Here are some tests that cover that code path.
> I've also thrown in a test with prepared statements, although that
> case was already working, it seemed worth checking.

Applied and backpatched, thanks!
Stephen