Re: Inval reliability, especially for inplace updates - Mailing list pgsql-hackers
From | Nitin Motiani |
---|---|
Subject | Re: Inval reliability, especially for inplace updates |
Date | |
Msg-id | CAH5HC94r2ZKy_MhNaPUjn7NHkOFgbyBAgV2jaYSRt35BNWuOAA@mail.gmail.com Whole thread Raw |
In response to | Re: Inval reliability, especially for inplace updates (Noah Misch <noah@leadboat.com>) |
Responses |
Re: Inval reliability, especially for inplace updates
|
List | pgsql-hackers |
On Sun, Oct 13, 2024 at 6:15 AM Noah Misch <noah@leadboat.com> wrote: > > 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. > Thanks for the clarification. > > 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. > Thanks. I would have probably done it using the NeedsInvalidateHeapTuple. But I don't have a strong enough preference to change it from the callback way. So the current approach seems good. > > 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. > Sure. Can be done in a different thread. Thanks, Nitin Motiani Google
pgsql-hackers by date: