Convert macros to static inline functions

On Mon, May 16, 2022 at 1:58 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Inspired by [0], I looked to convert more macros to inline functions.
The attached patches are organized "bottom up" in terms of their API
layering; some of the later ones depend on some of the earlier ones.

All the patches look good to me, except the following that are minor
things that can be ignored if you want.

0002 patch:

+static inline OffsetNumber
+PageGetMaxOffsetNumber(Page page)
+{
+   if (((PageHeader) page)->pd_lower <= SizeOfPageHeaderData)
+       return 0;
+   else
+       return ((((PageHeader) page)->pd_lower - SizeOfPageHeaderData)
/ sizeof(ItemIdData));
+}

The "else" is not necessary, we can have the return statement directly
which would save some indentation as well. The Similar pattern can be
considered for 0004 and 0007 patches as well.
--

0004 patch:

+static inline void
+XLogFromFileName(const char *fname, TimeLineID *tli, XLogSegNo
*logSegNo, int wal_segsz_bytes)
+{
+   uint32      log;
+   uint32      seg;
+   sscanf(fname, "%08X%08X%08X", tli, &log, &seg);
+   *logSegNo = (uint64) log * XLogSegmentsPerXLogId(wal_segsz_bytes) + seg;
+}

Can we have a blank line after variable declarations that we usually have?
--

0006 patch:
+static inline Datum
+fetch_att(const void *T, bool attbyval, int attlen)
+{
+   if (attbyval)
+   {
+#if SIZEOF_DATUM == 8
+       if (attlen == sizeof(Datum))
+           return *((const Datum *) T);
+       else
+#endif

Can we have a switch case like store_att_byval() instead of if-else,
code would look more symmetric, IMO.

Regards,
Amul

On 2022-May-16, Amul Sul wrote:

+static inline OffsetNumber
+PageGetMaxOffsetNumber(Page page)
+{
+   if (((PageHeader) page)->pd_lower <= SizeOfPageHeaderData)
+       return 0;
+   else
+       return ((((PageHeader) page)->pd_lower - SizeOfPageHeaderData)
/ sizeof(ItemIdData));
+}

The "else" is not necessary, we can have the return statement directly
which would save some indentation as well. The Similar pattern can be
considered for 0004 and 0007 patches as well.

Yeah. In these cases I propose to also have a local variable so that
the cast to PageHeader appears only once.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On 16.05.22 15:23, Amul Sul wrote:

+static inline OffsetNumber
+PageGetMaxOffsetNumber(Page page)
+{
+   if (((PageHeader) page)->pd_lower <= SizeOfPageHeaderData)
+       return 0;
+   else
+       return ((((PageHeader) page)->pd_lower - SizeOfPageHeaderData)
/ sizeof(ItemIdData));
+}

The "else" is not necessary, we can have the return statement directly
which would save some indentation as well. The Similar pattern can be
considered for 0004 and 0007 patches as well.

I kind of like it better this way. It preserves the functional style of
the original macro.

+static inline void
+XLogFromFileName(const char *fname, TimeLineID *tli, XLogSegNo
*logSegNo, int wal_segsz_bytes)
+{
+   uint32      log;
+   uint32      seg;
+   sscanf(fname, "%08X%08X%08X", tli, &log, &seg);
+   *logSegNo = (uint64) log * XLogSegmentsPerXLogId(wal_segsz_bytes) + seg;
+}

Can we have a blank line after variable declarations that we usually have?

done

0006 patch:
+static inline Datum
+fetch_att(const void *T, bool attbyval, int attlen)
+{
+   if (attbyval)
+   {
+#if SIZEOF_DATUM == 8
+       if (attlen == sizeof(Datum))
+           return *((const Datum *) T);
+       else
+#endif

Can we have a switch case like store_att_byval() instead of if-else,
code would look more symmetric, IMO.

done

On Mon, May 16, 2022 at 1:28 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Inspired by [0], I looked to convert more macros to inline functions.
The attached patches are organized "bottom up" in terms of their API
layering; some of the later ones depend on some of the earlier ones.

Big +1 from me.

I converted over most of the nbtree.h function style macros in
Postgres 13, having put it off in Postgres 12 (there is one remaining
function macro due to an issue with #include dependencies). This
vastly improved the maintainability of the code, and I wish I'd done
it sooner.

Inline functions made it a lot easier to pepper various B-Tree code
utility functions with defensive assertions concerning preconditions
and postconditions. That's something that I am particular about. In
theory you can just use AssertMacro() in a function style macro. In
practice that approach is ugly, and necessitates thinking about
multiple evaluation hazards, which is enough to discourage good
defensive coding practices.

--
Peter Geoghegan

On 16.05.22 10:27, Peter Eisentraut wrote:

Inspired by [0], I looked to convert more macros to inline functions.

Here is another one from the same batch of work that I somehow didn't
send in last time.

(IMO it's questionable whether this one should be an inline function or
macro at all, rather than a normal external function.)

On Tue, Oct 4, 2022 at 12:00 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 16.05.22 10:27, Peter Eisentraut wrote:

Inspired by [0], I looked to convert more macros to inline functions.

Here is another one from the same batch of work that I somehow didn't
send in last time.

I think assertion can be placed outside of the IF-block and braces can
be removed.

(IMO it's questionable whether this one should be an inline function or
macro at all, rather than a normal external function.)

IMO, it should be inlined with RelationGetSmgr().

Regards,
Amul

On 04.10.22 08:57, Amul Sul wrote:

On Tue, Oct 4, 2022 at 12:00 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 16.05.22 10:27, Peter Eisentraut wrote:

Inspired by [0], I looked to convert more macros to inline functions.

Here is another one from the same batch of work that I somehow didn't
send in last time.

I think assertion can be placed outside of the IF-block and braces can
be removed.

Committed that way, thanks.

On 16.05.22 10:27, Peter Eisentraut wrote:

Inspired by [0], I looked to convert more macros to inline functions.

This is an older thread where I left something unfinished:

Note 2: Many macros in htup_details.h operate both on HeapTupleHeader
and on MinimalTuple, so converting them to a function doesn't work in a
straightforward way.  I have some in-progress work in that area, but I
have not included any of that here.

Here is the patch set for this.

There are actually only two macros that operate on both HeapTupleHeader
and MinimalTuple, so it wasn't as much as I had written above. I just
left those as macros. I converted the rest to inline functions in a
straightforward way as before. A small amount of reordering was necessary.

But just for language-nerd fun, I'm including here an additional patch
showing how the remaining ones could be done with C11 generic selection.
I'm not planning to commit that one at this time.

On 27.12.24 11:16, Peter Eisentraut wrote:

On 16.05.22 10:27, Peter Eisentraut wrote:

Inspired by [0], I looked to convert more macros to inline functions.

This is an older thread where I left something unfinished:

Note 2: Many macros in htup_details.h operate both on HeapTupleHeader
and on MinimalTuple, so converting them to a function doesn't work in
a straightforward way.  I have some in-progress work in that area, but
I have not included any of that here.

Here is the patch set for this.

I have committed this.

There are actually only two macros that operate on both HeapTupleHeader
and MinimalTuple, so it wasn't as much as I had written above.  I just
left those as macros.  I converted the rest to inline functions in a
straightforward way as before.  A small amount of reordering was necessary.

But just for language-nerd fun, I'm including here an additional patch
showing how the remaining ones could be done with C11 generic selection.
 I'm not planning to commit that one at this time.

... except this.

On Thu, 23 Jan 2025 at 14:39, Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

On 27.12.24 11:16, Peter Eisentraut wrote:

On 16.05.22 10:27, Peter Eisentraut wrote:

Inspired by [0], I looked to convert more macros to inline functions.

This is an older thread where I left something unfinished:

Note 2: Many macros in htup_details.h operate both on HeapTupleHeader
and on MinimalTuple, so converting them to a function doesn't work in
a straightforward way. I have some in-progress work in that area, but
I have not included any of that here.

Here is the patch set for this.

I have committed this.

Great job! I've been working on the 64 XIDs patch for years, and I've never
liked this place. On the other hand,
as we know, inlining does not always work since it only suggests to the
compiler to do it. After all, many of
these calls are used in pretty "hot" places and every instruction is
important, in my opinion. Wouldn't it be
better to use pg_attribute_always_inline in this particular module?

PFA patch. I don't use pg_attribute_always_inline for fastgetattr and
heap_getattr because they are relatively
large. I think it's worth leaving the possibility for debugging here.

--
Best regards,
Maxim Orlov.

On 31.01.25 14:29, Maxim Orlov wrote:

Great job! I've been working on the 64 XIDs patch for years, and I've
never liked this place.  On the other hand,
as we know, inlining does not always work since it only suggests to the
compiler to do it. After all, many of
these calls are used in pretty "hot" places and every instruction is
important, in my opinion.  Wouldn't it be
better to use pg_attribute_always_inline in this particular module?

PFA patch.  I don't use pg_attribute_always_inline for fastgetattr and
heap_getattr because they are relatively
large. I think it's worth leaving the possibility for debugging here.

I've done some analysis with -Winline. The reasons for inlining to fail
are:

1) The function is too large.
2) The function call is unlikely. (Usually when used in elog() arguments.)
3) The function can never be inlined because it uses setjmp(). (This is
kind of a bug on our end, I think.)

The existing uses of pg_attribute_always_inline all appear to address
reason 1.

I think my/your patch does not touch any functions near the limit size,
so it does not seem necessary.

I think if we use pg_attribute_always_inline without any evidence, then
"inline" by itself might become meaningless.