Horton

Re: Inval reliability, especially for inplace updates

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Alexander Lakhin <exclusion@gmail.com>, Nitin Motiani <nitinmotiani@google.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Date: 2024-11-01T23:38:29Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Update .abi-compliance-history for PrepareToInvalidateCacheTuple().

  2. Assert lack of hazardous buffer locks before possible catalog read.

  3. For inplace update, send nontransactional invalidations.

  4. Update .abi-compliance-history for CacheInvalidateHeapTupleInplace().

  5. Revisit cosmetics of "For inplace update, send nontransactional invalidations."

  6. Correct comments of "Fix data loss at inplace update after heap_update()".

  7. Move I/O before the index_update_stats() buffer lock region.

  8. Revert "For inplace update, send nontransactional invalidations."

  9. Revert "WAL-log inplace update before revealing it to other sessions."

  10. Fix inplace update buffer self-deadlock.

  11. Remove duplicate words in comments

  12. At end of recovery, reset all sinval-managed caches.

  13. Fix data loss at inplace update after heap_update().

  14. Remove comment about xl_heap_inplace "AT END OF STRUCT".

  15. Reduce memory consumption for pending invalidation messages.

  16. Add support for streaming to built-in logical replication.

  17. WAL Log invalidations at command end with wal_level=logical.

  18. Introduce logical decoding.

  19. Rename and document some invalidation routines to make it clearer that

Attachments

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