Horton

Thread

Commits

  1. Convert confusing macros in multixact.c to static inline functions

  1. Bugfix and improvements in multixact.c

    Maxim Orlov <orlovmg@gmail.com> — 2024-06-14T13:56:44Z

    Hi!

While working on a making multix xact offsets 64-bit [0] I've discovered a
minor issue.  The
thing is that type 'xid' is used in all macro, but it doesn't correct.
Appropriate MultiXactId or
MultiXactOffset should be used, actually.

And the second thing, as Heikki Linnakangas points out, args naming is also
misleading.

Since, these problems are not the point of thread [0], I decided to create
this discussion.
And here is the patch set addressing mentioned issues (0001 and 0002).

Additionally, I made an optional patch 0003 to switch from macro to inline
functions.  For
me, personally, use of macro functions is justified if we are dealing with
different argument
types, to make polymorphic call.  Which is not the case here.  So, we can
have more
control over types and still generate the same code in terms of speed.
See https://godbolt.org/z/KM8voadhs Starting from O1 function is inlined,
thus no
overhead is noticeable.  Anyway, it's up to the actual commiter to decide
does it worth it
or not.  Again, this particular patch 0003 is completely optional.

As always, any opinions and reviews are very welcome!

[0]
https://www.postgresql.org/message-id/flat/ff143b24-a093-40da-9833-d36b83726bdf%40iki.fi#61d5a0e1cf6ab94b0e8aae8559bc4cf7

-- 
Best regards,
Maxim Orlov.

  2. Re: Bugfix and improvements in multixact.c

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-06-14T19:53:47Z

    On 14/06/2024 16:56, Maxim Orlov wrote:
> Hi!
> 
> While working on a making multix xact offsets 64-bit [0] I've
> discovered a minor issue.  The thing is that type 'xid' is used in
> all macro, but it doesn't correct. Appropriate MultiXactId or 
> MultiXactOffset should be used, actually.
> 
> And the second thing, as Heikki Linnakangas points out, args naming
> is also misleading.
Thanks!

Looks good to me at a quick glance. I'll try to review and commit these 
properly by Monday.

> Additionally, I made an optional patch 0003 to switch from macro to 
> inline functions.  For me, personally, use of macro functions is 
> justified if we are dealing with different argument types, to make 
> polymorphic call.  Which is not the case here.  So, we can have more
>  control over types and still generate the same code in terms of 
> speed. See https://godbolt.org/z/KM8voadhs Starting from O1 function
> is inlined, thus no overhead is noticeable.  Anyway, it's up to the 
> actual commiter to decide does it worth it or not.  Again, this 
> particular patch 0003 is completely optional.
I agree static inline functions are generally easier to work with than 
macros. These particular macros were not too bad, though.

I'll bite the bullet and commit this one too unless someone objects. 
It's late in the v17 release cycle, but these are local to multixact.c 
so there's no risk of breaking extensions, and it seems good to do it 
now since we're modifying the macros anyway.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  3. Re: Bugfix and improvements in multixact.c

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-06-16T17:54:16Z

    On 14/06/2024 16:56, Maxim Orlov wrote:
> +static inline int
> +MXOffsetToFlagsOffset(MultiXactOffset offset)
> +{
> +	int		flagsoff;
> +
> +	offset /= MULTIXACT_MEMBERS_PER_MEMBERGROUP;
> +	offset %= MULTIXACT_MEMBERGROUPS_PER_PAGE;
> +	flagsoff = offset * MULTIXACT_MEMBERGROUP_SIZE;
> +
> +	return flagsoff;
> +}

I found this reuse of the 'offset' variable a bit confusing, so I added 
separate local variables for each step.

Committed with that change, thanks!

-- 
Heikki Linnakangas
Neon (https://neon.tech)