Re: ExecRTCheckPerms() and many prunable partitions - Mailing list pgsql-hackers

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@gmail.com> wrote:
>
> On Fri, Mar 25, 2022 at 4:46 AM David Rowley <dgrowleyml@gmail.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