Subject: Re: ExecRTCheckPerms() and many prunable partitions

From: Amit Langote <[email-masked]>
Date: Mon, 17 Jan 2022 20:50:42 +0900

On Thu, Jan 13, 2022 at 3:39 PM Amit Langote <[email-masked]> wrote:
> On Thu, Jan 13, 2022 at 12:10 PM Julien Rouhaud <[email-masked]> wrote:
> > On Mon, Dec 20, 2021 at 04:13:04PM +0900, Amit Langote wrote:
> > > Patch 0002 needed a rebase, because a conflicting change to
> > > expected/rules.out has since been committed.
> >
> > The cfbot reports new conflicts since about a week ago with this patch:
> > Could you send a rebased patch?  In the meantime I'll switch the cf entry to
> > Waiting on Author.
>
> Turns out I had never compiled this patch set to exercise xml and lz4
> tests, whose output files contained view definitions shown using \d
> that also needed to be updated in the 0002 patch.
>
> Fixed in the attached updated version.

cfbot tells me it found a conflict when applying v7 on the latest
HEAD.  Fixed in the attached v8.

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

---

From: Zhihong Yu <[email-masked]>
Date: Mon, 17 Jan 2022 07:45:06 -0800

On Mon, Jan 17, 2022 at 3:51 AM Amit Langote <[email-masked]>
wrote:

> Hi,
For patch 02, in the description:

present for locking views during execition

Typo: execution.

+    * to be used by the executor to lock the view relation and for the
+    * planner to be able to record the view relation OID in the PlannedStmt
+    * that it produces for the query.

I think the sentence about executor can be placed after the sentence for
the planner.

For patch 01, GetRelPermissionInfo():

+       return perminfo;
+   }
+   else

keyword 'else' is not needed - the else block can be left-indented.

Cheers

---

From: Amit Langote <[email-masked]>
Date: Mon, 14 Mar 2022 16:36:53 +0900

> For patch 02, in the description:

Thanks for looking.

> I think the sentence about executor can be placed after the sentence for the planner.

Fixed.

> keyword 'else' is not needed - the else block can be left-indented.

OK, done.

Also needed fixes when rebasing.

---

From: Amit Langote <[email-masked]>
Date: Wed, 23 Mar 2022 16:03:04 +0900

> Also needed fixes when rebasing.

Needed another rebase.

As the changes being made with the patch are non-trivial and the patch
hasn't been reviewed very significantly since Alvaro's comments back
in Sept 2021 which I've since addressed, I'm thinking of pushing this
one into the version 16 dev cycle.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 23 Mar 2022 10:49:53 +0100

> one into the version 16 dev cycle.

Let's not get ahead of ourselves.  The commitfest is not yet over.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

---

From: Zhihong Yu <[email-masked]>
Date: Wed, 23 Mar 2022 14:02:10 -0700

> one into the version 16 dev cycle.

Hi,
For patch 1:

bq. makes permissions-checking needlessly expensive when many inheritance
children are added to the range range

'range' is repeated in the above sentence.

+ExecCheckOneRelPerms(RelPermissionInfo *perminfo)

Since RelPermissionInfo is for one relation, I think the 'One' in func name
can be dropped.

+       else                    /* this isn't a child result rel */
+           resultRelInfo->ri_RootToChildMap = NULL;
...
+       resultRelInfo->ri_RootToChildMapValid = true;

Should the assignment of true value be moved into the if block (in the else
block, ri_RootToChildMap is assigned NULL) ?

+   /* Looks like the RTE doesn't, so try to find it the hard way. */

doesn't -> doesn't know

Cheers

---

From: David Rowley <[email-masked]>
Date: Fri, 25 Mar 2022 08:46:09 +1300

> > Also needed fixes when rebasing.

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.

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. None of the comments helped me understand this 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

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! */

3. This should use READ_OID_FIELD()

READ_INT_FIELD(checkAsUser);

and this one:

READ_INT_FIELD(relid);

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.

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.

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?

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);

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++;
+ }

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);

David

---

From: Amit Langote <[email-masked]>
Date: Thu, 31 Mar 2022 12:16:02 +0900

> the Bitmapset to index the interesting range table entries.

Thanks David for taking a look at this.

> 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

Fixed.

> + * in non-foreign table relations too! */

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

Fixed.

> 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;

> 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.

> 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.

Fixed.

> + rte->perminfoindex = i + 1;

Oh, nice tip.  Done.

> RangeTblEntry *rte, bool missing_ok);

All fixed.

v11 attached.

---

From: Greg Stark <[email-masked]>
Date: Tue, 5 Apr 2022 10:26:58 -0400

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

--
greg

---

From: David Rowley <[email-masked]>
Date: Wed, 6 Apr 2022 08:21:40 +1200

> causing other patches to fail.

I think this will just be related to the useprefix =
list_length(es->rtable) > 1; in show_plan_tlist().  There's likely not
much point in keeping the RTE for the view anymore. IIRC it was just
there to check permissions. Amit has now added another way of doing
those.

David

---

From: Amit Langote <[email-masked]>
Date: Wed, 6 Apr 2022 10:18:48 +0900

> there to check permissions. Amit has now added another way of doing

That is correct.

I have rebased the patch and updated expected output of the failing test.

---

From: Ian Lawrence Barwick <[email-masked]>
Date: Fri, 4 Nov 2022 08:46:02 +0900

2022年10月15日(土) 15:01 Amit Langote <[email-masked]>:
>
> On Fri, Oct 7, 2022 at 4:31 PM Amit Langote <[email-masked]> wrote:
> > On Fri, Oct 7, 2022 at 3:49 PM Amit Langote <[email-masked]> wrote:
> > > On Fri, Oct 7, 2022 at 1:25 PM Amit Langote <[email-masked]> wrote:
> > > > On Fri, Oct 7, 2022 at 10:04 AM Amit Langote <[email-masked]> wrote:
> > > > > On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <[email-masked]> wrote:
> > > > > > Actually, List of Bitmapsets turned out to be something that doesn't
> > > > > > just-work with our Node infrastructure, which I found out thanks to
> > > > > > -DWRITE_READ_PARSE_PLAN_TREES.  So, I had to go ahead and add
> > > > > > first-class support for copy/equal/write/read support for Bitmapsets,
> > > > > > such that writeNode() can write appropriately labeled versions of them
> > > > > > and nodeRead() can read them as Bitmapsets.  That's done in 0003.  I
> > > > > > didn't actually go ahead and make *all* Bitmapsets in the plan trees
> > > > > > to be Nodes, but maybe 0003 can be expanded to do that.  We won't need
> > > > > > to make gen_node_support.pl emit *_BITMAPSET_FIELD() blurbs then; can
> > > > > > just use *_NODE_FIELD().
> > > > >
> > > > > All meson builds on the cfbot machines seem to have failed, maybe
> > > > > because I didn't update src/include/nodes/meson.build to add
> > > > > 'nodes/bitmapset.h' to the `node_support_input_i` collection.  Here's
> > > > > an updated version assuming that's the problem.  (Will set up meson
> > > > > builds on my machine to avoid this in the future.)
> > > >
> > > > And... noticed that a postgres_fdw test failed, because
> > > > _readBitmapset() not having been changed to set NodeTag would
> > > > "corrupt" any Bitmapsets that were created with it set.
> > >
> > > Broke the other cases while fixing the above.  Attaching a new version
> > > again.  In the latest version, I'm setting Bitmapset.type by hand with
> > > an XXX comment nearby saying that it would be nice to change that to
> > > makeNode(Bitmapset), which I know sounds pretty ad-hoc.
> >
> > Sorry, I attached the wrong patches with the last email.  The
> > "correct" v22 attached this time.
>
> Rebased over c037471832.

This entry was marked as "Needs review" in the CommitFest app but cfbot
reports the patch no longer applies.

We've marked it as "Waiting on Author". As CommitFest 2022-11 is
currently underway, this would be an excellent time update the patch.

Once you think the patchset is ready for review again, you (or any
interested party) can  move the patch entry forward by visiting

    https://commitfest.postgresql.org/40/3224/

and changing the status to "Needs review".

Thanks

Ian Barwick

---

From: Amit Langote <[email-masked]>
Date: Mon, 7 Nov 2022 16:03:45 +0900

> currently underway, this would be an excellent time update the patch.

Thanks for the heads up.

> and changing the status to "Needs review".

Rebased patch attached and done.

-- 
Thanks, Amit Langote

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 10 Nov 2022 12:58:01 +0100

Hello

I've been trying to understand what 0001 does.  One thing that strikes
me is that it seems like it'd be easy to have bugs because of modifying
the perminfo list inadequately.  I couldn't find any cross-check that
some perminfo element that we obtain for a rte does actually match the
relation we wanted to check.  Maybe we could add a test in some central
place that perminfo->relid equals rte->relid?

A related point is that concatenating lists doesn't seem to worry about
not processing one element multiple times and ending up with bogus offsets.
(I suppose you still have to let an element be processed multiple times
in case you have nested subqueries?  I wonder how good is the test
coverage for such scenarios.)

Why do callers of add_rte_to_flat_rtable() have to modify the rte's
perminfoindex themselves, instead of having the function do it for them?
That looks strange.  But also it's odd that flatten_unplanned_rtes
concatenates the two lists after all the perminfoindexes have been
modified, rather than doing both things (adding each RTEs perminfo to
the global list and offsetting the index) as we walk the list, in
flatten_rtes_walker.  It looks like these two actions are disconnected
from one another, but unless I misunderstand, in reality the opposite is
true.

I think the API of ConcatRTEPermissionInfoLists is a bit weird.  Why not
have the function return the resulting list instead, just like
list_append?  It is more verbose, but it seems easier to grok.

Two trivial changes attached.  (Maybe 0002 is not correct, if you're
also trying to reference finalrtepermlist; but in that case I think the
original may have been misleading as well.)

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 16 Nov 2022 12:44:02 +0100

On 2022-Nov-10, Alvaro Herrera wrote:

> I couldn't find any cross-check that

I hadn't looked hard enough.  This is already in GetRTEPermissionInfo().

> list_append?  It is more verbose, but it seems easier to grok.

Another point related to this.  I noticed that everyplace we do
ConcatRTEPermissionInfoLists, it is followed by list_append'ing the RT
list themselves.  This is strange.  Maybe that's the wrong way to look
at this, and instead we should have a function that does both things
together: pass both rtables and rtepermlists and smash them all
together.

I attach your 0001 again with a bunch of other fixups (I don't include
your 0002ff).  I've pushed this to see the CI results, and so far it's
looking good (hasn't finished yet though):
https://cirrus-ci.com/build/5126818977021952

I'll have a look at 0002 now.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

---

From: Amit Langote <[email-masked]>
Date: Mon, 21 Nov 2022 21:03:43 +0900

Hi Alvaro,

On Thu, Nov 10, 2022 at 8:58 PM Alvaro Herrera <[email-masked]> wrote:
> Hello
>
> I've been trying to understand what 0001 does.

Thanks a lot for taking a look.

> not processing one element multiple times and ending up with bogus offsets.

Could you please clarify what you mean by "an element" here?  Are you
are implying that any given relation, no matter how many times it
occurs in a query (possibly via view rule expansion), should end up
with only one RTEPermissionInfo node covering all its occurrences in
the combined/final rtepermlist, as a result of concatenation/merging
of rtepermlists of different Queries?  Versions of the patch prior to
v17 did it that way, but Tom didn't like that approach much [1], so I
switched to the current implementation where the merging of two
Queries' range tables using list_concat() is accompanied by the
merging of their rtepermlists, again, using list_concat().  So, if the
same table has multiple RTEs in a query, so will there be multiple
corresponding RTEPermissionInfos.

> coverage for such scenarios.)

ISTM the existing tests cover a good portion of the changes being made
here, but I guess I'm only saying that because I have spent a
non-trivial amount of time debugging the test failures across many
files over different versions of the patch, especially those that
involve views.

Do you think it might be better to add some new tests as part of this
patch than simply relying on the existing tests not failing?

> from one another, but unless I misunderstand, in reality the opposite is

OK, I have moved the step of updating perminfoindex into
add_rte_to_flat_rtable(), where it looks up the RTEPermissionInfo for
an RTE being added using GetRTEPermissionInfo() and lappend()'s it to
finalrtepermlist before updating the index.  For flatten_rtes_walker()
then to rely on that facility, I needed to make some changes to its
arguments to pass the correct Query node to pick the rtepermlist from.
Overall, setrefs.c changes now hopefully look saner than in the last
version.

As soon as I made that change, I noticed a bunch of ERRORs in
regression tests due to the checks in GetRTEPermissionInfo(), though
none that looked like live bugs.  Though I did find some others as I
was reworking the code to fix those errors, which I have fixed too.

> list_append?  It is more verbose, but it seems easier to grok.

Agreed, I have merged your delta patch into 0001.

> together: pass both rtables and rtepermlists and smash them all

OK, how does the attached 0002 look in that regard?  In it, I have
renamed ConcatRTEPermissionInfoLists() to CombineRangeTables() which
does all that.  Though, given the needs of rewriteRuleAction(), the
API of it may look a bit weird.  (Only posting it separately for the
ease of comparison.)

> https://cirrus-ci.com/build/5126818977021952

I have merged all.

While working on these changes, I realized that 0002 (the patch to
remove OLD/NEW RTEs from the stored view query's range table) was
going a bit too far by removing UpdateRangeTableOfViewParse()
altogether.  You may have noticed that a RTE_RELATION entry for the
view relation is needed anyway for permission checking, locking, etc.
and the patch was making the rewriter add one explicitly, whereas the
OLD RTE would be playing that role previously.  In the updated
version, I have decided to keep UpdateRangeTableOfViewParse() while
removing the code in it that adds the NEW RTE, which is totally
unnecessary.  Also removed the rewriter changes that were needed
previously.  Most of the previous version of the patch was a whole
bunch of regression test output changes, because the stored view
query's range table would be changed such that deparsed version of
those queries need no longer qualify output columns (only 1 entry in
the range table in those cases), though I didn't necessarily think
that that looked better.  In the new version, because the stored view
query contains the OLD entry, those qualifications stay, and so none
of the regression test changes are necessary anymore.  postgres_fdw
ones are unrelated and noted in the commit message.

[1] https://www.postgresql.org/message-id/3094251.1658955855%40sss.pgh.pa.us

---

From: Amit Langote <[email-masked]>
Date: Mon, 21 Nov 2022 21:46:03 +0900

On Mon, Nov 21, 2022 at 9:03 PM Amit Langote <[email-masked]> wrote:
> none that looked like live bugs.

I figured the no-live-bugs part warrants some clarification.  The
plan-time errors that I saw were caused in many cases by an RTE not
pointing into the correct list or having incorrect perminfoindex, most
or all of those cases involving views.  Passing a wrong perminfoindex
to the executor, though obviously not good and fixed in the latest
version, wasn't a problem in those cases, because none of those tests
would cause the executor to use the perminfoindex, such as by calling
GetRTEPermissionInfo().  I thought about that being problematic in
terms of our coverage of perminfoindex related code in the executor,
but don't really see how we could improve that situation as far as
views are concerned, because the executor is only concerned about
checking permissions for views and perminfoindex is irrelevant in that
path, because the RTEPermissionInfos are accessed directly from
es_rtepermlist.

---

From: Amit Langote <[email-masked]>
Date: Fri, 25 Nov 2022 20:28:37 +0900

> API of it may look a bit weird.  (Only posting it separately for the

Here's a revised version in which I've revised the code near the call
site of CombineRangeTables() in rewriteRuleAction() such that the
weirdness of that API in the last version becomes unnecessary.  When
doing those changes, I realized that we perhaps need some new tests to
exercise rewriteRuleAction(), especially to test the order of checking
permissions present in the (combined) range table of rewritten action
query, though I have not added them yet.

I've included a new patch (0002) that I've also posted at [1] for this
patch set to compile/work.

[1] https://www.postgresql.org/message-id/CA%2BHiwqHbv4xQd-yHx0LWA04AybA%2BGQPy66UJxt8m32gB6zCYQQ%40mail.gmail.com

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 29 Nov 2022 10:27:08 +0100

Thanks for the new version, in particular thank you for fixing the
annoyance with the CombineRangeTables API.

0002 was already pushed upstream, so we can forget about it.  I also
pushed the addition of missing_ok to build_attrmap_by_name{,_if_req}.
So this series needed a refresh, which is attached here, and tests are
running: https://cirrus-ci.com/build/4880219807416320

As for 0001+0003, here it is once again with a few fixups.  There are
two nontrivial changes here:

1. in get_rel_all_updated_cols (née GetRelAllUpdatedCols, which I
changed because it didn't match the naming style in inherits.c) you were
doing determining the relid to use in a roundabout way, then asserting
it is a value you already know:

-   use_relid = rel->top_parent_relids == NULL ? rel->relid :
-       bms_singleton_member(rel->top_parent_relids);
-   Assert(use_relid == root->parse->resultRelation);

Why not just use root->parse->resultRelation in the first place?
My 0002 does that.

2. my 0005 moves a block in add_rte_to_flat_rtable one level out:
there's no need to put it inside the rtekind == RTE_RELATION block, and
the comment in that block failed to mention that we copied the
RTEPermissionInfo; we can just let it work on the 'perminfoindex > 0'
condition.  Also, the comment is a bit misleading, and I changed it
some, but maybe not sufficiently: after add_rte_to_flat_rtable, the same
RTEPermissionInfo node will serve two RTEs: one in the Query struct,
whose perminfoindex corresponds to Query->rtepermlist; and the other in
PlannerGlobal->finalrtable, whose index corresponds to
PlannerGlobal->finalrtepermlist.  I was initially thinking that the old
RTE would result in a "corrupted" state, but that doesn't appear to be
the case.  (Also: I made it grab the RTEPermissionInfo using
rte->perminfoindex, not newrte->perminfoindex, because that seems
slightly bogus, even if they are identical because of the memcpy.)

The other changes are cosmetic.

I do not include here your 0004 and 0005.  (I think we can deal with
those separately later.)

---

From: Amit Langote <[email-masked]>
Date: Tue, 29 Nov 2022 22:37:56 +0900

Hi Alvaro,

Thanks for taking a look and all the fixup patches.  Was working on
that test I said we should add and then was spending some time
cleaning things up and breaking some things out into their patches,
mainly for the ease of review.

> annoyance with the CombineRangeTables API.

OK, now that you seem to think that looks good, I've merged it into
the main patch.

> pushed the addition of missing_ok to build_attrmap_by_name{,_if_req}.

Yeah, I thought that needed to be broken out and had done so in my
local repo.  Thanks for pushing that bit.

> Why not just use root->parse->resultRelation in the first place?

Facepalm, yes.

> My 0002 does that.

Merged.

> condition.

Yes, agree that's better.

>  Also, the comment is a bit misleading, and I changed it

Interesting point about two different RTEs (in different lists)
pointing to the same RTEPermissionInfo, also in different lists.
Maybe, we should have the following there so that the PlannedStmt's
contents don't point into the Query?

    newperminfo = copyObject(perminfo);

> The other changes are cosmetic.

Thanks, I've merged all.  I do wonder that it is only in PlannedStmt
that the list is called something that is not "rtepermlist", but I'm
fine with it if you prefer that.

> I do not include here your 0004 and 0005.  (I think we can deal with

OK, I have not attached them with this email either.

As I mentioned above, I've broken a couple of other changes out into
their own patches that I've put before the main patch.  0001 adds
ExecGetRootToChildMap().  I thought it would be better to write in the
commit message why the new map is necessary for the main patch.   0002
contains changes that has to do with changing how we access
checkAsUser in some foreign table planning/execution code sites.
Thought it might be better to describe it separately too.

0003 is the main patch into which I've merged both my patch that
invents CombineRangeTables() that I had posted separately before and
all of your fixups.  In it, you will see a new test case that I have
added in rules.sql to exercise the permission checking order stuff
that I had said I may have broken with this patch, especially the
hunks that change rewriteRuleAction().  That test would be broken with
v24, but not after the changes to add_rtes_to_flat_rtable() that I
made to address your review comment that blindly list_concat'ing
finalrtepermlist and Query's rtepermlist doesn't look very robust,
which it indeed wasn't [1].

[1] So, rewriteRuleAction(), with previous "wrong" versions of the
patch (~v26), would combine the original query's and action query's
rtepermlists in the "wrong" order, that is, not in the order in which
RTEs appear in the combined rtable.   But because
add_rtes_to_flat_rtable() now (v26~) adds perminfos into
finalrtepermlist in the RTE order using lappend(), that wrongness of
rewriteRuleAction() would be masked -- no execution-time failure of
the test.  Anyway, I've also fixed rewriteRuleAction() to be "correct"
in v27, so it is the least wrong version AFAIK ;).

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 29 Nov 2022 19:04:44 +0100

>     newperminfo = copyObject(perminfo);

Hmm, I suppose if we want a separate RTEPermissionInfo node, we should
instead do GetRTEPermissionInfo(rte) followed by
AddRTEPermissionInfo(newrte) and avoid the somewhat cowboy-ish coding
there.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

---

From: Amit Langote <[email-masked]>
Date: Wed, 30 Nov 2022 11:56:54 +0900

> AddRTEPermissionInfo(newrte) and avoid the somewhat cowboy-ish coding

OK, something like the attached?

---

From: Amit Langote <[email-masked]>
Date: Wed, 30 Nov 2022 15:45:53 +0900

> OK, something like the attached?

Thinking more about the patch I sent, which has this:

+       /* Get the existing one from this query's rtepermlist. */
        perminfo = GetRTEPermissionInfo(rtepermlist, newrte);
-       glob->finalrtepermlist = lappend(glob->finalrtepermlist, perminfo);
-       newrte->perminfoindex = list_length(glob->finalrtepermlist);
+
+       /*
+        * Add a new one to finalrtepermlist and copy the contents of the
+        * existing one into it.  Note that AddRTEPermissionInfo() also
+        * updates newrte->perminfoindex to point to newperminfo in
+        * finalrtepermlist.
+        */
+       newrte->perminfoindex = 0;  /* expected by AddRTEPermissionInfo() */
+       newperminfo = AddRTEPermissionInfo(&glob->finalrtepermlist, newrte);
+       memcpy(newperminfo, perminfo, sizeof(RTEPermissionInfo));

Note that simple memcpy'ing would lead to the selectedCols, etc.
bitmapsets being shared between the Query and the PlannedStmt, which
may be considered as not good.  But maybe that's fine, because the
same is true for RangeTblEntry members that do have substructure such
as the various Alias fields that are not reset?  Code paths that like
to keep a PlannedStmt to be decoupled from the corresponding Query,
such as plancache.c, do copy the former, so shared sub-structure in
the default case may be fine after all.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 30 Nov 2022 09:32:15 +0100

Hello

> mainly for the ease of review.

Right, excellent.  Thanks for this new version.  It looks pretty good to
me now.

> fine with it if you prefer that.

I was unsure about that one myself; I just changed it because that
struct uses camelCaseNaming, which the others do not, so it seemed fine
in the other places but not there.  As for changing "list" to "infos",
it seems to me we tend to avoid naming a list as "list", so.  (Maybe I
would change the others to be foo_rteperminfos.  Unless these naming
choices were already bikeshedded to its present form upthread and I
missed it?)

> commit message why the new map is necessary for the main patch.

I was thinking about this one and it seemed too closely tied to
ExecGetInsertedCols to be committed separately.  Notice how there is a
comment that mentions that function in your 0001, but that function
itself still uses ri_RootToPartitionMap, so before your 0003 the comment
is bogus.  And there's now quite some duplicity between
ri_RootToPartitionMap and ri_RootToChildMap, which I think it would be
better to reduce.  I mean, rather than add a new field it would be
better to repurpose the old one:

- ExecGetRootToChildMap should return TupleConversionMap *
- every place that accesses ri_RootToPartitionMap directly should be
  using ExecGetRootToChildMap() instead
- ExecGetRootToChildMap passes build_attrmap_by_name_if_req
  !resultRelInfo->ri_RelationDesc->rd_rel->relispartition
  as third argument to build_attrmap_by_name_if_req (rather than
  constant true), so that we keep the tuple compatibility checking we
  have there currently.

> 0002 contains changes that has to do with changing how we access

I'll get this one pushed soon, it seems good to me.  (I'll edit to not
use Oid as boolean.)