Date: Thu, 24 Mar 2022 14:26:10 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: turn fastgetattr and heap_getattr to inline functions
Message-ID: <202203241326.puromteoovaz@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="r7zmaabpzc6r33ml"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YjxfdpEcOBfIgmuH@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/202203241326.puromteoovaz%40alvherre.pgsql>
Precedence: bulk


--r7zmaabpzc6r33ml
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2022-Mar-24, Michael Paquier wrote:

> Hmm.  I think that you'd better add a return at the end of each
> function?  Some compilers are dumb in detecting that all the code 
> paths return (aka recent d0083c1) and could generate warnings, even if
> things are coded to return all the time, like in your patch.

Hmm, OK to do something about that.  I added pg_unreachable(): looking
at LWLockAttemptLock(), it looks that that should be sufficient.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

--r7zmaabpzc6r33ml
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0001-Change-fastgetattr-and-heap_getattr-to-inline-fun.patch"

From bd535ae2c8044de0c1eb6f4e17dd0f73c50e1622 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 24 Mar 2022 10:40:21 +0100
Subject: [PATCH v2] Change fastgetattr and heap_getattr to inline functions

They were macros previously, but recent callsite additions made Coverity
complain about one of the assertions being always true.  This change
could have been made a long time ago, but the Coverity complain broke
the inertia.
---
 src/backend/access/heap/heapam.c  |  46 ---------
 src/include/access/htup_details.h | 155 ++++++++++++++----------------
 2 files changed, 73 insertions(+), 128 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3746336a09..74ad445e59 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1131,52 +1131,6 @@ heapgettup_pagemode(HeapScanDesc scan,
 }
 
 
-#if defined(DISABLE_COMPLEX_MACRO)
-/*
- * This is formatted so oddly so that the correspondence to the macro
- * definition in access/htup_details.h is maintained.
- */
-Datum
-fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
-			bool *isnull)
-{
-	return (
-			(attnum) > 0 ?
-			(
-			 (*(isnull) = false),
-			 HeapTupleNoNulls(tup) ?
-			 (
-			  TupleDescAttr((tupleDesc), (attnum) - 1)->attcacheoff >= 0 ?
-			  (
-			   fetchatt(TupleDescAttr((tupleDesc), (attnum) - 1),
-						(char *) (tup)->t_data + (tup)->t_data->t_hoff +
-						TupleDescAttr((tupleDesc), (attnum) - 1)->attcacheoff)
-			   )
-			  :
-			  nocachegetattr((tup), (attnum), (tupleDesc))
-			  )
-			 :
-			 (
-			  att_isnull((attnum) - 1, (tup)->t_data->t_bits) ?
-			  (
-			   (*(isnull) = true),
-			   (Datum) NULL
-			   )
-			  :
-			  (
-			   nocachegetattr((tup), (attnum), (tupleDesc))
-			   )
-			  )
-			 )
-			:
-			(
-			 (Datum) NULL
-			 )
-		);
-}
-#endif							/* defined(DISABLE_COMPLEX_MACRO) */
-
-
 /* ----------------------------------------------------------------
  *					 heap access method interface
  * ----------------------------------------------------------------
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index b2d52ed16c..0c5c7b352f 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -690,88 +690,6 @@ struct MinimalTupleData
 #define HeapTupleClearHeapOnly(tuple) \
 		HeapTupleHeaderClearHeapOnly((tuple)->t_data)
 
-
-/* ----------------
- *		fastgetattr
- *
- *		Fetch a user attribute's value as a Datum (might be either a
- *		value, or a pointer into the data area of the tuple).
- *
- *		This must not be used when a system attribute might be requested.
- *		Furthermore, the passed attnum MUST be valid.  Use heap_getattr()
- *		instead, if in doubt.
- *
- *		This gets called many times, so we macro the cacheable and NULL
- *		lookups, and call nocachegetattr() for the rest.
- * ----------------
- */
-
-#if !defined(DISABLE_COMPLEX_MACRO)
-
-#define fastgetattr(tup, attnum, tupleDesc, isnull)					\
-(																	\
-	AssertMacro((attnum) > 0),										\
-	(*(isnull) = false),											\
-	HeapTupleNoNulls(tup) ?											\
-	(																\
-		TupleDescAttr((tupleDesc), (attnum)-1)->attcacheoff >= 0 ?	\
-		(															\
-			fetchatt(TupleDescAttr((tupleDesc), (attnum)-1),		\
-				(char *) (tup)->t_data + (tup)->t_data->t_hoff +	\
-				TupleDescAttr((tupleDesc), (attnum)-1)->attcacheoff)\
-		)															\
-		:															\
-			nocachegetattr((tup), (attnum), (tupleDesc))			\
-	)																\
-	:																\
-	(																\
-		att_isnull((attnum)-1, (tup)->t_data->t_bits) ?				\
-		(															\
-			(*(isnull) = true),										\
-			(Datum)NULL												\
-		)															\
-		:															\
-		(															\
-			nocachegetattr((tup), (attnum), (tupleDesc))			\
-		)															\
-	)																\
-)
-#else							/* defined(DISABLE_COMPLEX_MACRO) */
-
-extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
-						 bool *isnull);
-#endif							/* defined(DISABLE_COMPLEX_MACRO) */
-
-
-/* ----------------
- *		heap_getattr
- *
- *		Extract an attribute of a heap tuple and return it as a Datum.
- *		This works for either system or user attributes.  The given attnum
- *		is properly range-checked.
- *
- *		If the field in question has a NULL value, we return a zero Datum
- *		and set *isnull == true.  Otherwise, we set *isnull == false.
- *
- *		<tup> is the pointer to the heap tuple.  <attnum> is the attribute
- *		number of the column (field) caller wants.  <tupleDesc> is a
- *		pointer to the structure describing the row and all its fields.
- * ----------------
- */
-#define heap_getattr(tup, attnum, tupleDesc, isnull) \
-	( \
-		((attnum) > 0) ? \
-		( \
-			((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
-				getmissingattr((tupleDesc), (attnum), (isnull)) \
-			: \
-				fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
-		) \
-		: \
-			heap_getsysattr((tup), (attnum), (tupleDesc), (isnull)) \
-	)
-
-
 /* prototypes for functions in common/heaptuple.c */
 extern Size heap_compute_data_size(TupleDesc tupleDesc,
 								   Datum *values, bool *isnull);
@@ -815,4 +733,77 @@ extern size_t varsize_any(void *p);
 extern HeapTuple heap_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
 extern MinimalTuple minimal_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
 
+/*
+ *	fastgetattr
+ *		Fetch a user attribute's value as a Datum (might be either a
+ *		value, or a pointer into the data area of the tuple).
+ *
+ *		This must not be used when a system attribute might be requested.
+ *		Furthermore, the passed attnum MUST be valid.  Use heap_getattr()
+ *		instead, if in doubt.
+ *
+ *		This gets called many times, so we macro the cacheable and NULL
+ *		lookups, and call nocachegetattr() for the rest.
+ */
+static inline Datum
+fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, bool *isnull)
+{
+	AssertMacro(attnum > 0);
+
+	*isnull = false;
+	if (HeapTupleNoNulls(tup))
+	{
+		Form_pg_attribute att;
+
+		att = TupleDescAttr(tupleDesc, attnum - 1);
+		if (att->attcacheoff >= 0)
+			return fetchatt(att, (char *) tup->t_data + tup->t_data->t_hoff +
+							att->attcacheoff);
+		else
+			return nocachegetattr(tup, attnum, tupleDesc);
+	}
+	else
+	{
+		if (att_isnull(attnum - 1, tup->t_data->t_bits))
+		{
+			*isnull = true;
+			return (Datum) NULL;
+		}
+		else
+			return nocachegetattr(tup, attnum, tupleDesc);
+	}
+
+	pg_unreachable();
+}
+
+/*
+ *	heap_getattr
+ *		Extract an attribute of a heap tuple and return it as a Datum.
+ *		This works for either system or user attributes.  The given attnum
+ *		is properly range-checked.
+ *
+ *		If the field in question has a NULL value, we return a zero Datum
+ *		and set *isnull == true.  Otherwise, we set *isnull == false.
+ *
+ *		<tup> is the pointer to the heap tuple.  <attnum> is the attribute
+ *		number of the column (field) caller wants.  <tupleDesc> is a
+ *		pointer to the structure describing the row and all its fields.
+ *
+ */
+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);
+	}
+	else
+		return heap_getsysattr(tup, attnum, tupleDesc, isnull);
+
+	pg_unreachable();
+}
+
 #endif							/* HTUP_DETAILS_H */
-- 
2.30.2


--r7zmaabpzc6r33ml--