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