Re: ExecRTCheckPerms() and many prunable partitions

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Ian Lawrence Barwick <barwick(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Rowley <dgrowleyml(at)gmail(dot)com>, Greg Stark <stark(at)mit(dot)edu>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: ExecRTCheckPerms() and many prunable partitions
Date: 2022-11-29 09:27:08
Message-ID: 20221129092708.uooopfzpiwrhgedl@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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/

Attachment Content-Type Size
v28-0001-Rework-query-relation-permission-checking.patch text/x-diff 139.9 KB
v28-0002-rename-GetRelAllUpdatedCols-get_rel_all_updated_.patch text/x-diff 6.0 KB
v28-0003-code-style.patch text/x-diff 1.2 KB
v28-0004-pgindent.patch text/x-diff 22.7 KB
v28-0005-setrefs-split-out-addition-of-PermInfo-to-flat-l.patch text/x-diff 2.1 KB
v28-0006-wrap-line.patch text/x-diff 881 bytes
v28-0007-rename-PlannedStmt-rtepermlist-rtablePermInfos.patch text/x-diff 4.1 KB
v28-0008-wrap-lines.patch text/x-diff 1.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dean Rasheed 2022-11-29 10:11:00 Re: Non-decimal integer literals
Previous Message Yuya Watari 2022-11-29 08:58:25 Re: [PoC] Reducing planning time when tables have many partitions