Re: [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, May 1, 2013 at 6:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Fix permission tests for views/tables proven empty by constraint exclusion.

> I believe that this commit is responsible for the fact that the
> following test case now crashes the server:

> rhaas=# create or replace view foo (x) AS (select 1 union all select 2);
> CREATE VIEW
> rhaas=# select * from foo where false;
> The connection to the server was lost. Attempting reset: Failed.

OK, after looking a bit closer, this is actually a situation I had been
wondering about last week, but I failed to construct a test case proving
there was a problem. The issue is that the "append rel" code path also
needs changes to handle the fact that subqueries that are appendrel
members might get proven empty by constraint exclusion. (Even aside
from the crash introduced here, the excluded subqueries fail to
contribute to the final rangetable for permissions checks.)

Unfortunately this blows a bit of a hole in the minimal fix I committed
last week; there's no nice way to include these dummy subqueries into
the plan tree that's passed on to setrefs.c. Ideally we'd add an
out-of-band transmission path for them, and I think I will fix it that
way in HEAD, but that requires adding a new List field to PlannerInfo.
I'm afraid to do that in 9.2 for fear of breaking existing planner
plugin modules.

One way to fix it in 9.2 is to teach setrefs.c to thumb through the
append_rel_list looking for excluded child subqueries to pull up their
rangetables. Icky, but it shouldn't cost very many cycles in typical
queries. The main downside of this solution is that it would add at
least several dozen lines of new-and-poorly-tested code to a back
branch, where it would get no additional testing to speak of before
being loosed on users.

The other way I can think of to handle it without PlannerInfo changes
is to lobotomize set_append_rel_pathlist so that it doesn't omit dummy
children from the AppendPath it constructs. The main downside of this
is that fully dummy appendrels (those with no live children at all,
such as in your example) wouldn't be recognized by IS_DUMMY_PATH,
so the quality of planning in the outer query would be slightly
degraded. But such cases are probably sufficiently unusual that this
might be an okay price to pay for a back branch.

Thoughts?

regards, tom lane