Thread

Commits

Change fastgetattr and heap_getattr to inline functions
- e27f4ee0a701 15.0 landed

turn fastgetattr and heap_getattr to inline functions

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-24T10:21:07Z

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/

Re: turn fastgetattr and heap_getattr to inline functions

Michael Paquier <michael@paquier.xyz> — 2022-03-24T12:09:26Z

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

Re: turn fastgetattr and heap_getattr to inline functions

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-24T13:26:10Z

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/

Re: turn fastgetattr and heap_getattr to inline functions

Japin Li <japinli@hotmail.com> — 2022-03-24T13:36:51Z

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.

Re: turn fastgetattr and heap_getattr to inline functions

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-24T14:32:49Z

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/

Re: turn fastgetattr and heap_getattr to inline functions

Japin Li <japinli@hotmail.com> — 2022-03-24T15:41:43Z

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.

Re: turn fastgetattr and heap_getattr to inline functions

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-24T16:40:46Z

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.

Re: turn fastgetattr and heap_getattr to inline functions

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-24T16:44:38Z

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.

Re: turn fastgetattr and heap_getattr to inline functions

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-24T17:05:12Z

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)

Re: turn fastgetattr and heap_getattr to inline functions

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-03-25T09:42:14Z

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)

Re: turn fastgetattr and heap_getattr to inline functions

Japin Li <japinli@hotmail.com> — 2022-03-25T11:18:14Z

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.