Re: [BUGS] BUG #4350: 'select' acess given to views containing "union all" even though user has no grants

Index: src/backend/optimizer/prep/prepjointree.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/optimizer/prep/prepjointree.c,v
retrieving revision 1.44
diff -c -r1.44 prepjointree.c
*** src/backend/optimizer/prep/prepjointree.c 4 Oct 2006 00:29:54 -0000 1.44
--- src/backend/optimizer/prep/prepjointree.c 12 Aug 2008 07:39:30 -0000
***************
*** 46,52 ****
static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
RangeTblEntry *rte);
static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
! int parentRTindex, Query *setOpQuery);
static void make_setop_translation_lists(Query *query,
Index newvarno,
List **col_mappings, List **translated_vars);
--- 46,52 ----
static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
RangeTblEntry *rte);
static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
! int parentRTindex, Query *setOpQuery, Bitmapset **pulledUpRtes);
static void make_setop_translation_lists(Query *query,
Index newvarno,
List **col_mappings, List **translated_vars);
***************
*** 477,482 ****
--- 477,485 ----
{
int varno = ((RangeTblRef *) jtnode)->rtindex;
Query *subquery = rte->subquery;
+ Bitmapset *pulledUpRtes = NULL;
+ ListCell *l;
+ int i;

/*
* Recursively scan the subquery's setOperations tree and copy the leaf
***************
*** 484,490 ****
* them to the parent's append_rel_list, too.
*/
Assert(subquery->setOperations);
! pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery);

/*
* Mark the parent as an append relation.
--- 487,512 ----
* them to the parent's append_rel_list, too.
*/
Assert(subquery->setOperations);
! pull_up_union_leaf_queries(subquery->setOperations, root, varno, subquery,
! &pulledUpRtes);
!
! /*
! * pull_up_union_leaf_queries already copied those range table entries
! * from the subqueries to parent that were referenced in the subqueries,
! * but we need to make sure that all non-referenced ones are copied as
! * well. They are needed for permission checks during executor startup
! * (ExecCheckRTPerms).
! */
! i = 1;
! foreach(l, subquery->rtable)
! {
! if (!bms_is_member(i, pulledUpRtes))
! {
! RangeTblEntry *rte = rt_fetch(i, subquery->rtable);
! root->parse->rtable = lappend(root->parse->rtable, rte);
! }
! i++;
! }

/*
* Mark the parent as an append relation.
***************
*** 501,510 ****
* is where to look up the RTE if setOp is a RangeTblRef. This is *not* the
* same as root->parse, which is the top-level Query we are pulling up into.
* parentRTindex is the appendrel parent's index in root->parse->rtable.
*/
static void
pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
! Query *setOpQuery)
{
if (IsA(setOp, RangeTblRef))
{
--- 523,534 ----
* is where to look up the RTE if setOp is a RangeTblRef. This is *not* the
* same as root->parse, which is the top-level Query we are pulling up into.
* parentRTindex is the appendrel parent's index in root->parse->rtable.
+ * pulledUpRtes is a bitmap of those range table entries in setOpQuery that
+ * have already been copied to the parent
*/
static void
pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
! Query *setOpQuery, Bitmapset **pulledUpRtes)
{
if (IsA(setOp, RangeTblRef))
{
***************
*** 535,540 ****
--- 559,565 ----
*/
parse->rtable = lappend(parse->rtable, rte);
childRTindex = list_length(parse->rtable);
+ *pulledUpRtes = bms_add_member(*pulledUpRtes, rtr->rtindex);

/*
* Build a suitable AppendRelInfo, and attach to parent's list.
***************
*** 566,573 ****
SetOperationStmt *op = (SetOperationStmt *) setOp;

/* Recurse to reach leaf queries */
! pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery);
! pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery);
}
else
{
--- 591,600 ----
SetOperationStmt *op = (SetOperationStmt *) setOp;

/* Recurse to reach leaf queries */
! pull_up_union_leaf_queries(op->larg, root, parentRTindex, setOpQuery,
! pulledUpRtes);
! pull_up_union_leaf_queries(op->rarg, root, parentRTindex, setOpQuery,
! pulledUpRtes);
}
else
{
Tom Lane wrote:
> I wrote:
>> That's one heck of a scary patch: nowhere in list_union's API is there
>> any guarantee that it preserves list ordering, but we *must not* change
>> the positions of the existing rtable entries.

Good point.

> Actually there's a more fundamental problem, namely that pulled-up
> subqueries aren't necessarily equal() to the originals. They will
> definitely be different if there were any uplevel Var references.

Another good point. I just saw that they're copied with copyObject and
are thus equal, but missed the IncrementVarSublevelsUp call.

> While you could argue that it doesn't matter because we'll only
> end up redundantly checking permissions on multiple copies of the
> RTEs, that's a bit beyond my threshold of ugliness...

Yeah, that wasn't the intention.

Attached is a patch with slightly different approach. Instead of
list_union, I'm keeping track of which rtes are copied by
pull_up_union_leaf_queries in a bitmapset.

BTW, I wonder if it's possible to end up with multiple copies of the
same RTE anyway, if there's multiple references to the same RTE. I'm
guessing that it's either not possible or we don't care, because that
can happen without the patch just as well.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com