Re: ExecRTCheckPerms() and many prunable partitions

On Fri, Mar 25, 2022 at 4:46 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> I had a look at the v10-0001 patch. I agree that it seems to be a good
> idea to separate out the required permission checks rather than having
> the Bitmapset to index the interesting range table entries.

Thanks David for taking a look at this.

> One thing that I could just not determine from looking at the patch
> was if there's meant to be just 1 RelPermissionInfo per RTE or per rel
> Oid.

It's the latter.

> None of the comments helped me understand this

I agree that the comment above the RelPermissionInfo struct definition
missed mentioning this really important bit. I've tried fixing that
as:

@@ -1142,7 +1142,9 @@ typedef struct RangeTblEntry
* Per-relation information for permission checking. Added to the query
* by the parser when populating the query range table and subsequently
* editorialized on by the rewriter and the planner. There is an entry
- * each for all RTE_RELATION entries present in the range table.
+ * each for all RTE_RELATION entries present in the range table, though
+ * different RTEs for the same relation share the
RelPermissionInfo, that
+ * is, there is only one RelPermissionInfo containing a given relid.

> and
> MergeRelPermissionInfos() seems to exist that appears to try and
> uniquify this to some extent. I just see no code that does this
> process for a single query level. I've provided more detail on this in
> #5 below.
>
> Here's my complete review of v10-0001:
>
> 1. ExecCheckPermisssions -> ExecCheckPermissions

Fixed.

> 2. I think you'll want to move the userid field away from below a
> comment that claims the following fields are for foreign tables only.
>
> /* Information about foreign tables and foreign joins */
> Oid serverid; /* identifies server for the table or join */
> - Oid userid; /* identifies user to check access as */
> + Oid userid; /* identifies user to check access as; set
> + * in non-foreign table relations too! */

Actually, I realized that the comment should not have been touched to
begin with. Reverted.

> 3. This should use READ_OID_FIELD()
>
> READ_INT_FIELD(checkAsUser);
>
> and this one:
>
> READ_INT_FIELD(relid);

Fixed.

> 4. This looks wrong:
>
> - rel->userid = rte->checkAsUser;
> + if (rte->rtekind == RTE_RELATION)
> + {
> + /* otherrels use the root parent's value. */
> + rel->userid = parent ? parent->userid :
> + GetRelPermissionInfo(root->parse->relpermlist,
> + rte, false)->checkAsUser;
> + }
>
> If 'parent' is false then you'll assign the result of
> GetRelPermissionInfo (a RelPermissionInfo *) to an Oid.

Hmm, I don't see a problem, because what's being assigned is
`GetRelPermissionInfo(...)->checkAsUser`.

Anyway, I rewrote the block more verbosely as:

if (rte->rtekind == RTE_RELATION)
{
- /* otherrels use the root parent's value. */
- rel->userid = parent ? parent->userid :
- GetRelPermissionInfo(root->parse->relpermlist,
- rte, false)->checkAsUser;
+ /*
+ * Get the userid from the relation's RelPermissionInfo, though
+ * only the tables mentioned in query are assigned RelPermissionInfos.
+ * Child relations (otherrels) simply use the parent's value.
+ */
+ if (parent == NULL)
+ {
+ RelPermissionInfo *perminfo =
+ GetRelPermissionInfo(root->parse->relpermlist, rte, false);
+
+ rel->userid = perminfo->checkAsUser;
+ }
+ else
+ rel->userid = parent->userid;
}
+ else
+ rel->userid = InvalidOid;

> 5. I'm not sure if there's a case that can break this one, but I'm not
> very confident that there's not one:
>
> I'm not sure I agree with how you've coded GetRelPermissionInfo().
> You're searching for a RelPermissionInfo based on the table's Oid. If
> you have multiple RelPermissionInfos for the same Oid then this will
> just find the first one and return it, but that might not be the one
> for the RangeTblEntry in question.
>
> Here's an example of the sort of thing that could have problems with this:
>
> postgres=# create role bob;
> CREATE ROLE
> postgres=# create table ab (a int, b int);
> CREATE TABLE
> postgres=# grant all (a) on table ab to bob;
> GRANT
> postgres=# set session authorization bob;
> SET
> postgres=> update ab set a = (select b from ab);
> ERROR: permission denied for table ab
>
> The patch does correctly ERROR out here on permission failure, but as
> far as I can see, that's just due to the fact that we're checking the
> permissions of all items in the PlannedStmt.relpermlist List. If
> there had been code that had tried to find the RelPermissionInfo based
> on the relation's Oid then we'd have accidentally found that we only
> need an UPDATE permission on (a). SELECT on (b) wouldn't have been
> checked.
>
> As far as I can see, to fix that you'd either need to store the RTI of
> the RelPermissionInfo and lookup based on that, or you'd have to
> bms_union() all the columns and bitwise OR the required permissions
> and ensure you only have 1 RelPermissionInfo per Oid.
>
> The fact that I have two entries when I debug InitPlan() seems to
> disagree with what the comment in AddRelPermissionInfo() is claiming
> should happen:
>
> /*
> * To prevent duplicate entries for a given relation, check if already in
> * the list.
> */
>
> I'm not clear on if the list is meant to be unique on Oid or not.

Yeah, it is, but it seems that the code I added in
add_rtes_to_flat_rtable() to accumulate various subplans' relpermlists
into finalrelpermlist didn't actually bother to unique'ify the list.
It used list_concat() to combine finalrelpermlist and a given
subplan's relpermlist, instead of MergeRelPemissionInfos to merge the
two lists.

I've fixed that in the attached and can now see that the plan for the
query in your example ends up with just one RelPermissionInfo which
combines the permissions and column bitmapsets for all operations.

> 6. acesss?
>
> - * Set flags and access permissions.
> + * Set flags and initialize acesss permissions.
>
> 7. I was expecting to see an |= here:
>
> + /* "merge" proprties. */
> + dest_perminfo->inh = src_perminfo->inh;
>
> Why is a plain assignment ok?

You're perhaps right that |= is correct. I forget the details but I
think I added 'inh' field to RelPemissionInfo to get some tests in
sepgsql contrib module to pass and those tests apparently didn't mind
the current coding.

> 8. Some indentation problems here:
>
> @@ -3170,6 +3148,8 @@ rewriteTargetView(Query *parsetree, Relation view)
>
> base_rt_index = rtr->rtindex;
> base_rte = rt_fetch(base_rt_index, viewquery->rtable);
> +base_perminfo = GetRelPermissionInfo(viewquery->relpermlist, base_rte,
> + false);

Fixed.

>
> 9. You can use foreach_current_index(lc) + 1 in:
>
> + i = 0;
> + foreach(lc, relpermlist)
> + {
> + perminfo = (RelPermissionInfo *) lfirst(lc);
> + if (perminfo->relid == rte->relid)
> + {
> + /* And set the index in RTE. */
> + rte->perminfoindex = i + 1;
> + return perminfo;
> + }
> + i++;
> + }

Oh, nice tip. Done.

> 10. I think the double quote is not in the correct place in this comment:
>
> List *finalrtable; /* "flat" rangetable for executor */
>
> + List *finalrelpermlist; /* "flat list of RelPermissionInfo "*/
>
>
> 11. Looks like an accidental change:
>
> +++ b/src/include/optimizer/planner.h
> @@ -58,4 +58,5 @@ extern Path *get_cheapest_fractional_path(RelOptInfo *rel,
>
> extern Expr *preprocess_phv_expression(PlannerInfo *root, Expr *expr);
>
> +
>
> 12. These need to be broken into multiple lines:
>
> +extern RelPermissionInfo *AddRelPermissionInfo(List **relpermlist,
> RangeTblEntry *rte);
> +extern void MergeRelPermissionInfos(Query *dest_query, List *src_relpermlist);
> +extern RelPermissionInfo *GetRelPermissionInfo(List *relpermlist,
> RangeTblEntry *rte, bool missing_ok);

All fixed.

v11 attached.

--
Amit Langote
EDB: http://www.enterprisedb.com