On Thu, 24 Mar 2022 at 22:32, Alvaro Herrera <alvhe...@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.
