Horton

Re: Inval reliability, especially for inplace updates

Nitin Motiani <nitinmotiani@google.com>

View thread
From: Nitin Motiani <nitinmotiani@google.com>
To: Noah Misch <noah@leadboat.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Date: 2024-10-20T13:11:37Z
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 Mon, Oct 14, 2024 at 3:15 PM Nitin Motiani <nitinmotiani@google.com> wrote:
>
> 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.
>

I tested the patch locally and it works. And I have no other question
regarding the structure. So this patch looks good to me to commit.

Thanks,
Nitin Motiani
Google