Re: ExecRTCheckPerms() and many prunable partitions

Amit Langote <amitlangote09@gmail.com>

From: Amit Langote <amitlangote09@gmail.com>

To: Alvaro Herrera <alvherre@alvh.no-ip.org>

Cc: Ian Lawrence Barwick <barwick@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, David Rowley <dgrowleyml@gmail.com>, Greg Stark <stark@mit.edu>, Julien Rouhaud <rjuju123@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Date: 2022-11-25T11:28:37Z

Lists: pgsql-hackers

Attachments

v27-0002-Fix-AclMode-node-support.patch (application/octet-stream) patch v27-0002
v27-0001-Rework-query-relation-permission-checking.patch (application/octet-stream) patch v27-0001
v27-0003-ConcatRTEPermissionInfoLists-CombineRangeTable.patch (application/octet-stream) patch v27-0003
v27-0005-Add-per-result-relation-extraUpdatedCols-to-Modi.patch (application/octet-stream) patch v27-0005
v27-0004-Do-not-add-the-NEW-entry-to-view-rule-action-s-r.patch (application/octet-stream) patch v27-0004

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

Commits

Add a test case for a316a3bc
- 054ff3b33a85 17.0 landed
- 0a14bca662a4 16.0 landed
Correctly set userid of subquery relations' child rels
- a316a3bc6d3f 16.0 landed
Fix buggy recursion in flatten_rtes_walker().
- c7468c73f7b6 16.0 landed
Remove some dead code in selfuncs.c
- 438e6b724090 16.0 landed
Update outdated comment in ApplyRetrieveRule
- 29861e228a47 16.0 landed
Rework query relation permission checking
- a61b1f74823c 16.0 landed
Generalize ri_RootToPartitionMap to use for non-partition children
- fb958b5da86d 16.0 landed
Stop accessing checkAsUser via RTE in some cases
- 599b33b9492d 16.0 landed
Add 'missing_ok' argument to build_attrmap_by_name
- ad86d159b6ab 16.0 landed