Date: Sat, 11 Jan 2025 17:26:47 -0800
From: Noah Misch <noah@leadboat.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Xiaoran Wang <fanfuxiaoran@gmail.com>,
	pgsql-hackers@lists.postgresql.org,
	Andres Freund <andres@anarazel.de>
Subject: Re: Recovering from detoast-related catcache invalidations
Message-ID: <20250112012647.14.nmisch@google.com>
References: <754826.1705090452@sss.pgh.pa.us>
 <793469.1705092433@sss.pgh.pa.us>
 <20240114201411.d0@rfd.leadboat.com>
 <20240924212036.0a.nmisch@google.com>
 <ea198f85-5df0-464d-8a0a-45bf88166c33@iki.fi>
 <1066416.1734103822@sss.pgh.pa.us>
 <2234dc98-06fe-42ed-b5db-ac17384dc880@iki.fi>
 <1f11aacf-968c-4b8d-a917-31cb207b7a56@iki.fi>
 <20250107215653.82.nmisch@google.com>
 <5ef69a0d-8e8f-4104-a770-c77b1872d86d@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5ef69a0d-8e8f-4104-a770-c77b1872d86d@iki.fi>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250112012647.14.nmisch%40google.com>
Precedence: bulk

On Thu, Jan 09, 2025 at 11:39:53AM +0200, Heikki Linnakangas wrote:
> On 07/01/2025 23:56, Noah Misch wrote:
> > On Tue, Dec 24, 2024 at 12:18:09AM +0200, Heikki Linnakangas wrote:
> > > I'm thinking of the attached to fix this. It changes the strategy for
> > > detecting concurrent cache invalidations. Instead of the "recheck" mechanism
> > > that was introduced in commit ad98fb1422, keep a stack of "build
> > > in-progress" entries, and CatCacheInvalidate() invalidate those
> > > "in-progress" entries in addition to the actual CatCTup and CatCList
> > > entries.
> > > 
> > > My first attempt was to insert the CatCTup or CatCList entry to the catcache
> > > before starting to build it, marked with a flag to indicate that the entry
> > > isn't fully built yet. But when I started to write that it got pretty
> > > invasive, and it felt easier to add another structure to hold the
> > > in-progress entries instead.
> > 
> > That's similar to how relcache has been doing it (in_progress_list).  I see no
> > problem applying that technique here.
> 
> Oh thanks, I didn't notice that in relcache.c. I adjusted the naming and
> comments in the new catcache.c code to be a little closer to the relcache.c
> version, just to make it a bit more consistent.
> 
> The main difference is that relcache.c uses an array and an end-of-xact
> callback to reset the array on error, while my implementation uses a linked
> list of entries allocated on stack and PG_TRY-CATCH for error cleanup. I
> considered adopting relcache.c's approach for the sake of consistency, but
> decided to keep my original approach in the end. Some of the code in
> catcache.c already had a suitable PG_TRY-CATCH block and I didn't want to
> add a new end-of-xact callback just for this.

That difference is reasonable.  catcache will add PG_TRY overhead only in the
HEAP_HASEXTERNAL case, which is not a normal benchmark situation.  If relcache
were to adopt the PG_TRY approach, it would be harder to rule out the
significance of the overhead.  So a long-term state of using both designs is
reasonable.  It's not a mere historical accident.

> > gcc 4.8.5 warns:
> > 
> > catcache.c: In function ‘CatalogCacheCreateEntry’:
> > catcache.c:2159:29: warning: ‘dtp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >     ct->tuple.t_tableOid = dtp->t_tableOid;
> > 
> > I remember some commit of a gcc 4.x warning fix in a recent year, and we do
> > have buildfarm representation.  Consider silencing it.
> 
> Hmm, I guess the compiler doesn't see that it's initialized with all the
> PG_TRY()/CATCH() stuff. I initialized it to NULL to silence the warning.

It's warning-free now.

> @@ -697,9 +725,14 @@ CreateCacheMemoryContext(void)
>   *
>   * This is not very efficient if the target cache is nearly empty.
>   * However, it shouldn't need to be efficient; we don't invoke it often.
> + *
> + * If 'debug_discard' is true, we are being called as part of
> + * debug_discard_caches.  In that case, the cache not reset for correctness,

s/cache not/cache is not/ or similar

> +			PG_TRY();
>  			{
> -				matches = equalTuple(before, ntp);
> -				heap_freetuple(before);
> +				dtp = toast_flatten_tuple(ntp, cache->cc_tupdesc);
>  			}
> -			if (!matches || !systable_recheck_tuple(scandesc, ntp))
> +			PG_FINALLY();
>  			{
> -				heap_freetuple(dtp);

Is this an intentional removal of the heap_freetuple(dtp) before return NULL?

> -				return NULL;
> +				Assert(catcache_in_progress_stack == &in_progress_ent);
> +				catcache_in_progress_stack = save_in_progress;
>  			}
> +			PG_END_TRY();
> +
> +			if (in_progress_ent.dead)
> +				return NULL;
...
> +$node->append_conf(
> +	'postgresql.conf', qq{
> +debug_discard_caches=1
> +});

I'd drop this debug_discard_caches.  debug_discard_caches buildfarm runs will
still test it, so let's have normal runs test the normal case.