Horton

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

Amit Langote <amitlangote09@gmail.com>

View thread View on postgr.es
From: Amit Langote <amitlangote09@gmail.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
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-02-20T07:56:22Z
Lists: pgsql-hackers

Attachments

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

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