public inbox for [email protected]  

help / color / mirror / Atom feed

From: Jeff Davis <[email protected]>
To: Nathan Bossart <[email protected]>
To: Corey Huinker <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Ayush Vatsa <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Clarification on Role Access Rights to Table Indexes
Date: Mon, 13 Oct 2025 19:23:36 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <aO1TaPd0YesHy5Sn@nathan>
References: <Z88CB-vDehJ9rW8u@nathan>
	<aNQVIVKarUipPcnW@nathan>
	<[email protected]>
	<aNQhuRQfD3PlpeuT@nathan>
	<[email protected]>
	<[email protected]>
	<aOfXNAFkj_EFm-8q@nathan>
	<aOgmi6avE6qMw_6t@nathan>
	<aOkzoH-pXdBr0ewf@nathan>
	<[email protected]>
	<aO1TaPd0YesHy5Sn@nathan>

On Mon, 2025-10-13 at 14:30 -0500, Nathan Bossart wrote:
> * 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.

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

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

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

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.  I don't intend to back-
> patch
> this one.

Looks good.

Regards,
	Jeff Davis

view thread (12+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Clarification on Role Access Rights to Table Indexes
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox