Date: Thu, 24 Mar 2022 15:32:49 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Japin Li <japinli@hotmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: turn fastgetattr and heap_getattr to inline functions
Message-ID: <202203241432.mt44osd7kkyb@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <MEYP282MB166986146989BA9DC1F311B0B6199@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
Archived-At: 
 <https://www.postgresql.org/message-id/202203241432.mt44osd7kkyb%40alvherre.pgsql>
Precedence: bulk

On 2022-Mar-24, Japin Li wrote:

> I want to know why we do not use the following style?
> 
> +static inline Datum
> +heap_getattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, bool *isnull)
> +{
> +	if (attnum > 0)
> +	{
> +		if (attnum > (int) HeapTupleHeaderGetNatts(tup->t_data))
> +			return getmissingattr(tupleDesc, attnum, isnull);
> +		else
> +			return fastgetattr(tup, attnum, tupleDesc, isnull);
> +	}
> +
> +	return heap_getsysattr(tup, attnum, tupleDesc, isnull);
> +}

That was the first thing I wrote, but I can't get myself to like it.
For this one function the code flow is obvious enough; but if you apply
the same idea to fastgetattr(), the result is not nice at all.

If there are enough votes for doing it this way, I can do that.

I guess we could do something like this instead, which seems somewhat
less bad:

if (attnum <= 0)
	return heap_getsysattr(...)
if (likely(attnum <= HeapTupleHeaderGetNattrs(...)))
	return fastgetattr(...)

return getmissingattr(...)

but I still prefer the one in the v2 patch I posted.

It's annoying that case 0 (InvalidAttrNumber) is not well handled
anywhere.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/