Date: Sat, 12 Oct 2024 17:45:11 -0700
From: Noah Misch <noah@leadboat.com>
To: Nitin Motiani <nitinmotiani@google.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Subject: Re: Inval reliability, especially for inplace updates
Message-ID: <20241013004511.9c.nmisch@google.com>
References: <20240523000548.58.nmisch@google.com>
 <20240615223718.42.nmisch@google.com>
 <20240617235854.f8.nmisch@google.com>
 <20240618015730.jijhox2a3w3t26en@awork3.anarazel.de>
 <20240618152349.7f.nmisch@google.com>
 <20240831010711.67.nmisch@google.com>
 <20240930193350.04.nmisch@google.com>
 <CAH5HC9508Xvz4BVca8OCoDOv7pgHqAf85EqZkWMZ_Q2hAkebzg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAH5HC9508Xvz4BVca8OCoDOv7pgHqAf85EqZkWMZ_Q2hAkebzg@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20241013004511.9c.nmisch%40google.com>
Precedence: bulk

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