Re: ExecRTCheckPerms() and many prunable partitions

Hi Alvaro,

On Thu, Nov 10, 2022 at 8:58 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Hello
>
> I've been trying to understand what 0001 does.

Thanks a lot for taking a look.

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

Could you please clarify what you mean by "an element" here? Are you
are implying that any given relation, no matter how many times it
occurs in a query (possibly via view rule expansion), should end up
with only one RTEPermissionInfo node covering all its occurrences in
the combined/final rtepermlist, as a result of concatenation/merging
of rtepermlists of different Queries? Versions of the patch prior to
v17 did it that way, but Tom didn't like that approach much [1], so I
switched to the current implementation where the merging of two
Queries' range tables using list_concat() is accompanied by the
merging of their rtepermlists, again, using list_concat(). So, if the
same table has multiple RTEs in a query, so will there be multiple
corresponding RTEPermissionInfos.

> (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.)

ISTM the existing tests cover a good portion of the changes being made
here, but I guess I'm only saying that because I have spent a
non-trivial amount of time debugging the test failures across many
files over different versions of the patch, especially those that
involve views.

Do you think it might be better to add some new tests as part of this
patch than simply relying on the existing tests not failing?

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

OK, I have moved the step of updating perminfoindex into
add_rte_to_flat_rtable(), where it looks up the RTEPermissionInfo for
an RTE being added using GetRTEPermissionInfo() and lappend()'s it to
finalrtepermlist before updating the index. For flatten_rtes_walker()
then to rely on that facility, I needed to make some changes to its
arguments to pass the correct Query node to pick the rtepermlist from.
Overall, setrefs.c changes now hopefully look saner than in the last
version.

As soon as I made that change, I noticed a bunch of ERRORs in
regression tests due to the checks in GetRTEPermissionInfo(), though
none that looked like live bugs. Though I did find some others as I
was reworking the code to fix those errors, which I have fixed too.

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

Agreed, I have merged your delta patch into 0001.

On Wed, Nov 16, 2022 at 8:44 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > 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 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.
>
> Another point related to this. I noticed that everyplace we do
> ConcatRTEPermissionInfoLists, it is followed by list_append'ing the RT
> list themselves. This is strange. Maybe that's the wrong way to look
> at this, and instead we should have a function that does both things
> together: pass both rtables and rtepermlists and smash them all
> together.

OK, how does the attached 0002 look in that regard? In it, I have
renamed ConcatRTEPermissionInfoLists() to CombineRangeTables() which
does all that. Though, given the needs of rewriteRuleAction(), the
API of it may look a bit weird. (Only posting it separately for the
ease of comparison.)

> I attach your 0001 again with a bunch of other fixups (I don't include
> your 0002ff). I've pushed this to see the CI results, and so far it's
> looking good (hasn't finished yet though):
> https://cirrus-ci.com/build/5126818977021952

I have merged all.

While working on these changes, I realized that 0002 (the patch to
remove OLD/NEW RTEs from the stored view query's range table) was
going a bit too far by removing UpdateRangeTableOfViewParse()
altogether. You may have noticed that a RTE_RELATION entry for the
view relation is needed anyway for permission checking, locking, etc.
and the patch was making the rewriter add one explicitly, whereas the
OLD RTE would be playing that role previously. In the updated
version, I have decided to keep UpdateRangeTableOfViewParse() while
removing the code in it that adds the NEW RTE, which is totally
unnecessary. Also removed the rewriter changes that were needed
previously. Most of the previous version of the patch was a whole
bunch of regression test output changes, because the stored view
query's range table would be changed such that deparsed version of
those queries need no longer qualify output columns (only 1 entry in
the range table in those cases), though I didn't necessarily think
that that looked better. In the new version, because the stored view
query contains the OLD entry, those qualifications stay, and so none
of the regression test changes are necessary anymore. postgres_fdw
ones are unrelated and noted in the commit message.

[1] https://www.postgresql.org/message-id/3094251.1658955855%40sss.pgh.pa.us

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com