Feedback-ID: id4a34324:Fastmail
Date: Tue, 20 Jan 2026 13:38:39 -0500
From: Andres Freund <andres@anarazel.de>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Chao Li <li.evan.chao@gmail.com>, 
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: More speedups for tuple deformation
Message-ID: <rbskhk7scqbxqnaw4o6nh6na2ffcclg3cxn4d4cn5jfr2z7vv3@kadtz65meesb>
References: <CAApHDvpoFjaj3+w_jD5uPnGazaw41A71tVJokLDJg2zfcigpMQ@mail.gmail.com>
 <CAApHDvrF6DG7=xD8JGo2HoQKN0LRFNF0ysVt6cKSNPiqbdQOSA@mail.gmail.com>
 <CAApHDvoh3Q413szd-zsUTCpQPWNdpUYvx-fvsB8DP8zOja+ckg@mail.gmail.com>
 <9A17C43D-7A28-4885-8974-555A40C9523E@gmail.com>
 <CAApHDvqhbJU_-yF3Hbf4VhX33qXtpeYv3MsvMPDMcDwGGLr9ZQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAApHDvqhbJU_-yF3Hbf4VhX33qXtpeYv3MsvMPDMcDwGGLr9ZQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/rbskhk7scqbxqnaw4o6nh6na2ffcclg3cxn4d4cn5jfr2z7vv3%40kadtz65meesb>
Precedence: bulk

Hi,

On 2026-01-20 13:11:55 +1300, David Rowley wrote:
> I've attached the v4 patch, which also fixes the LLVM compiler warning
> that I introduced.

I wonder if it's possible to split the patch - it's big enough to be
nontrivial to review...  Perhaps the finalization could be introduced
separately from the patch actually making use of it?

I wonder if we should somehow change the API of tupledesc creation, to make
old code that doesn't have TupleDescFinalize() fail to compile, instead of
just warn...


> diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c b/contrib/pg_buffercache/pg_buffercache_pages.c
> index dcba3fb5473..2fdf5a341f6 100644
> --- a/contrib/pg_buffercache/pg_buffercache_pages.c
> +++ b/contrib/pg_buffercache/pg_buffercache_pages.c
> @@ -174,6 +174,7 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
>  			TupleDescInitEntry(tupledesc, (AttrNumber) 9, "pinning_backends",
>  							   INT4OID, -1, 0);
>  
> +		TupleDescFinalize(tupledesc);
>  		fctx->tupdesc = BlessTupleDesc(tupledesc);
>  

Think it'd be worth adding an assertion to BlessTupleDesc that
TupleDescFinalize has been called, I think that'll lead to easier to
understand backtraces in a lot of cases. Particularly if you consider cases
where BlessTupleDesc() will create a tupdesc in shared memory, that could then
trigger an assertion failure in a parallel worker or such.
>  /*
>   * slot_deform_heap_tuple
>   *		Given a TupleTableSlot, extract data from the slot's physical tuple
> @@ -1122,78 +1010,167 @@ static pg_attribute_always_inline void
>  slot_deform_heap_tuple(TupleTableSlot *slot, HeapTuple tuple, uint32 *offp,
>  					   int natts)
>  {
> +	CompactAttribute *cattr;
> +	TupleDesc	tupleDesc = slot->tts_tupleDescriptor;
>  	bool		hasnulls = HeapTupleHasNulls(tuple);
> +	HeapTupleHeader tup = tuple->t_data;
> +	bits8	   *bp;				/* ptr to null bitmap in tuple */
>  	int			attnum;
> +	int			firstNonCacheOffsetAttr;
> +
> +#ifdef OPTIMIZE_BYVAL
> +	int			firstByRefAttr;
> +#endif
> +	int			firstNullAttr;
> +	Datum	   *values;
> +	bool	   *isnull;
> +	char	   *tp;				/* ptr to tuple data */
>  	uint32		off;			/* offset in tuple data */
> -	bool		slow;			/* can we use/set attcacheoff? */
> +
> +	/* Did someone forget to call TupleDescFinalize()? */
> +	Assert(tupleDesc->firstNonCachedOffAttr >= 0);
>  
>  	/* We can only fetch as many attributes as the tuple has. */
> -	natts = Min(HeapTupleHeaderGetNatts(tuple->t_data), natts);
> +	natts = Min(HeapTupleHeaderGetNatts(tup), natts);
> +	attnum = slot->tts_nvalid;
> +	firstNonCacheOffsetAttr = Min(tupleDesc->firstNonCachedOffAttr, natts);
> +
> +	if (hasnulls)
> +	{
> +		bp = tup->t_bits;
> +		firstNullAttr = first_null_attr(bp, natts);
> +		firstNonCacheOffsetAttr = Min(firstNonCacheOffsetAttr, firstNullAttr);
> +	}
> +	else
> +	{
> +		bp = NULL;
> +		firstNullAttr = natts;
> +	}
> +
> +#ifdef OPTIMIZE_BYVAL
> +	firstByRefAttr = Min(firstNonCacheOffsetAttr, tupleDesc->firstByRefAttr);
> +#endif
> +	values = slot->tts_values;
> +	isnull = slot->tts_isnull;
> +	tp = (char *) tup + tup->t_hoff;
> +
> +#ifdef OPTIMIZE_BYVAL
>  
>  	/*
> -	 * Check whether the first call for this tuple, and initialize or restore
> -	 * loop state.
> +	 * Many tuples have leading byval attributes, try and process as many of
> +	 * those as possible with a special loop that can't handle byref types.
>  	 */
> -	attnum = slot->tts_nvalid;
> -	if (attnum == 0)
> +	if (attnum < firstByRefAttr)
> +	{
> +		/* Use do/while as we already know we need to loop at least once. */
> +		do
> +		{
> +			cattr = TupleDescCompactAttr(tupleDesc, attnum);
> +
> +			Assert(cattr->attcacheoff >= 0);
> +
> +			/*
> +			 * Hard code byval == true to allow the compiler to remove the
> +			 * byval check when inlining fetch_att().
> +			 */

Maybe add an assert for cattr->attbyval? Just to avoid a bad debugging
experience if somebody tries to extend this logic to
e.g. non-null-fixed-width-byref columns?

I also wonder if we could have assert-only crosschecking of the "real" offsets
against the cached ones?

Greetings,

Andres Freund