Feedback-ID: ia2694551:Fastmail
Date: Wed, 16 Nov 2022 12:44:02 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Ian Lawrence Barwick <barwick@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	David Rowley <dgrowleyml@gmail.com>, Greg Stark <stark@mit.edu>,
	Zhihong Yu <zyu@yugabyte.com>, Julien Rouhaud <rjuju123@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: ExecRTCheckPerms() and many prunable partitions
Message-ID: <20221116114402.merw5gdwkzlc35t4@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3sqhherzatux5zde"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20221110115801.qnsaci3viwlcpy42@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/20221116114402.merw5gdwkzlc35t4%40alvherre.pgsql>
Precedence: bulk


--3sqhherzatux5zde
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2022-Nov-10, Alvaro Herrera wrote:

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

I hadn't looked hard enough.  This is already in GetRTEPermissionInfo().


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

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'll have a look at 0002 now.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

--3sqhherzatux5zde
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v25-0001-Rework-query-relation-permission-checking.patch"