From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Ayush Vatsa <ayushvatsa1810@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Clarification on Role Access Rights to Table Indexes
Date: Wed, 24 Sep 2025 10:58:25 -0500
Message-ID: <aNQVIVKarUipPcnW@nathan> (raw)
In-Reply-To: <Z88CB-vDehJ9rW8u@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>
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
Attachments:
[text/plain] v3-0001-fix-priv-checks-in-stats-code.patch (0B, ../aNQVIVKarUipPcnW@nathan/2-v3-0001-fix-priv-checks-in-stats-code.patch)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-general@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, ayushvatsa1810@gmail.com, robertmhaas@gmail.com, david.g.johnston@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Clarification on Role Access Rights to Table Indexes
In-Reply-To: <aNQVIVKarUipPcnW@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox