Thread: ExecRTCheckPerms() and many prunable partitions

Hi,

Last year in [1], I had briefly mentioned $subject.  I'm starting this
thread to propose a small patch to alleviate the inefficiency of that
case.

As also mentioned in [1], when running -Mprepared benchmarks
(plan_cache_mode = force_generic_plan) using partitioned tables,
ExecRTCheckPerms() tends to show up in the profile, especially with
large partition counts.  Granted it's lurking behind
AcquireExecutorLocks(), LockReleaseAll() et al, but still seems like a
problem we should do something about.

The problem is that it loops over the entire range table even though
only one or handful of those entries actually need their permissions
checked.  Most entries, especially those of partition child tables
have their requiredPerms set to 0, which David pointed out to me in
[2], so what ExecCheckRTPerms() does in their case is pure overhead.

An idea to fix that is to store the RT indexes of the entries that
have non-0 requiredPerms into a separate list or a bitmapset in
PlannedStmt.  I thought of two implementation ideas for how to set
that:

1. Put add_rtes_to_flat_rtable() in the charge of populating it:

@@ -324,12 +324,18 @@ add_rtes_to_flat_rtable(PlannerInfo *root, bool recursing)
     * flattened rangetable match up with their original indexes.  When
     * recursing, we only care about extracting relation RTEs.
     */
+   rti = 1;
    foreach(lc, root->parse->rtable)
    {
        RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);

        if (!recursing || rte->rtekind == RTE_RELATION)
+       {
            add_rte_to_flat_rtable(glob, rte);
+           if (rte->requiredPerms != 0)
+               glob->checkPermRels = bms_add_member(glob->checkPermRels, rti);
+       }
+       rti++
    }

2. Start populating checkPermRels in ParseState (parse_relation.c),
passing it along in Query through the rewriter and finally the
planner.

1 seems very simple, but appears to add overhead to what is likely a
oft-taken path.  Also, the newly added code would have to run as many
times as there are partitions, which sounds like a dealbreaker to me.

2 can seem a bit complex.  Given that the set is tracked in Query,
special care is needed to handle views and subqueries correctly,
because those features involve intricate manipulation of Query nodes
and their range tables.  However, most of that special care code
remains out of the busy paths.  Also, none of that code touches
partition/child RTEs, so unaffected by how many of them there are.

For now, I have implemented the idea 2 as the attached patch.  While
it passes make check-world, I am not fully confident yet that it
correctly handles all the cases involving views and subqueries.

So while still kind of PoC, will add this to July CF for keeping track.

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

[1] https://www.postgresql.org/message-id/CA+HiwqG7ZruBmmih3wPsBZ4s0H2EhywrnXEduckY5Hr3fWzPWA@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAApHDvqPzsMcKLRpmNpUW97PmaQDTmD7b2BayEPS5AN4LY-0bA%40mail.gmail.com

On Thu, 1 Jul 2021 at 01:34, Amit Langote <amitlangote09@gmail.com> wrote:
> For now, I have implemented the idea 2 as the attached patch.

I only just had a fleeting glance at the patch. Aren't you
accidentally missing the 0th RTE here?

+ while ((rti = bms_next_member(checkPermRels, rti)) > 0)
  {
- RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
+ RangeTblEntry *rte = (RangeTblEntry *) list_nth(rangeTable, rti - 1);

I'd have expected >= 0 rather than > 0.

David

On Thu, 1 Jul 2021 at 02:58, Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Wed, Jun 30, 2021 at 23:34 David Rowley <dgrowleyml@gmail.com> wrote:
>> + while ((rti = bms_next_member(checkPermRels, rti)) > 0)
>>   {
>> - RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
>> + RangeTblEntry *rte = (RangeTblEntry *) list_nth(rangeTable, rti - 1);
>>
>> I'd have expected >= 0 rather than > 0.
>
> Hmm, a valid RT index cannot be 0, so that seems fine to me.  Note that RT indexes are added as-is to that bitmapset,
notafter subtracting 1.
 

Oh, you're right. My mistake.

David

Amit Langote <amitlangote09@gmail.com> writes:
> The problem is that it loops over the entire range table even though
> only one or handful of those entries actually need their permissions
> checked.  Most entries, especially those of partition child tables
> have their requiredPerms set to 0, which David pointed out to me in
> [2], so what ExecCheckRTPerms() does in their case is pure overhead.

> An idea to fix that is to store the RT indexes of the entries that
> have non-0 requiredPerms into a separate list or a bitmapset in
> PlannedStmt.

I think perhaps we ought to be more ambitious than that, and consider
separating the list of permissions-to-check from the rtable entirely.
Your patch hardly qualifies as non-invasive, plus it seems to invite
errors of omission, while if we changed the data structure altogether
then the compiler would help find any not-updated code.

But the main reason that this strikes me as possibly a good idea
is that I was just taking another look at the complaint in [1],
where I wrote

>> I think it's impossible to avoid less-than-O(N^2) growth on this sort
>> of case.  For example, the v2 subquery initially has RTEs for v2 itself
>> plus v1.  When we flatten v1 into v2, v2 acquires the RTEs from v1,
>> namely v1 itself plus foo.  Similarly, once vK-1 is pulled up into vK,
>> there are going to be order-of-K entries in vK's rtable, and that stacking
>> makes for O(N^2) work overall just in manipulating the rtable.
>>
>> We can't get rid of these rtable entries altogether, since all of them
>> represent table privilege checks that the executor will need to do.

Perhaps, if we separated the rtable from the required-permissions data
structure, then we could avoid pulling up otherwise-useless RTEs when
flattening a view (or even better, not make the extra RTEs in the
first place??), and thus possibly avoid that exponential planning-time
growth for nested views.

Or maybe not.  But I think we should take a hard look at whether
separating these data structures could solve both of these problems
at once.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/797aff54-b49b-4914-9ff9-aa42564a4d7d%40www.fastmail.com

On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > The problem is that it loops over the entire range table even though
> > only one or handful of those entries actually need their permissions
> > checked.  Most entries, especially those of partition child tables
> > have their requiredPerms set to 0, which David pointed out to me in
> > [2], so what ExecCheckRTPerms() does in their case is pure overhead.
>
> > An idea to fix that is to store the RT indexes of the entries that
> > have non-0 requiredPerms into a separate list or a bitmapset in
> > PlannedStmt.
>
> I think perhaps we ought to be more ambitious than that, and consider
> separating the list of permissions-to-check from the rtable entirely.
> Your patch hardly qualifies as non-invasive, plus it seems to invite
> errors of omission, while if we changed the data structure altogether
> then the compiler would help find any not-updated code.
>
> But the main reason that this strikes me as possibly a good idea
> is that I was just taking another look at the complaint in [1],
> where I wrote
>
> >> I think it's impossible to avoid less-than-O(N^2) growth on this sort
> >> of case.  For example, the v2 subquery initially has RTEs for v2 itself
> >> plus v1.  When we flatten v1 into v2, v2 acquires the RTEs from v1,
> >> namely v1 itself plus foo.  Similarly, once vK-1 is pulled up into vK,
> >> there are going to be order-of-K entries in vK's rtable, and that stacking
> >> makes for O(N^2) work overall just in manipulating the rtable.
> >>
> >> We can't get rid of these rtable entries altogether, since all of them
> >> represent table privilege checks that the executor will need to do.
>
> Perhaps, if we separated the rtable from the required-permissions data
> structure, then we could avoid pulling up otherwise-useless RTEs when
> flattening a view (or even better, not make the extra RTEs in the
> first place??), and thus possibly avoid that exponential planning-time
> growth for nested views.
>
> Or maybe not.  But I think we should take a hard look at whether
> separating these data structures could solve both of these problems
> at once.

Ah, okay.  I'll think about decoupling the permission checking stuff
from the range table data structure.

Thanks for the feedback.

I'll mark the CF entry as WoA, unless you'd rather I just mark it RwF.

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

On Fri, 2 Jul 2021 at 12:41, Amit Langote <amitlangote09@gmail.com> wrote:
> I'll mark the CF entry as WoA, unless you'd rather I just mark it RwF.

I've set it to waiting on author. It was still set to needs review.

If you think you'll not get time to write the patch during this CF,
feel free to bump it out.

David

On Wed, Jul 7, 2021 at 1:41 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Fri, 2 Jul 2021 at 12:41, Amit Langote <amitlangote09@gmail.com> wrote:
> > I'll mark the CF entry as WoA, unless you'd rather I just mark it RwF.
>
> I've set it to waiting on author. It was still set to needs review.

Sorry it slipped my mind to do that and thanks.

> If you think you'll not get time to write the patch during this CF,
> feel free to bump it out.

I will try to post an update next week if not later this week,
hopefully with an updated patch.

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

On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I think perhaps we ought to be more ambitious than that, and consider
> > separating the list of permissions-to-check from the rtable entirely.
> > Your patch hardly qualifies as non-invasive, plus it seems to invite
> > errors of omission, while if we changed the data structure altogether
> > then the compiler would help find any not-updated code.
> >
> > But the main reason that this strikes me as possibly a good idea
> > is that I was just taking another look at the complaint in [1],
> > where I wrote
> >
> > >> I think it's impossible to avoid less-than-O(N^2) growth on this sort
> > >> of case.  For example, the v2 subquery initially has RTEs for v2 itself
> > >> plus v1.  When we flatten v1 into v2, v2 acquires the RTEs from v1,
> > >> namely v1 itself plus foo.  Similarly, once vK-1 is pulled up into vK,
> > >> there are going to be order-of-K entries in vK's rtable, and that stacking
> > >> makes for O(N^2) work overall just in manipulating the rtable.
> > >>
> > >> We can't get rid of these rtable entries altogether, since all of them
> > >> represent table privilege checks that the executor will need to do.
> >
> > Perhaps, if we separated the rtable from the required-permissions data
> > structure, then we could avoid pulling up otherwise-useless RTEs when
> > flattening a view (or even better, not make the extra RTEs in the
> > first place??), and thus possibly avoid that exponential planning-time
> > growth for nested views.
> >
> > Or maybe not.  But I think we should take a hard look at whether
> > separating these data structures could solve both of these problems
> > at once.
>
> Ah, okay.  I'll think about decoupling the permission checking stuff
> from the range table data structure.

I have finished with the attached.  Sorry about the delay.

Think I've managed to get the first part done -- getting the
permission-checking info out of the range table -- but have not
seriously attempted the second -- doing away with the OLD/NEW range
table entries in the view/rule action queries, assuming that is what
you meant in the quoted.

One design point I think might need reconsidering is that the list of
the new RelPermissionInfo nodes that holds the permission-checking
info for relations has to be looked up with a linear search using the
relation OID, whereas it was basically free before if a particular of
code had the RTE handy.  Maybe I need to check if the overhead of that
is noticeable in some cases.

As there's not much time left in this CF, I've bumped the entry to the next one.

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

On Thu, Jul 29, 2021 at 5:40 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Perhaps, if we separated the rtable from the required-permissions data
> > > structure, then we could avoid pulling up otherwise-useless RTEs when
> > > flattening a view (or even better, not make the extra RTEs in the
> > > first place??), and thus possibly avoid that exponential planning-time
> > > growth for nested views.
>
> Think I've managed to get the first part done -- getting the
> permission-checking info out of the range table -- but have not
> seriously attempted the second -- doing away with the OLD/NEW range
> table entries in the view/rule action queries, assuming that is what
> you meant in the quoted.

I took a stab at the 2nd part, implemented in the attached 0002.

The patch removes UpdateRangeTableOfViewParse() which would add the
dummy OLD/NEW entries to a view rule's action query's rtable, citing
these reasons:

- * These extra RT entries are not actually used in the query,
- * except for run-time locking and permission checking.

0001 makes them unnecessary for permission checking.  Though, a
RELATION-kind RTE still be must be present in the rtable for run-time
locking, so I adjusted ApplyRetrieveRule() as follows:

@@ -1803,16 +1804,26 @@ ApplyRetrieveRule(Query *parsetree,
     * original RTE to a subquery RTE.
     */
    rte = rt_fetch(rt_index, parsetree->rtable);
+   subquery_rte = rte;

-   rte->rtekind = RTE_SUBQUERY;
-   rte->subquery = rule_action;
-   rte->security_barrier = RelationIsSecurityView(relation);
+   /*
+    * Before modifying, store a copy of itself so as to serve as the entry
+    * 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.
+    */
+   rte = copyObject(rte);
+   parsetree->rtable = lappend(parsetree->rtable, rte);
+
+   subquery_rte->rtekind = RTE_SUBQUERY;
+   subquery_rte->subquery = rule_action;
+   subquery_rte->security_barrier = RelationIsSecurityView(relation);
    /* Clear fields that should not be set in a subquery RTE */
-   rte->relid = InvalidOid;
-   rte->relkind = 0;
-   rte->rellockmode = 0;
-   rte->tablesample = NULL;
-   rte->inh = false;           /* must not be set for a subquery */
+   subquery_rte->relid = InvalidOid;
+   subquery_rte->relkind = 0;
+   subquery_rte->rellockmode = 0;
+   subquery_rte->tablesample = NULL;
+   subquery_rte->inh = false;          /* must not be set for a subquery */

    return parsetree;
 }

Outputs for a bunch of regression tests needed to be adjusted to
account for that pg_get_viewdef() no longer qualifies view column
names in the deparsed queries, that is, if they reference only a
single relation.   Previously, those dummy OLD/NEW entries tricked
make_ruledef(), get_query_def() et al into setting
deparse_context.varprefix to true.  contrib/postgre_fdw test output
likewise needed adjustment due to its deparse code being impacted by
those dummy entries no longer being present, I believe.

I haven't yet checked how this further improves the performance for
the case discussed at [1] that prompted this.

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

[1] https://www.postgresql.org/message-id/flat/797aff54-b49b-4914-9ff9-aa42564a4d7d%40www.fastmail.com

On Fri, Aug 20, 2021 at 10:46 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jul 29, 2021 at 5:40 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > > Perhaps, if we separated the rtable from the required-permissions data
> > > > structure, then we could avoid pulling up otherwise-useless RTEs when
> > > > flattening a view (or even better, not make the extra RTEs in the
> > > > first place??), and thus possibly avoid that exponential planning-time
> > > > growth for nested views.
> >
> > Think I've managed to get the first part done -- getting the
> > permission-checking info out of the range table -- but have not
> > seriously attempted the second -- doing away with the OLD/NEW range
> > table entries in the view/rule action queries, assuming that is what
> > you meant in the quoted.
>
> I took a stab at the 2nd part, implemented in the attached 0002.
>
> The patch removes UpdateRangeTableOfViewParse() which would add the
> dummy OLD/NEW entries to a view rule's action query's rtable
>
> I haven't yet checked how this further improves the performance for
> the case discussed at [1] that prompted this.
>
> [1] https://www.postgresql.org/message-id/flat/797aff54-b49b-4914-9ff9-aa42564a4d7d%40www.fastmail.com

I checked the time required to do explain select * from v512 (worst
case), using the setup described at the above link and I get the
following numbers:

HEAD: 119.774 ms
0001  : 129.802 ms
0002  : 109.456 ms

So it appears that applying only 0001 makes things a bit worse for
this case.  That seems to have to do with the following addition in
pull_up_simple_subquery():

@@ -1131,6 +1131,9 @@ pull_up_simple_subquery(PlannerInfo *root, Node
*jtnode, RangeTblEntry *rte,
     */
    parse->rtable = list_concat(parse->rtable, subquery->rtable);

+   parse->relpermlist = MergeRelPermissionInfos(parse->relpermlist,
+                                                subquery->relpermlist);
+

What it does is pull up the RelPermissionInfo nodes in the subquery
being pulled up into the parent query and it's not a simple
list_concat(), because I decided that it's better to de-duplicate the
entries for a given relation OID even across subqueries.

Things get better than HEAD with 0002, because less work needs to be
done in the rewriter when copying the subqueries into the main query,
especially the range table, which only has 1 entry now, not 3 per
view.

Attached updated patches.  I wrote a longer commit message for 0002 this time.

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

Got this warning:

/pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c: In function 'GetResultRelCheckAsUser':
/pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c:1898:7: warning: unused variable 'result' [-Wunused-variable]
  Oid  result;
       ^~~~~~

I think the idea that GetRelPermissionInfo always has to scan the
complete list by OID is a nonstarter.  Maybe it would be possible to
store the list index of the PermissionInfo element in the RelOptInfo or
the RTE?  Maybe use special negative values if unknown (it knows to
search the first time) or known non-existant (probably a coding error
condition, maybe not necessary to have this)

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

Thanks Alvaro for taking a look at this.

On Tue, Sep 7, 2021 at 4:35 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Got this warning:
>
> /pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c: In function 'GetResultRelCheckAsUser':
> /pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c:1898:7: warning: unused variable 'result'
[-Wunused-variable]
>   Oid  result;
>        ^~~~~~

Fixed.

> I think the idea that GetRelPermissionInfo always has to scan the
> complete list by OID is a nonstarter.  Maybe it would be possible to
> store the list index of the PermissionInfo element in the RelOptInfo or
> the RTE?  Maybe use special negative values if unknown (it knows to
> search the first time) or known non-existant (probably a coding error
> condition, maybe not necessary to have this)

I implemented this by adding an Index field in RangeTblEntry, because
GetRelPermissionInfo() is used in all phases of query processing and
only RTEs exist from start to end.  I did have to spend some time
getting that approach right (get `make check` to pass!), especially to
ensure that the indexes remain in sync during the merging of
RelPermissionInfo across subqueries.  The comments I wrote around
GetRelPermissionInfo(), MergeRelPermissionInfos() functions should
hopefully make things clear.  Though, I do have a slightly uneasy
feeling around the fact that RTEs now store information that is
computed using some non-trivial logic, whereas most other fields are
simple catalog state or trivial details extracted from how the query
is spelled out by the user.

I also noticed that setrefs.c: add_rtes_to_flat_rtable() was still
doing things -- adding dead subquery RTEs and any RTEs referenced in
the underlying subquery to flat rtable -- that the new approach of
permissions handling makes unnecessary.  I fixed that oversight in the
updated patch.  A benefit from that simplification is that there is
now a single loop over rtable in that function rather than two that
were needed before.

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

On Fri, Sep 10, 2021 at 12:22 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Sep 7, 2021 at 4:35 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > I think the idea that GetRelPermissionInfo always has to scan the
> > complete list by OID is a nonstarter.  Maybe it would be possible to
> > store the list index of the PermissionInfo element in the RelOptInfo or
> > the RTE?  Maybe use special negative values if unknown (it knows to
> > search the first time) or known non-existant (probably a coding error
> > condition, maybe not necessary to have this)
>
> I implemented this by adding an Index field in RangeTblEntry, because
> GetRelPermissionInfo() is used in all phases of query processing and
> only RTEs exist from start to end.  I did have to spend some time
> getting that approach right (get `make check` to pass!), especially to
> ensure that the indexes remain in sync during the merging of
> RelPermissionInfo across subqueries.  The comments I wrote around
> GetRelPermissionInfo(), MergeRelPermissionInfos() functions should
> hopefully make things clear.  Though, I do have a slightly uneasy
> feeling around the fact that RTEs now store information that is
> computed using some non-trivial logic, whereas most other fields are
> simple catalog state or trivial details extracted from how the query
> is spelled out by the user.
>
> I also noticed that setrefs.c: add_rtes_to_flat_rtable() was still
> doing things -- adding dead subquery RTEs and any RTEs referenced in
> the underlying subquery to flat rtable -- that the new approach of
> permissions handling makes unnecessary.  I fixed that oversight in the
> updated patch.  A benefit from that simplification is that there is
> now a single loop over rtable in that function rather than two that
> were needed before.

Patch 0002 needed a rebase, because a conflicting change to
expected/rules.out has since been committed.

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

Hi,

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:

Latest failure: https://cirrus-ci.com/task/4686414276198400 and
https://api.cirrus-ci.com/v1/artifact/task/4686414276198400/regress_diffs/src/test/regress/regression.diffs

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/xml.out /tmp/cirrus-ci-build/src/test/regress/results/xml.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/xml.out    2022-01-12 05:24:02.795477001 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/xml.out    2022-01-12 05:28:20.329086031 +0000
@@ -603,12 +603,12 @@
 CREATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as text);
 SELECT table_name, view_definition FROM information_schema.views
   WHERE table_name LIKE 'xmlview%' ORDER BY 1;
- table_name |                                                  view_definition

-------------+-------------------------------------------------------------------------------------------------------------------
+ table_name |                                              view_definition

+------------+------------------------------------------------------------------------------------------------------------
  xmlview1   |  SELECT xmlcomment('test'::text) AS xmlcomment;
  xmlview2   |  SELECT XMLCONCAT('hello'::xml, 'you'::xml) AS "xmlconcat";
  xmlview3   |  SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS
"xmlelement";
- xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(emp.name AS name, emp.age AS age, emp.salary AS pay)) AS
"xmlelement"+
+ xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement"
+
 
             |    FROM emp;
  xmlview5   |  SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
  xmlview6   |  SELECT XMLPI(NAME foo, 'bar'::text) AS "xmlpi";
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/compression.out
/tmp/cirrus-ci-build/src/test/regress/results/compression.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/compression.out    2022-01-12 05:24:02.739471690 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/compression.out    2022-01-12 05:28:23.537403929 +0000
@@ -187,7 +187,7 @@
 --------+------+-----------+----------+---------+----------+-------------+--------------+-------------
  x      | text |           |          |         | extended |             |              |
 View definition:
- SELECT cmdata1.f1 AS x
+ SELECT f1 AS x
    FROM cmdata1;

 SELECT pg_column_compression(f1) FROM cmdata1;
@@ -274,7 +274,7 @@
 --------+------+-----------+----------+---------+----------+-------------+--------------+-------------
  x      | text |           |          |         | extended | lz4         |              |
 View definition:
- SELECT cmdata1.f1 AS x
+ SELECT f1 AS x
    FROM cmdata1;

Could you send a rebased patch?  In the meantime I'll switch the cf entry to
Waiting on Author.

On Thu, Jan 13, 2022 at 12:10 PM Julien Rouhaud <rjuju123@gmail.com> 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:

I had noticed that too and was meaning to send a new version.  Thanks
for the reminder.

> Latest failure: https://cirrus-ci.com/task/4686414276198400 and
> https://api.cirrus-ci.com/v1/artifact/task/4686414276198400/regress_diffs/src/test/regress/regression.diffs
>
> diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/xml.out
/tmp/cirrus-ci-build/src/test/regress/results/xml.out
> --- /tmp/cirrus-ci-build/src/test/regress/expected/xml.out      2022-01-12 05:24:02.795477001 +0000
> +++ /tmp/cirrus-ci-build/src/test/regress/results/xml.out       2022-01-12 05:28:20.329086031 +0000
> @@ -603,12 +603,12 @@
>  CREATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as text);
>  SELECT table_name, view_definition FROM information_schema.views
>    WHERE table_name LIKE 'xmlview%' ORDER BY 1;
> - table_name |                                                  view_definition
>
-------------+-------------------------------------------------------------------------------------------------------------------
> + table_name |                                              view_definition
>
+------------+------------------------------------------------------------------------------------------------------------
>   xmlview1   |  SELECT xmlcomment('test'::text) AS xmlcomment;
>   xmlview2   |  SELECT XMLCONCAT('hello'::xml, 'you'::xml) AS "xmlconcat";
>   xmlview3   |  SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS
"xmlelement";
> - xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(emp.name AS name, emp.age AS age, emp.salary AS pay)) AS
"xmlelement"+
> + xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement"
  +
 
>              |    FROM emp;
>   xmlview5   |  SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
>   xmlview6   |  SELECT XMLPI(NAME foo, 'bar'::text) AS "xmlpi";
> diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/compression.out
/tmp/cirrus-ci-build/src/test/regress/results/compression.out
> --- /tmp/cirrus-ci-build/src/test/regress/expected/compression.out      2022-01-12 05:24:02.739471690 +0000
> +++ /tmp/cirrus-ci-build/src/test/regress/results/compression.out       2022-01-12 05:28:23.537403929 +0000
> @@ -187,7 +187,7 @@
>  --------+------+-----------+----------+---------+----------+-------------+--------------+-------------
>   x      | text |           |          |         | extended |             |              |
>  View definition:
> - SELECT cmdata1.f1 AS x
> + SELECT f1 AS x
>     FROM cmdata1;
>
>  SELECT pg_column_compression(f1) FROM cmdata1;
> @@ -274,7 +274,7 @@
>  --------+------+-----------+----------+---------+----------+-------------+--------------+-------------
>   x      | text |           |          |         | extended | lz4         |              |
>  View definition:
> - SELECT cmdata1.f1 AS x
> + SELECT f1 AS x
>     FROM cmdata1;
>
> 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.

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

On Thu, Jan 13, 2022 at 3:39 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jan 13, 2022 at 12:10 PM Julien Rouhaud <rjuju123@gmail.com> 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

On Tue, Jan 18, 2022 at 12:42 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> Hi,
> For patch 02, in the description:

Thanks for looking.

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

Fixed.

> For patch 01, GetRelPermissionInfo():
>
> +       return perminfo;
> +   }
> +   else
>
> keyword 'else' is not needed - the else block can be left-indented.

OK, done.

Also needed fixes when rebasing.

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

On Mon, Mar 14, 2022 at 4:36 PM Amit Langote <amitlangote09@gmail.com> wrote:
> 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.

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

On 2022-Mar-23, Amit Langote wrote:

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

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)

On Wed, 23 Mar 2022 at 20:03, Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Mon, Mar 14, 2022 at 4:36 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > Also needed fixes when rebasing.
>
> Needed another rebase.

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

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

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

On Wed, 6 Apr 2022 at 02:27, Greg Stark <stark@mit.edu> wrote:
>
> 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.

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

On Wed, Apr 6, 2022 at 5:22 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Wed, 6 Apr 2022 at 02:27, Greg Stark <stark@mit.edu> wrote:
> >
> > 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.
>
> 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.

That is correct.

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

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

Rebased to keep the cfbot green for now.

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

On Mon, Apr 11, 2022 at 2:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Rebased to keep the cfbot green for now.

And again to fix the rules.out conflicts.

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

Rebased over 964d01ae90.


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

On Wed, Jul 13, 2022 at 5:00 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Rebased over 964d01ae90.

Rebased over 2d04277121f.


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

Amit Langote <amitlangote09@gmail.com> writes:
> [ v16 patches ]

I took a quick look at this ...

I think that the notion behind MergeRelPermissionInfos, ie that
a single RelPermissionInfo can represent *all* the checks for
a given table OID, is fundamentally wrong.  For example, when
merging a view into an outer query that references a table
also used by the view, the checkAsUser fields might be different,
and the permissions to check might be different, and the columns
those permissions need to hold for might be different.  Blindly
bms_union'ing the column sets will lead to requiring far more
permissions than the query should require.  Conversely, this
approach could lead to allowing cases we should reject, if you
happen to "merge" checkAsUser in a way that ends in checking as a
higher-privilege user than should be checked.

I'm inclined to think that you should abandon the idea of
merging RelPermissionInfos at all.  It can only buy us much
in the case of self-joins, which ought to be rare.  It'd
be better to just say "there is one RelPermissionInfo for
each RTE requiring any sort of permissions check".  Either
that or you need to complicate RelPermissionInfo a lot, but
I don't see the advantage.

It'd likely be better to rename ExecutorCheckPerms_hook,
say to ExecCheckPermissions_hook given the rename of
ExecCheckRTPerms.  As it stands, it *looks* like the API
of that hook has not changed, when it has.  Better to
break calling code visibly than to make people debug their
way to an understanding that the List contents are no longer
what they expected.  A different idea could be to pass both
the rangetable and relpermlist, again making the API break obvious
(and who's to say a hook might not still want the rangetable?)

In parsenodes.h:
+    List       *relpermlist;    /* list of RTEPermissionInfo nodes for
+                                 * the RTE_RELATION entries in rtable */

I find this comment not very future-proof, if indeed it's strictly
correct even today.  Maybe better "list of RelPermissionInfo nodes for
rangetable entries having perminfoindex > 0".  Likewise for the comment
in RangeTableEntry: there's no compelling reason to assume that all and
only RELATION RTEs will have RelPermissionInfo.  Even if that remains
true at parse time it's falsified during planning.

Also note typo in node name: that comment is the only reference to
"RTEPermissionInfo" AFAICS.  Although, given the redefinition I
suggest above, arguably "RTEPermissionInfo" is the better name?

I'm confused as to why RelPermissionInfo.inh exists.  It doesn't
seem to me that permissions checking should care about child rels.

Why did you add checkAsUser to ForeignScan (and not any other scan
plan nodes)?  At best that's pretty asymmetric, but it seems mighty
bogus: under what circumstances would an FDW need to know that but
not any of the other RelPermissionInfo fields?  This seems to
indicate that someplace we should work harder at making the
RelPermissionInfo list available to FDWs.  (CustomScan providers
might have similar issues, btw.)

I've not looked at much of the actual code, just the .h file changes.
Haven't studied 0002 either.

            regards, tom lane

... One more thing: maybe we should rethink where to put
extraUpdatedCols.  Between the facts that it's not used for
actual permissions checks, and that it's calculated by the
rewriter not parser, it doesn't seem like it really belongs
in RelPermissionInfo.  Should we keep it in RangeTblEntry?
Should it go somewhere else entirely?  I'm just speculating,
but now is a good time to think about it.

            regards, tom lane

On Thu, Jul 28, 2022 at 6:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > [ v16 patches ]
>
> I took a quick look at this ...

Thanks for the review and sorry about the delay.

> I think that the notion behind MergeRelPermissionInfos, ie that
> a single RelPermissionInfo can represent *all* the checks for
> a given table OID, is fundamentally wrong.  For example, when
> merging a view into an outer query that references a table
> also used by the view, the checkAsUser fields might be different,
> and the permissions to check might be different, and the columns
> those permissions need to hold for might be different.  Blindly
> bms_union'ing the column sets will lead to requiring far more
> permissions than the query should require.  Conversely, this
> approach could lead to allowing cases we should reject, if you
> happen to "merge" checkAsUser in a way that ends in checking as a
> higher-privilege user than should be checked.
>
> I'm inclined to think that you should abandon the idea of
> merging RelPermissionInfos at all.  It can only buy us much
> in the case of self-joins, which ought to be rare.  It'd
> be better to just say "there is one RelPermissionInfo for
> each RTE requiring any sort of permissions check".  Either
> that or you need to complicate RelPermissionInfo a lot, but
> I don't see the advantage.

OK, I agree that the complexity of sharing a RelPermissionInfo between
RTEs far exceeds any performance benefit to be had from it.

I have changed things so that there's one RelPermissionInfo for every
RTE_RELATION entry in the range table, except those that the planner
adds when expanding inheritance.

> It'd likely be better to rename ExecutorCheckPerms_hook,
> say to ExecCheckPermissions_hook given the rename of
> ExecCheckRTPerms.  As it stands, it *looks* like the API
> of that hook has not changed, when it has.  Better to
> break calling code visibly than to make people debug their
> way to an understanding that the List contents are no longer
> what they expected.  A different idea could be to pass both
> the rangetable and relpermlist, again making the API break obvious
> (and who's to say a hook might not still want the rangetable?)

I agree it'd be better to break the API more explicitly.  Actually, I
decided to adopt both of these suggestions: renamed the hook and kept
the rangeTable parameter.

> In parsenodes.h:
> +    List       *relpermlist;    /* list of RTEPermissionInfo nodes for
> +                                 * the RTE_RELATION entries in rtable */
>
> I find this comment not very future-proof, if indeed it's strictly
> correct even today.  Maybe better "list of RelPermissionInfo nodes for
> rangetable entries having perminfoindex > 0".  Likewise for the comment
> in RangeTableEntry: there's no compelling reason to assume that all and
> only RELATION RTEs will have RelPermissionInfo.  Even if that remains
> true at parse time it's falsified during planning.

Ah right, inheritance children's RTE_RELATION entries don't have one.
I've fixed the comment.

> Also note typo in node name: that comment is the only reference to
> "RTEPermissionInfo" AFAICS.  Although, given the redefinition I
> suggest above, arguably "RTEPermissionInfo" is the better name?

Agreed.  I've renamed RelPermissionInfo to RTEPermissionInfo and
relpermlist to rtepermlist.

> I'm confused as to why RelPermissionInfo.inh exists.  It doesn't
> seem to me that permissions checking should care about child rels.

I had to do this for contrib/sepgsql, sepgsql_dml_privileges() has this:

        /*
         * If this RangeTblEntry is also supposed to reference inherited
         * tables, we need to check security label of the child tables. So, we
         * expand rte->relid into list of OIDs of inheritance hierarchy, then
         * checker routine will be invoked for each relations.
         */
        if (!rte->inh)
            tableIds = list_make1_oid(rte->relid);
        else
            tableIds = find_all_inheritors(rte->relid, NoLock, NULL);

> Why did you add checkAsUser to ForeignScan (and not any other scan
> plan nodes)?  At best that's pretty asymmetric, but it seems mighty
> bogus: under what circumstances would an FDW need to know that but
> not any of the other RelPermissionInfo fields?  This seems to
> indicate that someplace we should work harder at making the
> RelPermissionInfo list available to FDWs.  (CustomScan providers
> might have similar issues, btw.)

I think I had tried doing what you are suggesting -- getting the
checkAsUser from a RelPermissionInfo rather than putting that in
ForeignScan -- though we can't do it, because we need the userid for
child foreign table relations, for which we don't create a
RelPermissionInfo.  ForeignScan nodes for child relations don't store
their root parent's RT index, so we can't get the checkAsUser using
the root parent's RelPermissionInfo, like I could do for child foreign
table "result" relations using ResultRelInfo.ri_RootResultRelInfo.

As to why an FDW may not need to know any of the other
RelPermissionInfo fields, IIUC, ExecCheckPermissions() would have done
everything that ought to be done *locally* using that information.
Whatever the remote side needs to know wrt access permission checking
should have been put in fdw_private, no?

On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> ... One more thing: maybe we should rethink where to put
> extraUpdatedCols.  Between the facts that it's not used for
> actual permissions checks, and that it's calculated by the
> rewriter not parser, it doesn't seem like it really belongs
> in RelPermissionInfo.  Should we keep it in RangeTblEntry?
> Should it go somewhere else entirely?  I'm just speculating,
> but now is a good time to think about it.

Indeed, extraUpdatedCols doesn't really seem to belong in
RelPermissionInfo, so I have left it in RangeTblEntry.

Attached updated patches.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Hi,

On 2022-09-07 18:23:06 +0900, Amit Langote wrote:
> Attached updated patches.

Thanks to Justin's recent patch (89d16b63527) to add
-DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
to the FreeBSD ci task we now see the following:

https://cirrus-ci.com/task/4772259058417664
https://api.cirrus-ci.com/v1/artifact/task/4772259058417664/testrun/build/testrun/main/regress/regression.diffs

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/updatable_views.out
/tmp/cirrus-ci-build/build/testrun/main/regress/results/updatable_views.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/updatable_views.out    2022-10-02 10:37:08.888945000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/main/regress/results/updatable_views.out    2022-10-02 10:40:26.947887000 +0000
@@ -1727,14 +1727,16 @@
 (4 rows)

 UPDATE base_tbl SET id = 2000 WHERE id = 2;
+WARNING:  outfuncs/readfuncs failed to produce an equal rewritten parse tree
 UPDATE rw_view1 SET id = 3000 WHERE id = 3;
+WARNING:  outfuncs/readfuncs failed to produce an equal rewritten parse tree
 SELECT * FROM base_tbl;
   id  | idplus1
 ------+---------
     1 |       2
     4 |       5
- 2000 |    2001
- 3000 |    3001
+ 2000 |       3
+ 3000 |       4
 (4 rows)

 DROP TABLE base_tbl CASCADE;

and many more.

Greetings,

Andres Freund

Hi,

On Mon, Oct 3, 2022 at 2:10 AM Andres Freund <andres@anarazel.de> wrote:
> On 2022-09-07 18:23:06 +0900, Amit Langote wrote:
> > Attached updated patches.
>
> Thanks to Justin's recent patch (89d16b63527) to add
> -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
> to the FreeBSD ci task we now see the following:
>
> https://cirrus-ci.com/task/4772259058417664
> https://api.cirrus-ci.com/v1/artifact/task/4772259058417664/testrun/build/testrun/main/regress/regression.diffs
>
> diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/updatable_views.out
/tmp/cirrus-ci-build/build/testrun/main/regress/results/updatable_views.out
> --- /tmp/cirrus-ci-build/src/test/regress/expected/updatable_views.out  2022-10-02 10:37:08.888945000 +0000
> +++ /tmp/cirrus-ci-build/build/testrun/main/regress/results/updatable_views.out 2022-10-02 10:40:26.947887000 +0000
> @@ -1727,14 +1727,16 @@
>  (4 rows)
>
>  UPDATE base_tbl SET id = 2000 WHERE id = 2;
> +WARNING:  outfuncs/readfuncs failed to produce an equal rewritten parse tree
>  UPDATE rw_view1 SET id = 3000 WHERE id = 3;
> +WARNING:  outfuncs/readfuncs failed to produce an equal rewritten parse tree
>  SELECT * FROM base_tbl;
>    id  | idplus1
>  ------+---------
>      1 |       2
>      4 |       5
> - 2000 |    2001
> - 3000 |    3001
> + 2000 |       3
> + 3000 |       4
>  (4 rows)
>
>  DROP TABLE base_tbl CASCADE;
>
> and many more.

Thanks for the heads up.  Grateful for those new -D flags.

Turns out I had forgotten to update out/readRangeTblEntry() after
bringing extraUpdatedCols back into RangeTblEntry per Tom's comment.

Fixed in the attached.

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

On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> ... One more thing: maybe we should rethink where to put
> extraUpdatedCols.  Between the facts that it's not used for
> actual permissions checks, and that it's calculated by the
> rewriter not parser, it doesn't seem like it really belongs
> in RelPermissionInfo.  Should we keep it in RangeTblEntry?
> Should it go somewhere else entirely?  I'm just speculating,
> but now is a good time to think about it.

After fixing the issue related to this mentioned by Andres, I started
thinking more about this, especially the "Should it go somewhere else
entirely?" part.

I've kept extraUpdatedCols in RangeTblEntry in the latest patch, but
perhaps it makes sense to put that into Query?  So, the stanza in
RewriteQuery() that sets extraUpdatedCols will be changed as:

@@ -3769,7 +3769,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
                                    NULL, 0, NULL);

            /* Also populate extraUpdatedCols (for generated columns) */
-           fill_extraUpdatedCols(rt_entry, rt_perminfo, rt_entry_relation);
+           parsetree->extraUpdatedCols =
+               get_extraUpdatedCols(rt_entry, rt_perminfo, rt_entry_relation);
        }
        else if (event == CMD_MERGE)
        {

Then, like withCheckOptionLists et al, have grouping_planner()
populate an extraUpdatedColsBitmaps in ModifyTable.
ExecInitModifyTable() will assign them directly into ResultRelInfos.
That way, anyplace in the executor that needs to look at
extraUpdatedCols of a given result relation can get that from its
ResultRelInfo, rather than from the RangeTblEntry as now.

Thoughts?


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

Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... One more thing: maybe we should rethink where to put
>> extraUpdatedCols.  Between the facts that it's not used for
>> actual permissions checks, and that it's calculated by the
>> rewriter not parser, it doesn't seem like it really belongs
>> in RelPermissionInfo.  Should we keep it in RangeTblEntry?
>> Should it go somewhere else entirely?  I'm just speculating,
>> but now is a good time to think about it.

> I've kept extraUpdatedCols in RangeTblEntry in the latest patch, but
> perhaps it makes sense to put that into Query?

That's got morally the same problem as keeping it in RangeTblEntry:
those are structures that are built by the parser.  Hacking on them
later isn't terribly clean.

I wonder if it could make sense to postpone calculation of the
extraUpdatedCols out of the rewriter and into the planner, with
the idea that it ends up $someplace in the finished plan tree
but isn't part of the original parsetree.

A different aspect of this is that putting it in Query doesn't
make a lot of sense unless there is only one version of the
bitmap per Query.  In simple UPDATEs that would be true, but
I think that inherited/partitioned UPDATEs would need one per
result relation, which is likely the reason it got dumped in
RangeTblEntry to begin with.

            regards, tom lane

On Tue, Oct 4, 2022 at 12:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> ... One more thing: maybe we should rethink where to put
> >> extraUpdatedCols.  Between the facts that it's not used for
> >> actual permissions checks, and that it's calculated by the
> >> rewriter not parser, it doesn't seem like it really belongs
> >> in RelPermissionInfo.  Should we keep it in RangeTblEntry?
> >> Should it go somewhere else entirely?  I'm just speculating,
> >> but now is a good time to think about it.
>
> > I've kept extraUpdatedCols in RangeTblEntry in the latest patch, but
> > perhaps it makes sense to put that into Query?
>
> That's got morally the same problem as keeping it in RangeTblEntry:
> those are structures that are built by the parser.  Hacking on them
> later isn't terribly clean.
>
> I wonder if it could make sense to postpone calculation of the
> extraUpdatedCols out of the rewriter and into the planner, with
> the idea that it ends up $someplace in the finished plan tree
> but isn't part of the original parsetree.

Looking at PlannerInfo.update_colnos, something that's needed for
execution but not in Query, maybe we can make preprocess_targetlist()
also populate an PlannerInfo.extraUpdatedCols?

> A different aspect of this is that putting it in Query doesn't
> make a lot of sense unless there is only one version of the
> bitmap per Query.  In simple UPDATEs that would be true, but
> I think that inherited/partitioned UPDATEs would need one per
> result relation, which is likely the reason it got dumped in
> RangeTblEntry to begin with.

Yeah, so if we have PlannerInfos.extraUpdatedCols as the root table's
version of that, grouping_planner() can make copies for all result
relations and put the list in ModifyTable.

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

On Tue, Oct 4, 2022 at 1:11 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Oct 4, 2022 at 12:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Amit Langote <amitlangote09@gmail.com> writes:
> > > On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> ... One more thing: maybe we should rethink where to put
> > >> extraUpdatedCols.  Between the facts that it's not used for
> > >> actual permissions checks, and that it's calculated by the
> > >> rewriter not parser, it doesn't seem like it really belongs
> > >> in RelPermissionInfo.  Should we keep it in RangeTblEntry?
> > >> Should it go somewhere else entirely?  I'm just speculating,
> > >> but now is a good time to think about it.
> >
> > > I've kept extraUpdatedCols in RangeTblEntry in the latest patch, but
> > > perhaps it makes sense to put that into Query?
> >
> > That's got morally the same problem as keeping it in RangeTblEntry:
> > those are structures that are built by the parser.  Hacking on them
> > later isn't terribly clean.
> >
> > I wonder if it could make sense to postpone calculation of the
> > extraUpdatedCols out of the rewriter and into the planner, with
> > the idea that it ends up $someplace in the finished plan tree
> > but isn't part of the original parsetree.
>
> Looking at PlannerInfo.update_colnos, something that's needed for
> execution but not in Query, maybe we can make preprocess_targetlist()
> also populate an PlannerInfo.extraUpdatedCols?
>
> > A different aspect of this is that putting it in Query doesn't
> > make a lot of sense unless there is only one version of the
> > bitmap per Query.  In simple UPDATEs that would be true, but
> > I think that inherited/partitioned UPDATEs would need one per
> > result relation, which is likely the reason it got dumped in
> > RangeTblEntry to begin with.
>
> Yeah, so if we have PlannerInfos.extraUpdatedCols as the root table's
> version of that, grouping_planner() can make copies for all result
> relations and put the list in ModifyTable.

I tried in the attached 0004.  ModifyTable gets a new member
extraUpdatedColsBitmaps, which is List of Bitmapset "nodes".

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

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

On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> 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.)

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

On Fri, Oct 7, 2022 at 10:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> 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.

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

On Fri, Oct 7, 2022 at 1:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Oct 7, 2022 at 10:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> 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.

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

On Fri, Oct 7, 2022 at 3:49 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Oct 7, 2022 at 1:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Oct 7, 2022 at 10:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> 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.

Wondering if it might be a good idea to reorder the patches such that
the changes that move extraUpdatedCols out of RangeTblEntry (patches
0003 and 0004) can be considered/applied independently of the changes
that move permission-checking-related fields out of RangeTblEntry
(patch 0001).  The former seem more straightforward except of course
the Bitmapset node infrastructure changes.

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

On 06.10.22 15:29, Amit Langote wrote:
> I tried in the attached 0004.  ModifyTable gets a new member
> extraUpdatedColsBitmaps, which is List of Bitmapset "nodes".
> 
> 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().

Seeing that on 64-bit platforms we have a 4-byte padding gap in the 
Bitmapset struct, sticking a node tag in there seems pretty sensible. 
So turning Bitmapset into a proper Node and then making the other 
adjustments you describe makes sense to me.

Making a new thread about this might be best.

(I can't currently comment on the rest of the patch set.  So I don't 
know if you'll really end up needing lists of bitmapsets.  But from here 
it looks like turning bitmapsets into nodes might be a worthwhile change 
just by itself.)

On Wed, Oct 12, 2022 at 10:50 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> On 06.10.22 15:29, Amit Langote wrote:
> > I tried in the attached 0004.  ModifyTable gets a new member
> > extraUpdatedColsBitmaps, which is List of Bitmapset "nodes".
> >
> > 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().
>
> Seeing that on 64-bit platforms we have a 4-byte padding gap in the
> Bitmapset struct, sticking a node tag in there seems pretty sensible.
> So turning Bitmapset into a proper Node and then making the other
> adjustments you describe makes sense to me.
>
> Making a new thread about this might be best.
>
> (I can't currently comment on the rest of the patch set.  So I don't
> know if you'll really end up needing lists of bitmapsets.  But from here
> it looks like turning bitmapsets into nodes might be a worthwhile change
> just by itself.)

Ok, thanks.  I'll start a new thread about it.

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

On Fri, Oct 7, 2022 at 4:31 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Oct 7, 2022 at 3:49 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Oct 7, 2022 at 1:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Fri, Oct 7, 2022 at 10:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> 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.


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

2022年10月15日(土) 15:01 Amit Langote <amitlangote09@gmail.com>:
>
> On Fri, Oct 7, 2022 at 4:31 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Oct 7, 2022 at 3:49 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Fri, Oct 7, 2022 at 1:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > On Fri, Oct 7, 2022 at 10:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > > On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> 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

On Fri, Nov 4, 2022 at 8:46 AM Ian Lawrence Barwick <barwick@gmail.com> wrote:
> 2022年10月15日(土) 15:01 Amit Langote <amitlangote09@gmail.com>:
> > On Fri, Oct 7, 2022 at 4:31 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Fri, Oct 7, 2022 at 3:49 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > On Fri, Oct 7, 2022 at 1:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > > On Fri, Oct 7, 2022 at 10:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > > > On Thu, Oct 6, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> 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.

Thanks for the heads up.

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

Rebased patch attached and done.

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

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

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 2022-Oct-06, Amit Langote 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().

Hmm, is this related to what Tom posted as part of his 0004 in
https://postgr.es/m/2901865.1667685211@sss.pgh.pa.us

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

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Oct-06, Amit Langote 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,

> Hmm, is this related to what Tom posted as part of his 0004 in
> https://postgr.es/m/2901865.1667685211@sss.pgh.pa.us

It could be.  For some reason I thought that Amit had withdrawn
his proposal to make Bitmapsets be Nodes.  But if it's still live,
then the data structure I invented in my 0004 could plausibly be
replaced by a List of Bitmapsets.

The code I was using that for would rather have fixed-size arrays
of Bitmapsets than variable-size Lists, mainly because it always
knows ab initio what the max length of the array will be.  But
I don't think that the preference is so strong that it justifies
a private data structure.

The main thing I was wondering about in connection with that
was whether to assume that there could be other future applications
of the logic to perform multi-bitmapset union, intersection,
etc.  If so, then I'd be inclined to choose different naming and
put those functions in or near to bitmapset.c.  It doesn't look
like Amit's code needs anything like that, but maybe somebody
has an idea about other applications?

Anyway, I concur with Peter's upthread comment that making
Bitmapsets be Nodes is probably justifiable all by itself.
The lack of a Node tag in them now is just because in a 32-bit
world it seemed like unnecessary bloat.  But on 64-bit machines
it's free, and we aren't optimizing for 32-bit any more.

I do not like the details of v24-0003 at all though, because
it seems to envision that a "node Bitmapset" is a different
thing from a raw Bitmapset.  That can only lead to bugs ---
why would we not make it the case that every Bitmapset is
properly labeled with the node tag?

Also, although I'm on board with making Bitmapsets be Nodes,
I don't think I'm on board with changing their dump format.
Planner node dumps would get enormously bulkier and less
readable if we changed things like

   :relids (b 1 2)

to

   :relids
      {BITMAPSET
       (b 1 2)
      }

or whatever the output would look like as the patch stands.
So that needs a bit more effort, but it's surely manageable.

            regards, tom lane

On Sat, Nov 12, 2022 at 1:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2022-Oct-06, Amit Langote 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,
>
> > Hmm, is this related to what Tom posted as part of his 0004 in
> > https://postgr.es/m/2901865.1667685211@sss.pgh.pa.us
>
> It could be.  For some reason I thought that Amit had withdrawn
> his proposal to make Bitmapsets be Nodes.

I think you are referring to [1] that I had forgotten to link to here.
I did reimplement a data structure in my patch on the "Re: generic
plans and initial pruning" thread to stop using a List of Bitmapsets,
so the Bitmapset as Nodes functionality became unnecessary there,
though I still need it for the proposal here to move
extraUpdatedColumns (patch 0004) into ModifyTable node.

> The code I was using that for would rather have fixed-size arrays
> of Bitmapsets than variable-size Lists, mainly because it always
> knows ab initio what the max length of the array will be.  But
> I don't think that the preference is so strong that it justifies
> a private data structure.
>
> The main thing I was wondering about in connection with that
> was whether to assume that there could be other future applications
> of the logic to perform multi-bitmapset union, intersection,
> etc.  If so, then I'd be inclined to choose different naming and
> put those functions in or near to bitmapset.c.  It doesn't look
> like Amit's code needs anything like that, but maybe somebody
> has an idea about other applications?

Yes, simple storage of multiple Bitmapsets in a List somewhere in a
parse/plan tree sounded like that would have wider enough use to add
proper node support for.   Assuming you mean trying to generalize
VarAttnoSet in your patch 0004 posted at [2], I wonder if you want to
somehow make its indexability by varno / RT index a part of the
interface of the generic code you're thinking for it?  For example,
varattnoset_*_members collection of routines in that patch seem to
assume that the Bitmapsets at a given index in the provided pair of
VarAttnoSets are somehow related -- covering to the same base relation
in this case.  That does not sound very generalizable but maybe that
is not what you are thinking at all.

> Anyway, I concur with Peter's upthread comment that making
> Bitmapsets be Nodes is probably justifiable all by itself.
> The lack of a Node tag in them now is just because in a 32-bit
> world it seemed like unnecessary bloat.  But on 64-bit machines
> it's free, and we aren't optimizing for 32-bit any more.
>
> I do not like the details of v24-0003 at all though, because
> it seems to envision that a "node Bitmapset" is a different
> thing from a raw Bitmapset.  That can only lead to bugs ---
> why would we not make it the case that every Bitmapset is
> properly labeled with the node tag?

Yeah, I just didn't think hard enough to realize that having
bitmapset.c itself set the node tag is essentially free, and it looks
like a better design anyway than the design where callers get to
choose to make the bitmapset they are manipulating a Node or not.

> Also, although I'm on board with making Bitmapsets be Nodes,
> I don't think I'm on board with changing their dump format.
> Planner node dumps would get enormously bulkier and less
> readable if we changed things like
>
>    :relids (b 1 2)
>
> to
>
>    :relids
>       {BITMAPSET
>        (b 1 2)
>       }
>
> or whatever the output would look like as the patch stands.
> So that needs a bit more effort, but it's surely manageable.

Agreed with leaving the dump format unchanged or not bloating it.

Thanks a lot for 5e1f3b9ebf6e5.

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

[1] https://www.postgresql.org/message-id/CA+HiwqG8L3DVoZauJi1-eorLnnoM6VcfJCCauQX8=ofi-qMYCQ@mail.gmail.com
[2] https://www.postgresql.org/message-id/2901865.1667685211%40sss.pgh.pa.us

On 2022-Nov-10, Alvaro Herrera wrote:

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

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


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

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/

Hi Alvaro,

On Thu, Nov 10, 2022 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Hello
>
> I've been trying to understand what 0001 does.

Thanks a lot for taking a look.

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

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.

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

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?

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

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.

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

Agreed, I have merged your delta patch into 0001.

On Wed, Nov 16, 2022 at 8:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > 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 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.
>
> 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.

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

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

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

On Mon, Nov 21, 2022 at 9:03 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Nov 10, 2022 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > 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.
>
> 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.

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.

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

On Mon, Nov 21, 2022 at 9:03 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Nov 16, 2022 at 8:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > > 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 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.
> >
> > 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.
>
> 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.)

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.

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

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

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

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

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.

On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Thanks for the new version, in particular thank you for fixing the
> annoyance with the CombineRangeTables API.

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

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

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

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

Facepalm, yes.

> My 0002 does that.

Merged.

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

Yes, agree that's better.

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

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
> those separately later.)

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

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

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

On 2022-Nov-29, Amit Langote wrote:

> Maybe, we should have the following there so that the PlannedStmt's
> contents don't point into the Query?
> 
>     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/

On Wed, Nov 30, 2022 at 3:04 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Nov-29, Amit Langote wrote:
>
> > Maybe, we should have the following there so that the PlannedStmt's
> > contents don't point into the Query?
> >
> >     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.

OK, something like the attached?

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

On Wed, Nov 30, 2022 at 11:56 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Nov 30, 2022 at 3:04 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2022-Nov-29, Amit Langote wrote:
> >
> > > Maybe, we should have the following there so that the PlannedStmt's
> > > contents don't point into the Query?
> > >
> > >     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.
>
> 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.

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

Hello

On 2022-Nov-29, Amit Langote wrote:

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

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

> On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

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

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

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
> checkAsUser in some foreign table planning/execution code sites.
> Thought it might be better to describe it separately too.

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

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

Hello,

This didn't apply, so I rebased it on current master, excluding the one
I already pushed.  No further changes.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

Hi Alvaro,

On Wed, Nov 30, 2022 at 5:32 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > 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 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?)

No, I think it was I who came up with the "..list" naming and
basically just stuck with it.

Actually, I don't mind changing to "...infos", which I have done in
the attached updated patch.

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

This sounds like a better idea than adding a new AttrMap, so done this
way in the attached 0001.

> > 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.
>
> I'll get this one pushed soon, it seems good to me.  (I'll edit to not
> use Oid as boolean.)

Thanks for committing that one.

I've also merged into 0002 the delta patch I had posted earlier to add
a copy of RTEPermInfos into the flattened permInfos list instead of
adding the Query's copy.

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

On Fri, Dec 2, 2022 at 4:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Hi Alvaro,
>
> On Wed, Nov 30, 2022 at 5:32 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > > On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > > 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 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?)
>
> No, I think it was I who came up with the "..list" naming and
> basically just stuck with it.
>
> Actually, I don't mind changing to "...infos", which I have done in
> the attached updated patch.
>
> > > 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.
> >
> > 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.
>
> This sounds like a better idea than adding a new AttrMap, so done this
> way in the attached 0001.
>
> > > 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.
> >
> > I'll get this one pushed soon, it seems good to me.  (I'll edit to not
> > use Oid as boolean.)
>
> Thanks for committing that one.
>
> I've also merged into 0002 the delta patch I had posted earlier to add
> a copy of RTEPermInfos into the flattened permInfos list instead of
> adding the Query's copy.

Oops, hit send before attaching anything.

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

Hello,

On 2022-Dec-02, Amit Langote wrote:

> This sounds like a better idea than adding a new AttrMap, so done this
> way in the attached 0001.

Thanks for doing that!  I have pushed it, but I renamed
ri_RootToPartitionMap to ri_RootToChildMap and moved it to another spot
in ResultRelInfo, which allows to simplify the comments.

> I've also merged into 0002 the delta patch I had posted earlier to add
> a copy of RTEPermInfos into the flattened permInfos list instead of
> adding the Query's copy.

Great.  At this point I have no other comments, except that in both
parse_relation.c and rewriteManip.c you've chosen to add the new
functions at the bottom of each file, which is seldom a good choice.
I think in the case of CombineRangeTables it should be the very first
function in the file, before all the walker-type stuff; and for
Add/GetRTEPermissionInfo I would suggest that right below
addRangeTableEntryForENR might be a decent choice (need to fix the .h
files to match, of course.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"World domination is proceeding according to plan"        (Andrew Morton)

On Fri, Dec 2, 2022 at 7:00 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Dec-02, Amit Langote wrote:
> > This sounds like a better idea than adding a new AttrMap, so done this
> > way in the attached 0001.
>
> Thanks for doing that!  I have pushed it, but I renamed
> ri_RootToPartitionMap to ri_RootToChildMap and moved it to another spot
> in ResultRelInfo, which allows to simplify the comments.

Thanks.

> > I've also merged into 0002 the delta patch I had posted earlier to add
> > a copy of RTEPermInfos into the flattened permInfos list instead of
> > adding the Query's copy.
>
> Great.  At this point I have no other comments, except that in both
> parse_relation.c and rewriteManip.c you've chosen to add the new
> functions at the bottom of each file, which is seldom a good choice.
> I think in the case of CombineRangeTables it should be the very first
> function in the file, before all the walker-type stuff; and for
> Add/GetRTEPermissionInfo I would suggest that right below
> addRangeTableEntryForENR might be a decent choice (need to fix the .h
> files to match, of course.)

Okay, I've moved the functions and their .h declarations to the places
you suggest.  While at it, I also uncapitalized Add/Get, because
that's how the nearby functions in the header are named.

Thanks again for the review.  The patch looks much better than it did
3 weeks ago.

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

On Fri, Dec 2, 2022 at 8:13 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Dec 2, 2022 at 7:00 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Great.  At this point I have no other comments, except that in both
> > parse_relation.c and rewriteManip.c you've chosen to add the new
> > functions at the bottom of each file, which is seldom a good choice.
> > I think in the case of CombineRangeTables it should be the very first
> > function in the file, before all the walker-type stuff; and for
> > Add/GetRTEPermissionInfo I would suggest that right below
> > addRangeTableEntryForENR might be a decent choice (need to fix the .h
> > files to match, of course.)
>
> Okay, I've moved the functions and their .h declarations to the places
> you suggest.  While at it, I also uncapitalized Add/Get, because
> that's how the nearby functions in the header are named.
>
> Thanks again for the review.  The patch looks much better than it did
> 3 weeks ago.

Rebased over 2605643a3a9d.

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

I have pushed this finally.

I made two further changes:

1. there was no reason to rename ExecCheckPerms_hook, since its
   signature was changing anyway.  I reverted it to the original name.

2. I couldn't find any reason to expose ExecGetRTEPermissionInfo, and
   given that it's a one-line function, I removed it.

Maybe you had a reason to add ExecGetRTEPermissionInfo, thinking about
external callers; if so please discuss it.

I'll mark this commitfest entry as committed soon; please post the other
two patches you had in this series in a new thread.

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

On Wed, Dec 7, 2022 at 12:19 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I have pushed this finally.

Thanks a lot.

> I made two further changes:
>
> 1. there was no reason to rename ExecCheckPerms_hook, since its
>    signature was changing anyway.  I reverted it to the original name.

Sure, that makes sense.

> 2. I couldn't find any reason to expose ExecGetRTEPermissionInfo, and
>    given that it's a one-line function, I removed it.
>
> Maybe you had a reason to add ExecGetRTEPermissionInfo, thinking about
> external callers; if so please discuss it.

My thinking was that it might be better to have a macro/function that
takes EState, not es_rteperminfos, from the callers.  Kind of like how
there is exec_rt_fetch().  Though, that is only a cosmetic
consideration, so I don't want to insist.

> I'll mark this commitfest entry as committed soon; please post the other
> two patches you had in this series in a new thread.

Will do, thanks.

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

On Wed, Dec 7, 2022 at 4:01 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Dec 7, 2022 at 12:19 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > I have pushed this finally.
>>
> > I'll mark this commitfest entry as committed soon; please post the other
> > two patches you had in this series in a new thread.
>
> Will do, thanks.

While doing that, I noticed that I had missed updating at least one
comment which still says that permission checking is done off of the
range table.  Attached patch fixes that.

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

On 2022-Dec-06, Alvaro Herrera wrote:

> I have pushed this finally.
> 
> I made two further changes:

Actually, I made one further change that I forgot to mention -- I
changed the API of CombineRangeTables once again; the committed patch
has it this way:

+/*
+ * CombineRangeTables
+ *         Adds the RTEs of 'src_rtable' into 'dst_rtable'
+ *
+ * This also adds the RTEPermissionInfos of 'src_perminfos' (belonging to the
+ * RTEs in 'src_rtable') into *dst_perminfos and also updates perminfoindex of
+ * the RTEs in 'src_rtable' to now point to the perminfos' indexes in
+ * *dst_perminfos.
+ *
+ * Note that this changes both 'dst_rtable' and 'dst_perminfo' destructively,
+ * so the caller should have better passed safe-to-modify copies.
+ */
+void
+CombineRangeTables(List **dst_rtable, List **dst_perminfos,
+                  List *src_rtable, List *src_perminfos)

The original one had the target rangetable first, then the source
RT+perminfos, and the target perminfos at the end.  This seemed
inconsistent and potentially confusing.  I also changed the argument
names (from using numbers to "dst/src" monikers) and removed the
behavior of returning the list: ISTM it did turn out to be a bad idea
after all.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Wed, Dec 7, 2022 at 7:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Dec-06, Alvaro Herrera wrote:
> > I have pushed this finally.
> >
> > I made two further changes:
>
> Actually, I made one further change that I forgot to mention -- I
> changed the API of CombineRangeTables once again; the committed patch
> has it this way:
>
> +/*
> + * CombineRangeTables
> + *         Adds the RTEs of 'src_rtable' into 'dst_rtable'
> + *
> + * This also adds the RTEPermissionInfos of 'src_perminfos' (belonging to the
> + * RTEs in 'src_rtable') into *dst_perminfos and also updates perminfoindex of
> + * the RTEs in 'src_rtable' to now point to the perminfos' indexes in
> + * *dst_perminfos.
> + *
> + * Note that this changes both 'dst_rtable' and 'dst_perminfo' destructively,
> + * so the caller should have better passed safe-to-modify copies.
> + */
> +void
> +CombineRangeTables(List **dst_rtable, List **dst_perminfos,
> +                  List *src_rtable, List *src_perminfos)
>
> The original one had the target rangetable first, then the source
> RT+perminfos, and the target perminfos at the end.  This seemed
> inconsistent and potentially confusing.  I also changed the argument
> names (from using numbers to "dst/src" monikers) and removed the
> behavior of returning the list: ISTM it did turn out to be a bad idea
> after all.

This looks better to me too.

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

On 2022-Dec-07, Amit Langote wrote:

> While doing that, I noticed that I had missed updating at least one
> comment which still says that permission checking is done off of the
> range table.  Attached patch fixes that.

Pushed, thanks.

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

On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> 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.

This was committed as 599b33b94:
    Stop accessing checkAsUser via RTE in some cases

Which does this in a couple places in selfuncs.c:

                                if (!vardata->acl_ok &&
                                    root->append_rel_array != NULL)
                                {   
                                    AppendRelInfo *appinfo;
                                    Index       varno = index->rel->relid;

                                    appinfo = root->append_rel_array[varno];
                                    while (appinfo &&
                                           planner_rt_fetch(appinfo->parent_relid,
                                                            root)->rtekind == RTE_RELATION)
                                    {   
                                        varno = appinfo->parent_relid;
                                        appinfo = root->append_rel_array[varno];
                                    }
                                    if (varno != index->rel->relid)
                                    {   
                                        /* Repeat access check on this rel */
                                        rte = planner_rt_fetch(varno, root);
                                        Assert(rte->rtekind == RTE_RELATION);

-                                       userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+                                       userid = OidIsValid(onerel->userid) ?
+                                           onerel->userid : GetUserId();

                                        vardata->acl_ok =
                                            rte->securityQuals == NIL &&
                                            (pg_class_aclcheck(rte->relid,
                                                               userid,
                                                               ACL_SELECT) == ACLCHECK_OK);
                                    }
                                }


The original code rechecks rte->checkAsUser with the rte of the parent
rel.  The patch changed to access onerel instead, but that's not updated
after looping to find the parent.

Is that okay ?  It doesn't seem intentional, since "userid" is still
being recomputed, but based on onerel, which hasn't changed.  The
original intent (since 553d2ec27) is to recheck the parent's
"checkAsUser".  

It seems like this would matter for partitioned tables, when the
partition isn't readable, but its parent is, and accessed via a view
owned by another user?

-- 
Justin

Hi,

On Sun, Dec 11, 2022 at 5:17 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> > 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.
>
> This was committed as 599b33b94:
>     Stop accessing checkAsUser via RTE in some cases
>
> Which does this in a couple places in selfuncs.c:
>
>                                 if (!vardata->acl_ok &&
>                                     root->append_rel_array != NULL)
>                                 {
>                                     AppendRelInfo *appinfo;
>                                     Index       varno = index->rel->relid;
>
>                                     appinfo = root->append_rel_array[varno];
>                                     while (appinfo &&
>                                            planner_rt_fetch(appinfo->parent_relid,
>                                                             root)->rtekind == RTE_RELATION)
>                                     {
>                                         varno = appinfo->parent_relid;
>                                         appinfo = root->append_rel_array[varno];
>                                     }
>                                     if (varno != index->rel->relid)
>                                     {
>                                         /* Repeat access check on this rel */
>                                         rte = planner_rt_fetch(varno, root);
>                                         Assert(rte->rtekind == RTE_RELATION);
>
> -                                       userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
> +                                       userid = OidIsValid(onerel->userid) ?
> +                                           onerel->userid : GetUserId();
>
>                                         vardata->acl_ok =
>                                             rte->securityQuals == NIL &&
>                                             (pg_class_aclcheck(rte->relid,
>                                                                userid,
>                                                                ACL_SELECT) == ACLCHECK_OK);
>                                     }
>                                 }
>
>
> The original code rechecks rte->checkAsUser with the rte of the parent
> rel.  The patch changed to access onerel instead, but that's not updated
> after looping to find the parent.
>
> Is that okay ?  It doesn't seem intentional, since "userid" is still
> being recomputed, but based on onerel, which hasn't changed.  The
> original intent (since 553d2ec27) is to recheck the parent's
> "checkAsUser".
>
> It seems like this would matter for partitioned tables, when the
> partition isn't readable, but its parent is, and accessed via a view
> owned by another user?

Thanks for pointing this out.

I think these blocks which are rewriting userid to basically the same
value should have been removed from these sites as part of 599b33b94.
Even before that commit, the checkAsUser value should have been the
same in the RTE of both the child relation passed to these functions
and that of the root parent that's looked up by looping.  That's
because expand_single_inheritance_child(), which adds child RTEs,
copies the parent RTE's checkAsUser, that is, as of commit 599b33b94.
A later commit a61b1f74823c has removed the checkAsUser field from
RangeTblEntry.

Moreover, 599b33b94 adds some code in build_simple_rel() to set a
given rel's userid value by copying it from the parent rel, such that
the userid value would be the same in all relations in a given
inheritance tree.

I've attached 0001 to remove those extraneous code blocks and add a
comment mentioning that userid need not be recomputed.

While staring at the build_simple_rel() bit mentioned above, I
realized that this code fails to set userid correctly in the
inheritance parent rels that are child relations of subquery parent
relations, such as UNION ALL subqueries.  In that case, instead of
copying the userid (= 0) of the parent rel, the child should look up
its own RTEPermissionInfo, which should be there, and use the
checkAsUser value from there.  I've attached 0002 to fix this hole.  I
am not sure whether there's a way to add a test case for this in the
core suite.

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

On Sun, Dec 11, 2022 at 06:25:48PM +0900, Amit Langote wrote:
> On Sun, Dec 11, 2022 at 5:17 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > The original code rechecks rte->checkAsUser with the rte of the parent
> > rel.  The patch changed to access onerel instead, but that's not updated
> > after looping to find the parent.
> >
> > Is that okay ?  It doesn't seem intentional, since "userid" is still
> > being recomputed, but based on onerel, which hasn't changed.  The
> > original intent (since 553d2ec27) is to recheck the parent's
> > "checkAsUser".
> >
> > It seems like this would matter for partitioned tables, when the
> > partition isn't readable, but its parent is, and accessed via a view
> > owned by another user?
> 
> Thanks for pointing this out.
> 
> I think these blocks which are rewriting userid to basically the same
> value should have been removed from these sites as part of 599b33b94.

I thought maybe; thanks for checking.

Little nitpicks:

001:
Fine to use the same userid as it's same in all
=> the same

002:
give that it's a subquery rel.
=> given

-- 
Justin

On Sun, Dec 11, 2022 at 6:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> I've attached 0001 to remove those extraneous code blocks and add a
> comment mentioning that userid need not be recomputed.
>
> While staring at the build_simple_rel() bit mentioned above, I
> realized that this code fails to set userid correctly in the
> inheritance parent rels that are child relations of subquery parent
> relations, such as UNION ALL subqueries.  In that case, instead of
> copying the userid (= 0) of the parent rel, the child should look up
> its own RTEPermissionInfo, which should be there, and use the
> checkAsUser value from there.  I've attached 0002 to fix this hole.  I
> am not sure whether there's a way to add a test case for this in the
> core suite.

Ah, I realized we could just expand the test added by 553d2ec27 with a
wrapper view (to test checkAsUser functionality) and a UNION ALL query
over the view (to test this change).

I've done that in the attached updated patch, in which I've also
addressed Justin's comments.

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

Alvaro could you comment on this ?

On Wed, Dec 21, 2022 at 01:44:11PM -0600, Justin Pryzby wrote:
> Alvaro could you comment on this ?

I added here so it's not forgotten.
https://commitfest.postgresql.org/42/4107/

Justin Pryzby <pryzby@telsasoft.com> writes:
> On Wed, Dec 21, 2022 at 01:44:11PM -0600, Justin Pryzby wrote:
>> Alvaro could you comment on this ?

I believe Alvaro's on vacation for a few days more.

            regards, tom lane

On 2022-Dec-12, Amit Langote wrote:

> On Sun, Dec 11, 2022 at 6:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > I've attached 0001 to remove those extraneous code blocks and add a
> > comment mentioning that userid need not be recomputed.
> >
> > While staring at the build_simple_rel() bit mentioned above, I
> > realized that this code fails to set userid correctly in the
> > inheritance parent rels that are child relations of subquery parent
> > relations, such as UNION ALL subqueries.  In that case, instead of
> > copying the userid (= 0) of the parent rel, the child should look up
> > its own RTEPermissionInfo, which should be there, and use the
> > checkAsUser value from there.  I've attached 0002 to fix this hole.  I
> > am not sure whether there's a way to add a test case for this in the
> > core suite.
> 
> Ah, I realized we could just expand the test added by 553d2ec27 with a
> wrapper view (to test checkAsUser functionality) and a UNION ALL query
> over the view (to test this change).

Hmm, but if I run this test without the code change in 0002, the test
also passes (to wit: the plan still has two hash joins), so I understand
that either you're testing something that's not changed by the patch,
or the test case itself isn't really what you wanted.

As for 0001, it seems simpler to me to put the 'userid' variable in the
same scope as 'onerel', and then compute it just once and don't bother
with it at all afterwards, as in the attached.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

On Tue, Jan 17, 2023 at 7:33 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Dec-12, Amit Langote wrote:
> > On Sun, Dec 11, 2022 at 6:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > I've attached 0001 to remove those extraneous code blocks and add a
> > > comment mentioning that userid need not be recomputed.
> > >
> > > While staring at the build_simple_rel() bit mentioned above, I
> > > realized that this code fails to set userid correctly in the
> > > inheritance parent rels that are child relations of subquery parent
> > > relations, such as UNION ALL subqueries.  In that case, instead of
> > > copying the userid (= 0) of the parent rel, the child should look up
> > > its own RTEPermissionInfo, which should be there, and use the
> > > checkAsUser value from there.  I've attached 0002 to fix this hole.  I
> > > am not sure whether there's a way to add a test case for this in the
> > > core suite.
> >
> > Ah, I realized we could just expand the test added by 553d2ec27 with a
> > wrapper view (to test checkAsUser functionality) and a UNION ALL query
> > over the view (to test this change).
>
> Hmm, but if I run this test without the code change in 0002, the test
> also passes (to wit: the plan still has two hash joins), so I understand
> that either you're testing something that's not changed by the patch,
> or the test case itself isn't really what you wanted.

Yeah, the test case is bogus. :-(.

It seems that, with the test as written, it's not the partitioned
table referenced in the view's query that becomes a child of the UNION
ALL parent subquery, but the subquery itself.  The bug being fixed in
0002 doesn't affect the planning of this query at all, because child
subquery is planned independently of the main query involving UNION
ALL because of it being unable to be pushed up into the latter.  We
want the partitioned table referenced in the child subquery to become
a child of the UNION ALL parent subquery for the bug to be relevant.

I tried rewriting the test such that the view's subquery does get
pulled up such that the partitioned table becomes a child of the UNION
ALL subquery.  By attaching a debugger, I do see the bug affecting the
planning of this query, but still not in a way that changes the plan.
I will keep trying but in the meantime I'm attaching 0001 to show the
rewritten query and the plan.

> As for 0001, it seems simpler to me to put the 'userid' variable in the
> same scope as 'onerel', and then compute it just once and don't bother
> with it at all afterwards, as in the attached.

That sounds better.  Attached as 0002.

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

On 2023-Jan-19, Amit Langote wrote:

> It seems that, with the test as written, it's not the partitioned
> table referenced in the view's query that becomes a child of the UNION
> ALL parent subquery, but the subquery itself.  The bug being fixed in
> 0002 doesn't affect the planning of this query at all, because child
> subquery is planned independently of the main query involving UNION
> ALL because of it being unable to be pushed up into the latter.  We
> want the partitioned table referenced in the child subquery to become
> a child of the UNION ALL parent subquery for the bug to be relevant.
> 
> I tried rewriting the test such that the view's subquery does get
> pulled up such that the partitioned table becomes a child of the UNION
> ALL subquery.  By attaching a debugger, I do see the bug affecting the
> planning of this query, but still not in a way that changes the plan.
> I will keep trying but in the meantime I'm attaching 0001 to show the
> rewritten query and the plan.

Thanks for spending time tracking down a test case.  I'll try to have a
look later today.

> > As for 0001, it seems simpler to me to put the 'userid' variable in the
> > same scope as 'onerel', and then compute it just once and don't bother
> > with it at all afterwards, as in the attached.
> 
> That sounds better.  Attached as 0002.

Pushed this one, thank you.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> 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.

This was committed as 599b33b94:
    Stop accessing checkAsUser via RTE in some cases

That seems to add various elog()s which are hit frequently by sqlsmith:

postgres=# select from
(select transaction
from pg_prepared_xacts
right join pg_available_extensions
on false limit 0) where false;
ERROR:  permission info at index 2 (with relid=1262) does not match provided RTE (with relid=12081)

postgres=# select from (select confl_tablespace
from pg_stat_database_conflicts
where datname <> (select 'af')
limit 1) where false;
ERROR:  invalid perminfoindex 1 in RTE with relid 12271

Amit Langote <amitlangote09@gmail.com> writes:
> On Mon, Feb 13, 2023 at 5:07 Justin Pryzby <pryzby@telsasoft.com> wrote:
>> That seems to add various elog()s which are hit frequently by sqlsmith:

> Thanks for the report.  I’ll take a look once I’m back at a computer in a
> few days.

Looks like we already have a diagnosis and fix [1].  I'll get that
pushed.

            regards, tom lane

[1] https://www.postgresql.org/message-id/CAHewXNnnNySD_YcKNuFpQDV2gxWA7_YLWqHmYVcyoOYxn8kY2A%40mail.gmail.com

On 2022-Dec-11, Amit Langote wrote:

> While staring at the build_simple_rel() bit mentioned above, I
> realized that this code fails to set userid correctly in the
> inheritance parent rels that are child relations of subquery parent
> relations, such as UNION ALL subqueries.  In that case, instead of
> copying the userid (= 0) of the parent rel, the child should look up
> its own RTEPermissionInfo, which should be there, and use the
> checkAsUser value from there.  I've attached 0002 to fix this hole.  I
> am not sure whether there's a way to add a test case for this in the
> core suite.

I gave this a look and I thought it was clearer to have the new
condition depend on rel->reloptkind instead parent or no.

I tried a few things for a new test case, but I was unable to find
anything useful.  Maybe an intermediate view, I thought; no dice.
Maybe one with a security barrier would do?  Anyway, for now I just kept
what you added in v2.

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

On 2023-Feb-17, Alvaro Herrera wrote:

> I tried a few things for a new test case, but I was unable to find
> anything useful.  Maybe an intermediate view, I thought; no dice.
> Maybe one with a security barrier would do?  Anyway, for now I just kept
> what you added in v2.

Sorry, I failed to keep count of the patch version correctly.  The test
case here is what you sent in v3 [1], and consequently the patch I just
attached should have been labelled v4.

[1] https://postgr.es/m/CA+HiwqF6ricH7HFCkyrK72c=KN-PRkdncxdLmU_mEQx=DRAkJA@mail.gmail.com

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La vida es para el que se aventura"

On Fri, Feb 17, 2023 at 9:02 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Dec-11, Amit Langote wrote:
> > While staring at the build_simple_rel() bit mentioned above, I
> > realized that this code fails to set userid correctly in the
> > inheritance parent rels that are child relations of subquery parent
> > relations, such as UNION ALL subqueries.  In that case, instead of
> > copying the userid (= 0) of the parent rel, the child should look up
> > its own RTEPermissionInfo, which should be there, and use the
> > checkAsUser value from there.  I've attached 0002 to fix this hole.  I
> > am not sure whether there's a way to add a test case for this in the
> > core suite.
>
> I gave this a look and I thought it was clearer to have the new
> condition depend on rel->reloptkind instead parent or no.

Thanks for looking into this again.  I agree the condition with
reloptkind might be better.

> I tried a few things for a new test case, but I was unable to find
> anything useful.  Maybe an intermediate view, I thought; no dice.
> Maybe one with a security barrier would do?  Anyway, for now I just kept
> what you added in v2.

Hmm, I'm fine with leaving the test case out if it doesn't really test
the code we're changing, as you also pointed out?

One more thing we could try is come up with a postgres_fdw test case,
because it uses the RelOptInfo.userid value for remote-costs-based
path size estimation.  But adding a test case to contrib module's
suite test a core planner change might seem strange, ;-).

Attaching v4 without the test case.

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

On 2023-Feb-20, Amit Langote wrote:

> On Fri, Feb 17, 2023 at 9:02 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > I tried a few things for a new test case, but I was unable to find
> > anything useful.  Maybe an intermediate view, I thought; no dice.
> > Maybe one with a security barrier would do?  Anyway, for now I just kept
> > what you added in v2.
> 
> Hmm, I'm fine with leaving the test case out if it doesn't really test
> the code we're changing, as you also pointed out?

Yeah, pushed like that.

> One more thing we could try is come up with a postgres_fdw test case,
> because it uses the RelOptInfo.userid value for remote-costs-based
> path size estimation.  But adding a test case to contrib module's
> suite test a core planner change might seem strange, ;-).

Maybe.  Perhaps adding it in a separate file there is okay?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Small aircraft do not crash frequently ... usually only once!"
                                  (ponder, http://thedailywtf.com/)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Feb-20, Amit Langote wrote:
>> One more thing we could try is come up with a postgres_fdw test case,
>> because it uses the RelOptInfo.userid value for remote-costs-based
>> path size estimation.  But adding a test case to contrib module's
>> suite test a core planner change might seem strange, ;-).

> Maybe.  Perhaps adding it in a separate file there is okay?

There is plenty of stuff in contrib module tests that is really
there to test core-code behavior.  (You could indeed argue that
*all* of contrib is there for that purpose.)  If it's not
convenient to test something without an extension, just do it
and don't sweat about that.

            regards, tom lane

On Tue, Feb 21, 2023 at 12:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2023-Feb-20, Amit Langote wrote:
> >> One more thing we could try is come up with a postgres_fdw test case,
> >> because it uses the RelOptInfo.userid value for remote-costs-based
> >> path size estimation.  But adding a test case to contrib module's
> >> suite test a core planner change might seem strange, ;-).
>
> > Maybe.  Perhaps adding it in a separate file there is okay?
>
> There is plenty of stuff in contrib module tests that is really
> there to test core-code behavior.  (You could indeed argue that
> *all* of contrib is there for that purpose.)  If it's not
> convenient to test something without an extension, just do it
> and don't sweat about that.

OK.  Attached adds a test case to postgres_fdw's suite.  You can see
that it fails without a316a3bc.

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

Hi,

On Tue, Feb 21, 2023 at 4:12 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Feb 21, 2023 at 12:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > > On 2023-Feb-20, Amit Langote wrote:
> > >> One more thing we could try is come up with a postgres_fdw test case,
> > >> because it uses the RelOptInfo.userid value for remote-costs-based
> > >> path size estimation.  But adding a test case to contrib module's
> > >> suite test a core planner change might seem strange, ;-).
> >
> > > Maybe.  Perhaps adding it in a separate file there is okay?
> >
> > There is plenty of stuff in contrib module tests that is really
> > there to test core-code behavior.  (You could indeed argue that
> > *all* of contrib is there for that purpose.)  If it's not
> > convenient to test something without an extension, just do it
> > and don't sweat about that.
>
> OK.  Attached adds a test case to postgres_fdw's suite.  You can see
> that it fails without a316a3bc.

Noticed that there's an RfC entry for this in the next CF.  Here's an
updated version of the patch where I updated the comments a bit and
the commit message.

I'm thinking of pushing this on Friday barring objections.

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

On Wed, Jun 28, 2023 at 4:30 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi,
>
> On Tue, Feb 21, 2023 at 4:12 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Tue, Feb 21, 2023 at 12:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > > > On 2023-Feb-20, Amit Langote wrote:
> > > >> One more thing we could try is come up with a postgres_fdw test case,
> > > >> because it uses the RelOptInfo.userid value for remote-costs-based
> > > >> path size estimation.  But adding a test case to contrib module's
> > > >> suite test a core planner change might seem strange, ;-).
> > >
> > > > Maybe.  Perhaps adding it in a separate file there is okay?
> > >
> > > There is plenty of stuff in contrib module tests that is really
> > > there to test core-code behavior.  (You could indeed argue that
> > > *all* of contrib is there for that purpose.)  If it's not
> > > convenient to test something without an extension, just do it
> > > and don't sweat about that.
> >
> > OK.  Attached adds a test case to postgres_fdw's suite.  You can see
> > that it fails without a316a3bc.
>
> Noticed that there's an RfC entry for this in the next CF.  Here's an
> updated version of the patch where I updated the comments a bit and
> the commit message.
>
> I'm thinking of pushing this on Friday barring objections.

Seeing none, I've pushed this to HEAD and 16.

Marking the CF entry as committed.

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