Re: pgsql: Add regression test for bug fixed by recent refactoring.

Add regression test for bug fixed by recent refactoring.

Test case by Andres Freund for bug fixed by Tom Lane's refactoring
in commit 5194024d72f33fb209e10f9ab0ada7cc67df45b7

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/200ba1667b3a8d7a9d559d2f05f83d209c9d8267

Modified Files
--------------
src/test/regress/expected/matview.out | 12 ++++++++++++
src/test/regress/sql/matview.sql | 7 +++++++
2 files changed, 19 insertions(+), 0 deletions(-)

Kevin Grittner <kgrittn(at)postgresql(dot)org> writes:
> Add regression test for bug fixed by recent refactoring.
> Test case by Andres Freund for bug fixed by Tom Lane's refactoring
> in commit 5194024d72f33fb209e10f9ab0ada7cc67df45b7

Hm, that actually has got nothing much to do with matviews:

regression=# create view vv1 as select false;
CREATE VIEW
regression=# create view vv2 as select false where false;
CREATE VIEW
regression=# create user joe;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> select * from vv1;
ERROR: permission denied for relation vv1
regression=> select * from vv2;
bool
------
(0 rows)

Of course the select from vv2 should fail as well, but it doesn't,
because vv2 is nowhere to be seen in the rangetable passed to the
executor. I think the planner is probably trashing the rangetable
once it realizes that the query is completely dummy; but this is wrong
because we need to leave view rangetable entries behind for permissions
checks, even when they're unreferenced in the finished plan.

regards, tom lane

I wrote:
> Of course the select from vv2 should fail as well, but it doesn't,
> because vv2 is nowhere to be seen in the rangetable passed to the
> executor. I think the planner is probably trashing the rangetable
> once it realizes that the query is completely dummy; but this is wrong
> because we need to leave view rangetable entries behind for permissions
> checks, even when they're unreferenced in the finished plan.

I found the problem. set_subquery_pathlist does this when the subquery
is demonstrably empty (ie "select something where false"):

/*
* 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.
*/
if (is_dummy_plan(rel->subplan))
{
set_dummy_rel_pathlist(rel);
return;
}

The trouble is that, with no SubqueryScan path, setrefs.c doesn't
realize that it needs to pull up the subquery's rangetable into the main
query. We could just delete the above chunk of code, but that would
result in worse planning choices in some complicated queries.
What I'm planning to do instead is teach createplan.c to compensate for
this by wrapping the dummy Result plan in a SubqueryScan, thus producing
the same plan tree as if set_subquery_pathlist had not done the above.
This is a tad Rube Goldbergian, perhaps, because after setrefs.c has
done its thing it will conclude that the SubqueryScan is pointless and
strip it off again :-). (So we end up with the same finished plan tree
as now, only it will have all the rangetable entries it should have for
permissions checking purposes.) However, the alternative seems to be
to devise some entirely new method for setrefs.c to figure out which
subqueries' rangetables still need to be pulled up into the main query
and which don't. That would be a much more invasive patch, without a
lot of redeeming social value AFAICS.

regards, tom lane