Re: [HACKERS] WITH CHECK OPTION bug [was RLS Design]

2014-09-22 Thread Stephen Frost
* Dean Rasheed (dean.a.rash...@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


signature.asc
Description: Digital signature


Re: [HACKERS] WITH CHECK OPTION bug [was RLS Design]

2014-09-21 Thread Dean Rasheed
On 20 September 2014 14:08, Michael Paquier michael.paqu...@gmail.com wrote:
 On Sat, Sep 20, 2014 at 7:03 AM, Dean Rasheed dean.a.rash...@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
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
new file mode 100644
index 5bf84c1..9ddc8ad
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
*** set_plan_refs(PlannerInfo *root, Plan *p
*** 696,701 
--- 696,704 
  Assert(splan-plan.targetlist == NIL);
  Assert(splan-plan.qual == NIL);
  
+ splan-withCheckOptionLists =
+ 	fix_scan_list(root, splan-withCheckOptionLists, rtoffset);
+ 
  if (splan-returningLists)
  {
  	List	   *newRL = NIL;
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index 6a35925..8a81251
*** a/src/test/regress/expected/updatable_views.out
--- b/src/test/regress/expected/updatable_views.out
*** NOTICE:  drop cascades to 3 other object
*** 1567,1572 
--- 1567,1592 
  DETAIL:  drop cascades to view rw_view1
  drop cascades to view rw_view2
  drop cascades to view rw_view3
+ -- WITH CHECK OPTION with scalar array ops
+ CREATE TABLE base_tbl (a int, b int[]);
+ CREATE VIEW rw_view1 AS SELECT * FROM base_tbl WHERE a = ANY (b)
+   WITH CHECK OPTION;
+ INSERT INTO rw_view1 VALUES (1, ARRAY[1,2,3]); -- ok
+ INSERT INTO rw_view1 VALUES (10, ARRAY[4,5]); -- should fail
+ ERROR:  new row violates WITH CHECK OPTION for rw_view1
+ DETAIL:  Failing row contains (10, {4,5}).
+ UPDATE rw_view1 SET b[2] = -b[2] WHERE a = 1; -- ok
+ UPDATE rw_view1 SET b[1] = -b[1] WHERE a = 1; -- should fail
+ ERROR:  new row violates WITH CHECK OPTION for rw_view1
+ DETAIL:  Failing row contains (1, {-1,-2,3}).
+ PREPARE ins(int, int[]) AS INSERT INTO rw_view1 VALUES($1, $2);
+ EXECUTE ins(2, ARRAY[1,2,3]); -- ok
+ EXECUTE ins(10, ARRAY[4,5]); -- should fail
+ ERROR:  new row violates WITH CHECK OPTION for rw_view1
+ DETAIL:  Failing row contains (10, {4,5}).
+ DEALLOCATE PREPARE ins;
+ DROP TABLE base_tbl CASCADE;
+ NOTICE:  drop cascades to view rw_view1
  -- WITH CHECK OPTION with subquery
  CREATE TABLE base_tbl (a int);
  CREATE TABLE ref_tbl (a int PRIMARY KEY);
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index c072fca..60c7e29
*** a/src/test/regress/sql/updatable_views.sql
--- b/src/test/regress/sql/updatable_views.sql
*** INSERT INTO rw_view3 VALUES (3); -- ok
*** 707,712 
--- 707,731 
  
  DROP TABLE base_tbl CASCADE;
  
+ -- WITH CHECK OPTION with scalar array ops
+ 
+ CREATE TABLE base_tbl (a int, b int[]);
+ CREATE VIEW rw_view1 AS SELECT * FROM base_tbl WHERE a = ANY (b)
+   WITH CHECK OPTION;
+ 
+ INSERT INTO rw_view1 VALUES (1, ARRAY[1,2,3]); -- ok
+ INSERT INTO rw_view1 VALUES (10, ARRAY[4,5]); -- should fail
+ 
+ UPDATE rw_view1 SET b[2] = -b[2] WHERE a = 1; -- ok
+ UPDATE rw_view1 SET b[1] = -b[1] WHERE a = 1; -- should fail
+ 
+ PREPARE ins(int, int[]) AS INSERT INTO rw_view1 VALUES($1, $2);
+ EXECUTE ins(2, ARRAY[1,2,3]); -- ok
+ EXECUTE ins(10, ARRAY[4,5]); -- should fail
+ DEALLOCATE PREPARE ins;
+ 
+ DROP TABLE base_tbl CASCADE;
+ 
  -- WITH CHECK OPTION with subquery
  
  CREATE TABLE base_tbl (a int);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] WITH CHECK OPTION bug [was RLS Design]

2014-09-21 Thread Stephen Frost
Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 20 September 2014 14:08, Michael Paquier michael.paqu...@gmail.com wrote:
  On Sat, Sep 20, 2014 at 7:03 AM, Dean Rasheed dean.a.rash...@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


signature.asc
Description: Digital signature


[HACKERS] WITH CHECK OPTION bug [was RLS Design]

2014-09-20 Thread Dean Rasheed
On 20 September 2014 06:13, Andrew Gierth and...@tao11.riddles.org.uk wrote:
 Adam == Brightwell, Adam adam.brightw...@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
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
new file mode 100644
index 5bf84c1..9ddc8ad
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
*** set_plan_refs(PlannerInfo *root, Plan *p
*** 696,701 
--- 696,704 
  Assert(splan-plan.targetlist == NIL);
  Assert(splan-plan.qual == NIL);
  
+ splan-withCheckOptionLists =
+ 	fix_scan_list(root, splan-withCheckOptionLists, rtoffset);
+ 
  if (splan-returningLists)
  {
  	List	   *newRL = NIL;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] WITH CHECK OPTION bug [was RLS Design]

2014-09-20 Thread Michael Paquier
On Sat, Sep 20, 2014 at 7:03 AM, Dean Rasheed dean.a.rash...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers