Horton

Re: Inval reliability, especially for inplace updates

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Nitin Motiani <nitinmotiani@google.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Date: 2024-10-13T00:45:11Z
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

On Sat, Oct 12, 2024 at 06:05:06PM +0530, Nitin Motiani wrote:
> 1. In heap_inplace_update_and_unlock, currently both buffer and tuple
> are unlocked outside the critical section. Why do we have to move the
> buffer unlock within the critical section here? My guess is that it
> needs to be unlocked for the inplace invals to be processed. But what
> is the reasoning behind that?

AtInplace_Inval() acquires SInvalWriteLock.  There are two reasons to want to
release the buffer lock before acquiring SInvalWriteLock:

1. Otherwise, we'd need to maintain the invariant that no other part of the
   system tries to lock the buffer while holding SInvalWriteLock.  (That would
   cause an undetected deadlock.)

2. Concurrency is better if we release a no-longer-needed LWLock before doing
   something time-consuming, like acquiring another LWLock potentially is.

Inplace invals do need to happen in the critical section, because we've
already written the change to shared buffers, making it the new authoritative
value.  If we fail to invalidate, other backends may continue operating with
stale caches.

> 2. Is there any benefit in CacheInvalidateHeapTupleCommon taking the
> preapre_callback argument? Wouldn't it be simpler to just pass an
> InvalidationInfo* to the function?

CacheInvalidateHeapTupleCommon() has three conditions that cause it to return
without invoking the callback.  Every heap_update() calls
CacheInvalidateHeapTuple().  In typical performance-critical systems, non-DDL
changes dwarf DDL.  Hence, the overwhelming majority of heap_update() calls
involve !IsCatalogRelation().  I wouldn't want to allocate InvalidationInfo in
DDL-free transactions.  To pass in InvalidationInfo*, I suppose I'd move those
three conditions to a function and make the callers look like:

CacheInvalidateHeapTuple(Relation relation,
						 HeapTuple tuple,
						 HeapTuple newtuple)
{
	if (NeedsInvalidateHeapTuple(relation))
		CacheInvalidateHeapTupleCommon(relation, tuple, newtuple,
									   PrepareInvalidationState());
}

I don't have a strong preference between that and the callback way.

> Also is inval-requires-xid-v0.patch planned to be fixed up to inplace160?

I figure I'll pursue that on a different thread, after inplace160 and
inplace180.  If there's cause to pursue it earlier, let me know.

Thanks,
nm