Обсуждение: Convert macros to static inline functions

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.


Note 1: Some macros that do by-value assignments like

#define PageXLogRecPtrSet(ptr, lsn) \
  ((ptr).xlogid = (uint32) ((lsn) >> 32), (ptr).xrecoff = (uint32) (lsn))

can't be converted to functions without changing the API, so I left 
those alone for now.


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.


[0]: 
https://www.postgresql.org/message-id/202203241021.uts52sczx3al@alvherre.pgsql

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 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 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.