pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedturn fastgetattr and heap_getattr to inline functions
11+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2022-03-24 e27f4ee0a7 Change fastgetattr and heap_getattr to inline functions
* turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 10:21 Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2022-03-24 10:21 UTC (permalink / raw)
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
This patch should silence some recent Coverity (false positive)
complaints about assertions contained in these macros.
Portability testing at:
https://cirrus-ci.com/github/alvherre/postgres/macros-to-inlinefuncs
Intend to push later today, unless something ugly happens.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Attachments:
0001-Change-fastgetattr-and-heap_getattr-to-inline-functi.patch (text/x-diff, 7.3K) download
From 215eb05b368c202fce71efe242b58f0fe7025b38 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] 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 | 151 ++++++++++++++----------------
2 files changed, 69 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..de0b91e1fa 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,73 @@ 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);
+ }
+}
+
+/*
+ * 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);
+}
+
#endif /* HTUP_DETAILS_H */
--
2.30.2
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 12:09 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 2 replies; 11+ messages in thread
From: Michael Paquier @ 2022-03-24 12:09 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Mar 24, 2022 at 11:21:07AM +0100, Alvaro Herrera wrote:
> This patch should silence some recent Coverity (false positive)
> complaints about assertions contained in these macros.
The logic looks fine. Good idea to get rid of DISABLE_COMPLEX_MACRO.
> Portability testing at:
> https://cirrus-ci.com/github/alvherre/postgres/macros-to-inlinefuncs
>
> Intend to push later today, unless something ugly happens.
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.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 13:26 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2022-03-24 13:26 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
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.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
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 13:36 Japin Li <japinli@hotmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 11+ messages in thread
From: Japin Li @ 2022-03-24 13:36 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers@lists.postgresql.org
On Thu, 24 Mar 2022 at 21:26, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 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.
Hi,
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);
+}
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 14:32 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Japin Li <japinli@hotmail.com>
0 siblings, 2 replies; 11+ messages in thread
From: Alvaro Herrera @ 2022-03-24 14:32 UTC (permalink / raw)
To: Japin Li <japinli@hotmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers@lists.postgresql.org
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/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 15:41 Japin Li <japinli@hotmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 1 reply; 11+ messages in thread
From: Japin Li @ 2022-03-24 15:41 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers@lists.postgresql.org
On Thu, 24 Mar 2022 at 22:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 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.
Thanks for your detail explaination. I find bottomup_sort_and_shrink_cmp()
has smilar code
static int
bottomup_sort_and_shrink_cmp(const void *arg1, const void *arg2)
{
const IndexDeleteCounts *group1 = (const IndexDeleteCounts *) arg1;
const IndexDeleteCounts *group2 = (const IndexDeleteCounts *) arg2;
[...]
pg_unreachable();
return 0;
}
IIUC, the last statement is used to keep the compiler quiet. However,
it doesn't exist in LWLockAttemptLock(). Why?
The difference between bottomup_sort_and_shrink_cmp() and LWLockAttemptlock()
is that LWLockAttemptlock() always returned before pg_unreachable(), however,
bottomup_sort_and_shrink_cmp() might be returned after pg_unreachable(), which
isn't expected.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 16:40 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 11+ messages in thread
From: Peter Eisentraut @ 2022-03-24 16:40 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
On 24.03.22 13:09, 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.
That is a different case. We know that not all compilers understand
when elog/ereport return. But no compiler is stupid enough not to
understand that
foo()
{
if (something)
return this;
else
return that;
}
always reaches a return.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 16:44 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 0 replies; 11+ messages in thread
From: Peter Eisentraut @ 2022-03-24 16:44 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; Japin Li <japinli@hotmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers@lists.postgresql.org
On 24.03.22 15:32, Alvaro Herrera wrote:
>> +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.
I like your first patch. That is more of a functional style, whereas
the above is more of a procedural style.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-24 17:05 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2022-03-24 17:05 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Pg Hackers <pgsql-hackers@lists.postgresql.org>
On 2022-Mar-24, Peter Eisentraut wrote:
> But no compiler is stupid enough not to understand that
>
> foo()
> {
> if (something)
> return this;
> else
> return that;
> }
>
> always reaches a return.
We have a number of examples of this pattern, so I guess it must be
true. Pushed without the pg_unreachables, then.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Las navajas y los monos deben estar siempre distantes" (Germán Poo)
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-25 09:42 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Japin Li <japinli@hotmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2022-03-25 09:42 UTC (permalink / raw)
To: Japin Li <japinli@hotmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers@lists.postgresql.org
On 2022-Mar-24, Japin Li wrote:
> Thanks for your detail explaination. I find bottomup_sort_and_shrink_cmp()
> has smilar code
... except that bottomup_sort_and_shrink_cmp never handles the case of
the two structs being exactly identical, so I don't think this is a
great counter-example.
> IIUC, the last statement is used to keep the compiler quiet. However,
> it doesn't exist in LWLockAttemptLock(). Why?
What I do care about is the fact that LWLockAttemptLock does compile
silently everywhere without a final "return dummy_value" statement. I
don't have to build a theory for why the other function has a statement
that may or may not be actually doing anything.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: turn fastgetattr and heap_getattr to inline functions
@ 2022-03-25 11:18 Japin Li <japinli@hotmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 11+ messages in thread
From: Japin Li @ 2022-03-25 11:18 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers@lists.postgresql.org
On Fri, 25 Mar 2022 at 17:42, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Mar-24, Japin Li wrote:
>
>> Thanks for your detail explaination. I find bottomup_sort_and_shrink_cmp()
>> has smilar code
>
> ... except that bottomup_sort_and_shrink_cmp never handles the case of
> the two structs being exactly identical, so I don't think this is a
> great counter-example.
>
>> IIUC, the last statement is used to keep the compiler quiet. However,
>> it doesn't exist in LWLockAttemptLock(). Why?
>
> What I do care about is the fact that LWLockAttemptLock does compile
> silently everywhere without a final "return dummy_value" statement.
I'm just a bit confused about this.
> I
> don't have to build a theory for why the other function has a statement
> that may or may not be actually doing anything.
Anyway, thanks for your explaination!
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2022-03-25 11:18 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 10:21 turn fastgetattr and heap_getattr to inline functions Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-03-24 12:09 ` Michael Paquier <michael@paquier.xyz>
2022-03-24 13:26 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-03-24 13:36 ` Japin Li <japinli@hotmail.com>
2022-03-24 14:32 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-03-24 15:41 ` Japin Li <japinli@hotmail.com>
2022-03-25 09:42 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-03-25 11:18 ` Japin Li <japinli@hotmail.com>
2022-03-24 16:44 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2022-03-24 16:40 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2022-03-24 17:05 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
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