Message-ID: <bd0826223bff21bd2ad3d0b1520ee8429568c526.camel@j-davis.com>
Subject: Re: Clarification on Role Access Rights to Table Indexes
From: Jeff Davis <pgsql@j-davis.com>
To: Nathan Bossart <nathandbossart@gmail.com>, Corey Huinker
	 <corey.huinker@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Ayush Vatsa <ayushvatsa1810@gmail.com>, 
 Robert Haas <robertmhaas@gmail.com>, "David G. Johnston"
 <david.g.johnston@gmail.com>, PostgreSQL Hackers
 <pgsql-hackers@postgresql.org>
Date: Mon, 13 Oct 2025 19:23:36 -0700
In-Reply-To: <aO1TaPd0YesHy5Sn@nathan>
References: <Z88CB-vDehJ9rW8u@nathan> <aNQVIVKarUipPcnW@nathan>
	 <3432170.1758730414@sss.pgh.pa.us> <aNQhuRQfD3PlpeuT@nathan>
	 <8af53c6e8992aa706e63aafe60a3bcf100b524d1.camel@j-davis.com>
	 <7b0e2774cdcc8f522ac82f64a8d7266f353a5094.camel@j-davis.com>
	 <aOfXNAFkj_EFm-8q@nathan> <aOgmi6avE6qMw_6t@nathan>
	 <aOkzoH-pXdBr0ewf@nathan>
	 <31a67adbb10b85ff7cddeafe75b9f6505c902e57.camel@j-davis.com>
	 <aO1TaPd0YesHy5Sn@nathan>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.3-0ubuntu1 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/bd0826223bff21bd2ad3d0b1520ee8429568c526.camel%40j-davis.com>
Precedence: bulk

On Mon, 2025-10-13 at 14:30 -0500, Nathan Bossart wrote:
> * 0001 moves the code for stats clearing/setting to use
> RangeVarGetRelidExtended().=C2=A0 The existing code looks up the relation=
,
> locks
> it, and then checks permissions.=C2=A0 There's no guarantee that the
> relation
> you looked up didn't concurrently change before locking, and locking
> before
> privilege checks is troublesome from a DOS perspective.=C2=A0 One downsid=
e
> of
> using RangeVarGetRelidExtended() is that we can't use AccessShareLock
> for
> regular indexes, but I'm not sure that's really a problem since we're
> already using ShareUpdateExclusiveLock for everything else.=C2=A0 The
> RangeVarGetRelidExtended() callback is similar to the one modified by
> 0002.
> This should be back-patched to v18.

We tried to match the locking behavior for analyze. Originally, that's
because we were using in-place updates, which required those specific
kinds of locks. Now that the in-place code is gone, then I think it's
OK to use ShareUpdateExclusiveLock for indexes, too, but it is a
notable difference in behavior.

Including Corey in case he has comments.

As for the patch itself, it looks good to me. Stylistically I might
have kept the "index_oid" variable, which makes some of the tests a bit
clearer, but I don't have a strong opinion.

The unlikely scenarios are a bit confusing. I'd probably error for
either case. Also, the error message on the second scenario is wrong if
the previous lookup was a table, I think.

> * 0002 fixes the RangeVarGetRelidExtended() callback for REINDEX
> INDEX to
> handle unlikely scenarios involving OID reuse (e.g., lookup returns
> the
> same index OID for a different table).=C2=A0 I did confirm there was a bu=
g
> here
> by concurrently re-creating an index with the same OID for a heap
> with a
> different OID (via the pg_upgrade support functions).=C2=A0 In previous
> versions
> of this patch, I tried to fix this by unconditionally unlocking the
> heap at
> the beginning of the callback, but upon further inspection, I noticed
> that
> creates deadlock hazards because we might've already locked the
> index.=C2=A0 (We
> need to lock the heap first.)=C2=A0 In v6, I've just added an ERROR for
> these
> extremely unlikely scenarios.=C2=A0 I've also replaced all early returns
> in this
> function with ERRORs (except for the invalid relId case).

+1 for throwing errors when we have race conditions combined with name
reuse. Looks fine to me.

>=20
> * 0003 fixes the privilege checks in pg_prewarm by using a similar
> approach
> to amcheck_lock_relation_and_check().=C2=A0 This seems correct to me, but
> it
> does add more locking.=C2=A0 This should be back-patched to v13.

IIUC this is locking before the privilege check. Is there a reason why
we think this is OK here (and in amcheck_lock_relation_and_check()) but
not for the stats?

> * 0004 is a small patch to teach dblink to use
> RangeVarGetRelidExtended().
> I believe this code predates that function.=C2=A0 I don't intend to back-
> patch
> this one.

Looks good.

Regards,
	Jeff Davis