Re: Inval reliability, especially for inplace updates

On Thu, Oct 31, 2024 at 09:20:52PM -0700, Noah Misch wrote:
> Here, one of the autovacuum workers had the guilty stack trace, appearing at
> the end of this message. heap_inplace_update_and_unlock() calls
> CacheInvalidateHeapTupleInplace() while holding BUFFER_LOCK_EXCLUSIVE on a
> buffer of pg_class. CacheInvalidateHeapTupleInplace() may call
> CatalogCacheInitializeCache(), which opens the cache's rel. If there's not a
> valid relcache entry for the catcache's rel, we scan pg_class to make a valid
> relcache entry. The ensuing hang makes sense.
>
> Tomorrow, I'll think more about fixes. Two that might work:
>
> 1. Call CacheInvalidateHeapTupleInplace() before locking the buffer. Each
> time we need to re-find the tuple, discard the previous try's inplace
> invals and redo CacheInvalidateHeapTupleInplace(). That's because
> concurrent activity may have changed cache key fields like relname.

Attached. With this, I got no hangs in 1.9h of your test procedure. Without
the patch, I got fourteen hangs in the same amount of time.

> 2. Add some function that we call before locking the buffer. Its charter is
> to ensure PrepareToInvalidateCacheTuple() won't have to call
> CatalogCacheInitializeCache().

The existing InitCatalogCachePhase2() could satisfy that charter. I liked
this a little less than (1), for two reasons. First, confirming that it
avoids deadlocks requires thinking through more of the catcache and relcache
procedures. Second, InitCatalogCachePhase2() would init unnecessary caches.

> I think nothing resets catcache to the
> extent that CatalogCacheInitializeCache() must happen again, so this should
> suffice regardless of concurrent sinval traffic, debug_discard_caches, etc.

The comment at CatalogCacheFlushCatalog() confirms that.

> What else is worth considering? Any preferences among those?

3. Let a process take LW_SHARED if it already holds LW_EXCLUSIVE.

That would change outcomes well beyond inplace update, and it feels like a
bandage for doing things in a suboptimal order. (1) or (2) would be an
improvement even if we did (3), since they would reduce I/O done while holding
BUFFER_LOCK_EXCLUSIVE.

This was a near miss to having a worst-in-years regression in a minor release,
so I'm proposing this sequence:

- Revert from non-master branches commits 8e7e672 (inplace180, "WAL-log
inplace update before revealing it to other sessions.") and 243e9b4
(inplace160, "For inplace update, send nontransactional invalidations.").

- Back-patch inplace230-index_update_stats-io-before-buflock to harden commit
a07e03f (inplace110, "Fix data loss at inplace update after heap_update()").

- Push attached inplace240 to master.

- Make the commitfest entry a request for review of v17 inplace160+inplace240.
After some amount of additional review and master bake time, the reverted
patches would return to non-master branches.

If someone agrees or if nobody objects by 2024-11-02T15:00+0000, I'll make it
so. That's not much time, but I want to minimize buildfarm members hanging
and maximize inplace230 bake time before the release wrap.

Less urgently, we likely should add defense in depth against adding
rarely-reached LWLock deadlocks. Perhaps one or both of:

- Assert-fail if entering a conditional CatalogCacheInitializeCache() caller
(e.g. SearchCatCacheInternal()) while holding a catalog buffer lock.

- elog(ERROR) instead of sleeping in LWLockAcquire() if the current process is
the current lock holder.

Thanks,
nm