From 15b0421002624919c62ae3c6574af2a8452bf6c4 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 1 May 2013 18:26:58 -0400 Subject: [PATCH] Fix permission tests for views/tables proven empty by constraint exclusion. A view defined as "select where false" had the curious property that the system wouldn't check whether users had the privileges necessary to select from it. More generally, permissions checks could be skipped for tables referenced in sub-selects or views that were proven empty by constraint exclusion (although some quick testing suggests this seldom happens in cases of practical interest). This happened because the planner failed to include rangetable entries for such tables in the finished plan. This was noticed in connection with erroneous handling of materialized views, but actually the issue is quite unrelated to matviews. Therefore, revert commit 200ba1667b3a8d7a9d559d2f05f83d209c9d8267 in favor of a more direct test for the real problem. Back-patch to 9.2 where the bug was introduced (by commit 7741dd6590073719688891898e85f0cb73453159). --- src/backend/optimizer/path/allpaths.c | 4 +- src/backend/optimizer/plan/createplan.c | 54 ++++++++++++++++++------ src/test/regress/expected/privileges.out | 5 +++ src/test/regress/sql/privileges.sql | 4 ++ 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 654ee5849b..0b6a44cbce 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -1129,7 +1129,9 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel, /* * It's possible that constraint exclusion proved the subquery empty. If * so, it's convenient to turn it back into a dummy path so that we will - * recognize appropriate optimizations at this level. + * recognize appropriate optimizations at this query level. (But see + * create_append_plan in createplan.c, which has to reverse this + * substitution.) */ if (is_dummy_plan(rel->subplan)) { diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 99412b6e07..42f58bd5c7 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -664,30 +664,49 @@ static Plan * create_append_plan(PlannerInfo *root, AppendPath *best_path) { Append *plan; - List *tlist = build_relation_tlist(best_path->path.parent); + RelOptInfo *rel = best_path->path.parent; + List *tlist = build_relation_tlist(rel); List *subplans = NIL; ListCell *subpaths; /* - * It is possible for the subplans list to contain only one entry, or even - * no entries. Handle these cases specially. + * The subpaths list could be empty, if every child was proven empty by + * constraint exclusion. In that case generate a dummy plan that returns + * no rows. * - * XXX ideally, if there's just one entry, we'd not bother to generate an - * Append node but just return the single child. At the moment this does - * not work because the varno of the child scan plan won't match the - * parent-rel Vars it'll be asked to emit. + * Note that an AppendPath with no members is also generated in certain + * cases where there was no appending construct at all, but we know the + * relation is empty (see set_dummy_rel_pathlist). */ if (best_path->subpaths == NIL) { - /* Generate a Result plan with constant-FALSE gating qual */ - return (Plan *) make_result(root, - tlist, - (Node *) list_make1(makeBoolConst(false, - false)), - NULL); + /* + * If this is a dummy path for a subquery, we have to wrap the + * subquery's original plan in a SubqueryScan so that setrefs.c will + * do the right things. (In particular, it must pull up the + * subquery's rangetable so that the executor will apply permissions + * checks to those rels at runtime.) + */ + if (rel->rtekind == RTE_SUBQUERY) + { + Assert(is_dummy_plan(rel->subplan)); + return (Plan *) make_subqueryscan(tlist, + NIL, + rel->relid, + rel->subplan); + } + else + { + /* Generate a Result plan with constant-FALSE gating qual */ + return (Plan *) make_result(root, + tlist, + (Node *) list_make1(makeBoolConst(false, + false)), + NULL); + } } - /* Normal case with multiple subpaths */ + /* Build the plan for each child */ foreach(subpaths, best_path->subpaths) { Path *subpath = (Path *) lfirst(subpaths); @@ -695,6 +714,13 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path) subplans = lappend(subplans, create_plan_recurse(root, subpath)); } + /* + * XXX ideally, if there's just one child, we'd not bother to generate an + * Append node but just return the single child. At the moment this does + * not work because the varno of the child scan plan won't match the + * parent-rel Vars it'll be asked to emit. + */ + plan = make_append(subplans, tlist); return (Plan *) plan; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index e8930cb2eb..68afecc91f 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -198,6 +198,8 @@ CREATE VIEW atestv1 AS SELECT * FROM atest1; -- ok /* The next *should* fail, but it's not implemented that way yet. */ CREATE VIEW atestv2 AS SELECT * FROM atest2; CREATE VIEW atestv3 AS SELECT * FROM atest3; -- ok +/* Empty view is a corner case that failed in 9.2. */ +CREATE VIEW atestv0 AS SELECT 0 as x WHERE false; -- ok SELECT * FROM atestv1; -- ok a | b ---+----- @@ -224,6 +226,8 @@ SELECT * FROM atestv3; -- ok -----+-----+------- (0 rows) +SELECT * FROM atestv0; -- fail +ERROR: permission denied for relation atestv0 CREATE VIEW atestv4 AS SELECT * FROM atestv3; -- nested view SELECT * FROM atestv4; -- ok one | two | three @@ -1386,6 +1390,7 @@ drop table dep_priv_test; drop sequence x_seq; DROP FUNCTION testfunc2(int); DROP FUNCTION testfunc4(boolean); +DROP VIEW atestv0; DROP VIEW atestv1; DROP VIEW atestv2; -- this should cascade to drop atestv4 diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index d4d328e649..6ac3378a8d 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -147,6 +147,8 @@ CREATE VIEW atestv1 AS SELECT * FROM atest1; -- ok /* The next *should* fail, but it's not implemented that way yet. */ CREATE VIEW atestv2 AS SELECT * FROM atest2; CREATE VIEW atestv3 AS SELECT * FROM atest3; -- ok +/* Empty view is a corner case that failed in 9.2. */ +CREATE VIEW atestv0 AS SELECT 0 as x WHERE false; -- ok SELECT * FROM atestv1; -- ok SELECT * FROM atestv2; -- fail @@ -158,6 +160,7 @@ SET SESSION AUTHORIZATION regressuser4; SELECT * FROM atestv1; -- ok SELECT * FROM atestv2; -- fail SELECT * FROM atestv3; -- ok +SELECT * FROM atestv0; -- fail CREATE VIEW atestv4 AS SELECT * FROM atestv3; -- nested view SELECT * FROM atestv4; -- ok @@ -828,6 +831,7 @@ drop sequence x_seq; DROP FUNCTION testfunc2(int); DROP FUNCTION testfunc4(boolean); +DROP VIEW atestv0; DROP VIEW atestv1; DROP VIEW atestv2; -- this should cascade to drop atestv4 -- 2.40.0