Horton

Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)

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

View thread View on postgr.es
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, 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>, pgsql-hackers@lists.postgresql.org
Date: 2023-01-17T10:26:54Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Add a test case for a316a3bc

  2. Correctly set userid of subquery relations' child rels

  3. Fix buggy recursion in flatten_rtes_walker().

  4. Remove some dead code in selfuncs.c

  5. Update outdated comment in ApplyRetrieveRule

  6. Rework query relation permission checking

  7. Generalize ri_RootToPartitionMap to use for non-partition children

  8. Stop accessing checkAsUser via RTE in some cases

  9. Add 'missing_ok' argument to build_attrmap_by_name