Date: Mon, 13 Oct 2025 14:30:48 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.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>
Subject: Re: Clarification on Role Access Rights to Table Indexes
Message-ID: <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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="iujXWOSrXz0NaSpd"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <31a67adbb10b85ff7cddeafe75b9f6505c902e57.camel@j-davis.com>
Archived-At: <https://www.postgresql.org/message-id/aO1TaPd0YesHy5Sn%40nathan>
Precedence: bulk


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

On Fri, Oct 10, 2025 at 11:31:03AM -0700, Jeff Davis wrote:
> On Fri, 2025-10-10 at 11:26 -0500, Nathan Bossart wrote:
>> After sleeping on it, I still think this is the right call.  In any
>> case, I've spent way too much time on this stuff, so I plan to commit
>> the attached soon.
> 
> I'm OK with that. v5-0001 is an improvement over the current situation.

Okay, I lied.  I spent even more time on these patches and came up with the
attached.  Here's a summary of what's going on:

* 0001 moves the code for stats clearing/setting to use
RangeVarGetRelidExtended().  The existing code looks up the relation, locks
it, and then checks permissions.  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.  One downside 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.  The
RangeVarGetRelidExtended() callback is similar to the one modified by 0002.
This should be back-patched to v18.

* 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).  I did confirm there was a bug here
by concurrently re-creating an index with the same OID for a heap with a
different OID (via the pg_upgrade support functions).  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.  (We
need to lock the heap first.)  In v6, I've just added an ERROR for these
extremely unlikely scenarios.  I've also replaced all early returns in this
function with ERRORs (except for the invalid relId case).  AFAICT the extra
checks are unecessary, and even if they were necessary, I think they break
some of the code related to heap locking in subtle ways.  Some callbacks do
these extra checks, and others do not, and AFAIK there haven't been any
reported problems either way.  0002 should be back-patched to v13, but it
will look a little different on v16 and newer, i.e., before MAINTAIN was
added.

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

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

-- 
nathan

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