Re: Inval reliability, especially for inplace updates - Mailing 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