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
Date: Thu, 24 Mar 2022 14:26:10 +0100
Message-ID: <202203241326.puromteoovaz@alvherre.pgsql> (raw)
In-Reply-To: <YjxfdpEcOBfIgmuH@paquier.xyz>
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/Attachments:v2-0001-Change-fastgetattr-and-heap_getattr-to-inline-fun.patch (text/x-diff, 7.4K) download
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.cindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: turn fastgetattr and heap_getattr to inline functions
In-Reply-To: <202203241326.puromteoovaz@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox