turn fastgetattr and heap_getattr to inline functions

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

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/

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.