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
