Date: Wed, 24 Sep 2025 10:58:25 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Ayush Vatsa <ayushvatsa1810@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	"David G. Johnston" <david.g.johnston@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Clarification on Role Access Rights to Table Indexes
Message-ID: <aNQVIVKarUipPcnW@nathan>
References: 
 <CACX+KaNAbOzePn710EtzH9F5xiUdBC+u59=UMab=Wr8jgDKQtw@mail.gmail.com>
 <Z8dcGMMP3-D5dobY@nathan>
 <CACX+KaO4R9QDxbPSxSB0jNXFsqA6Jf=UPS+tyUvT_YvuP_grVA@mail.gmail.com>
 <Z8yxsm9ZWVkHlPbV@nathan>
 <CACX+KaP+6U9jf=GT4wpR7TvRvSMtTAhz=vP2Zr+ZdUFVZzqNsA@mail.gmail.com>
 <Z8y9RTT-vU6oVI_Y@nathan>
 <149429.1741472260@sss.pgh.pa.us>
 <Z8zwVmGzXyDdkAXj@nathan>
 <279947.1741535285@sss.pgh.pa.us>
 <Z88CB-vDehJ9rW8u@nathan>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="31qWFSWVZhdEZdR9"
Content-Disposition: inline
In-Reply-To: <Z88CB-vDehJ9rW8u@nathan>
Archived-At: <https://www.postgresql.org/message-id/aNQVIVKarUipPcnW%40nathan>
Precedence: bulk


--31qWFSWVZhdEZdR9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Mar 10, 2025 at 10:15:19AM -0500, Nathan Bossart wrote:
> On Sun, Mar 09, 2025 at 11:48:05AM -0400, Tom Lane wrote:
>> Nathan Bossart <nathandbossart@gmail.com> writes:
>>> On Sat, Mar 08, 2025 at 05:17:40PM -0500, Tom Lane wrote:
>>>> ReindexIndex() faces this same problem and solves it with some
>>>> very complex code that manages to get the table's lock first.
>> 
>>> I noticed that amcheck's bt_index_check_internal() handles this problem,
>>> ...
>>> stats_lock_check_privileges() does something similar, but it's not as
>>> cautious about the "heapid != IndexGetRelation(indrelid, false)" race
>>> condition.
>> 
>> Egad, we've already got three inconsistent implementations of this
>> functionality?  I think the first step must be to unify them into
>> a common implementation, if at all possible.
> 
> Agreed.  I worry that trying to unify each bespoke implementation into a
> single function might result in an unwieldy mess, but I'll give it a
> shot...

I tried to unify these, but each one seems to be just different enough to
make it not worth the trouble.  Instead, I took a look at each
implementation:

* amcheck's amcheck_lock_relation_and_check() seems correct to me.

* stats_lock_check_privileges() appears to be missing the second
IndexGetRelation() check after locking the table and index, so I added
that in 0001.  Since this code is new to v18, I proposed to back-patch 0001
there.

* RangeVarCallbackForReindexIndex() was checking privileges on the table
before locking it, so I reversed it in 0002.  Interestingly, this caused
test errors because LockRelationOid() checks for invalidation messages, so
the pg_class_aclcheck() call started failing with unhelpful errors due to
concurrently dropped relations.  To deal with that, I switched to
pg_class_aclcheck_ext() so that we can handle missing relations.
Furthermore, I noticed that this callback seems to assume that as long as
the index does not change between calls, its table won't, either.  That's
probably always true in practice, but even if it's completely true, I see
no reason to rely on it.  So, I simplified the code to unconditionally
unlock any previously-locked table and to lock whatever IndexGetRelation()
returns.  This could probably be back-patched, but in the absence of any
reports or any reproducible bugs, I don't think we should.

* 0003 fixes pg_prewarm's privilege checks by following a similar pattern.
This probably ought to get back-patched to all supported versions.

-- 
nathan

--31qWFSWVZhdEZdR9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v3-0001-fix-priv-checks-in-stats-code.patch