Date: Tue, 7 Jan 2025 13:56:53 -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: <20250107215653.82.nmisch@google.com>
References: <329632.1705011663@sss.pgh.pa.us>
 <CAGjhLkOFmzx9gbB8qudKxdg8UR0SApo-thKuPZ=1zJaZ6-ORXQ@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1f11aacf-968c-4b8d-a917-31cb207b7a56@iki.fi>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250107215653.82.nmisch%40google.com>
Precedence: bulk

On Tue, Dec 24, 2024 at 12:18:09AM +0200, Heikki Linnakangas wrote:
> On 14/12/2024 02:06, Heikki Linnakangas wrote:
> > Ok, I missed that. It does not handle the 2nd scenario though: If a new
> > catalog tuple is concurrently inserted that should be part of the list,
> > it is missed.

> > Attached is an injection point test case to reproduce that. If you
> > change the test so that the function's body is shorter, so that it's not
> > toasted, the test passes.

Nice discovery.

> 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.

> not sure I want to
> commit the test with the injection point, but it's useful now to demonstrate
> the bug.)

I'd err on the side of including it.  Apart from some copied comments, the
test looks ready.

On Wed, Dec 25, 2024 at 11:27:38AM +0200, Heikki Linnakangas wrote:
> +++ b/src/test/modules/test_misc/t/007_bugs.pl

test_misc/t/007_bugs.pl could be home to almost anything.  How about naming it
007_catcache_inval.pl?

> @@ -744,6 +770,13 @@ ResetCatalogCache(CatCache *cache)
>  #endif
>  		}
>  	}
> +
> +	/* Also invalidate any entries that are being built */
> +	for (CatCCreating *e = catcache_creating_stack; e != NULL; e = e->next)
> +	{
> +		if (e->cache == cache)
> +			e->dead = true;
> +	}
>  }

With debug_discard_caches=1, "make check" hangs early with some INSERT using
100% CPU.  The new test file does likewise.  I bet this needs a special case
to short-circuit debug_discard_caches, like RelationCacheInvalidate() has.

> @@ -1665,6 +1698,8 @@ SearchCatCacheList(CatCache *cache,
>  	HeapTuple	ntp;
>  	MemoryContext oldcxt;
>  	int			i;
> +	volatile CatCCreating creating_list;

You could drop the volatile by copying catcache_creating_stack to an
additional var that you reference after longjmp, instead of referencing
creating_list.next after longjmp.  Likewise for the instance of volatile in
CatalogCacheCreateEntry().
https://pubs.opengroup.org/onlinepubs/9799919799/functions/longjmp.html has
the rules.  Using volatile is fine, though.

> +	bool		first_iter = true;

This is okay as non-volatile, but it could just as easily move inside the
PG_TRY.

> @@ -2076,38 +2118,33 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, SysScanDesc scandesc,

> +			PG_TRY();
>  			{
> -				matches = equalTuple(before, ntp);
> -				heap_freetuple(before);
> +				dtp = toast_flatten_tuple(ntp, cache->cc_tupdesc);

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.


The rest looks good.  Thanks.