Thread: turn fastgetattr and heap_getattr to inline functions
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/
Attachment
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
Attachment
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/
Attachment
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.
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/
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.
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.
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.
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)
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)
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.