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>, Zhihong Yu <zyu(at)yugabyte(dot)com>, 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-10 11:58:01
Message-ID: 20221110115801.qnsaci3viwlcpy42@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


I've been trying to understand what 0001 does. One thing that strikes
me is that it seems like it'd be easy to have bugs because of modifying
the perminfo list inadequately. I couldn't find any cross-check that
some perminfo element that we obtain for a rte does actually match the
relation we wanted to check. Maybe we could add a test in some central
place that perminfo->relid equals rte->relid?

A related point is that concatenating lists doesn't seem to worry about
not processing one element multiple times and ending up with bogus offsets.
(I suppose you still have to let an element be processed multiple times
in case you have nested subqueries? I wonder how good is the test
coverage for such scenarios.)

Why do callers of add_rte_to_flat_rtable() have to modify the rte's
perminfoindex themselves, instead of having the function do it for them?
That looks strange. But also it's odd that flatten_unplanned_rtes
concatenates the two lists after all the perminfoindexes have been
modified, rather than doing both things (adding each RTEs perminfo to
the global list and offsetting the index) as we walk the list, in
flatten_rtes_walker. It looks like these two actions are disconnected
from one another, but unless I misunderstand, in reality the opposite is

I think the API of ConcatRTEPermissionInfoLists is a bit weird. Why not
have the function return the resulting list instead, just like
list_append? It is more verbose, but it seems easier to grok.

Two trivial changes attached. (Maybe 0002 is not correct, if you're
also trying to reference finalrtepermlist; but in that case I think the
original may have been misleading as well.)

Álvaro Herrera 48°01'N 7°57'E —

Attachment Content-Type Size
0002-Simplify-comment-a-little-bit.patch.txt text/plain 906 bytes
0001-fix-typo.patch.txt text/plain 933 bytes

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Hamid Akhtar 2022-11-10 12:01:15 Re: Allow pageinspect's bt_page_stats function to return a set of rows instead of a single row
Previous Message Ranier Vilela 2022-11-10 11:56:25 Re: Avoid overhead open-close indexes (catalog updates)