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