Re: ExecRTCheckPerms() and many prunable partitions

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(at)alvh(dot)no-ip(dot)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/