Re: ExecRTCheckPerms() and many prunable partitions

This is failing regression tests. I don't understand how this patch
could be affecting this test though. Perhaps it's a problem with the
json patches that were committed recently -- but they don't seem to be
causing other patches to fail.

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/jsonb_sqljson.out
/tmp/cirrus-ci-build/src/test/regress/results/jsonb_sqljson.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/jsonb_sqljson.out
2022-04-05 12:15:40.590168291 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/jsonb_sqljson.out
2022-04-05 12:20:17.338045137 +0000
@@ -1159,37 +1159,37 @@
);
\sv jsonb_table_view
CREATE OR REPLACE VIEW public.jsonb_table_view AS
- SELECT "json_table".id,
- "json_table".id2,
- "json_table"."int",
- "json_table".text,
- "json_table"."char(4)",
- "json_table".bool,
- "json_table"."numeric",
- "json_table".domain,
- "json_table".js,
- "json_table".jb,
- "json_table".jst,
- "json_table".jsc,
- "json_table".jsv,
- "json_table".jsb,
- "json_table".jsbq,
- "json_table".aaa,
- "json_table".aaa1,
- "json_table".exists1,
- "json_table".exists2,
- "json_table".exists3,
- "json_table".js2,
- "json_table".jsb2w,
- "json_table".jsb2q,
- "json_table".ia,
- "json_table".ta,
- "json_table".jba,
- "json_table".a1,
- "json_table".b1,
- "json_table".a11,
- "json_table".a21,
- "json_table".a22
+ SELECT id,
+ id2,
+ "int",
+ text,
+ "char(4)",
+ bool,
+ "numeric",
+ domain,
+ js,
+ jb,
+ jst,
+ jsc,
+ jsv,
+ jsb,
+ jsbq,
+ aaa,
+ aaa1,
+ exists1,
+ exists2,
+ exists3,
+ js2,
+ jsb2w,
+ jsb2q,
+ ia,
+ ta,
+ jba,
+ a1,
+ b1,
+ a11,
+ a21,
+ a22
FROM JSON_TABLE(
'null'::jsonb, '$[*]'
PASSING

On Wed, 30 Mar 2022 at 23:16, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> 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

--
greg