Re: turn fastgetattr and heap_getattr to inline functions

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.