Use pg_malloc macros in src/fe_utils

Started by Henrik TJ20 days ago6 messages
Jump to latest
#1Henrik TJ
henrik@0x48.dk

Hi

Inspired by [1], I took a stab at converting src/fe_utils to the new
pg_malloc macros.

Some mostly useless metrics: 31/34 converted, 7 casts removed.

meson test looks good for me.

1. /messages/by-id/CAHut+PvpGPDLhkHAoxw_g3jdrYxA1m16a8uagbgH3TGWSKtXNQ@mail.gmail.com

best regards, Henrik

Attachments:

v1-0001-fe_utils-Use-pg_malloc_object-and-pg_malloc_array.patchtext/plain; charset=US-ASCII; name=v1-0001-fe_utils-Use-pg_malloc_object-and-pg_malloc_array.patchDownload+32-33
#2Andreas Karlsson
andreas.karlsson@percona.com
In reply to: Henrik TJ (#1)
Re: Use pg_malloc macros in src/fe_utils

On 2/18/26 4:05 PM, Henrik TJ wrote:

Inspired by [1], I took a stab at converting src/fe_utils to the new
pg_malloc macros.

Thanks for the patch!

Looks like a nice change but why not just fix all instances of it in one
swoop? It cannot be that many as there are 166 calls to pg_malloc() and
62 calls to pg_malloc0() after your patch that need to be looked at.

--
Andreas Karlsson
Percona

#3Michael Paquier
michael@paquier.xyz
In reply to: Andreas Karlsson (#2)
Re: Use pg_malloc macros in src/fe_utils

On Mon, Feb 23, 2026 at 03:17:52AM +0100, Andreas Karlsson wrote:

Looks like a nice change but why not just fix all instances of it in one
swoop? It cannot be that many as there are 166 calls to pg_malloc() and 62
calls to pg_malloc0() after your patch that need to be looked at.

FWIW, I don't really mind if these changes are proposed gradually, and
this looked fine enough on its own. So applied.
--
Michael

#4Andreas Karlsson
andreas.karlsson@percona.com
In reply to: Michael Paquier (#3)
Re: Use pg_malloc macros in src/fe_utils

On 2/24/26 4:36 AM, Michael Paquier wrote:

On Mon, Feb 23, 2026 at 03:17:52AM +0100, Andreas Karlsson wrote:

Looks like a nice change but why not just fix all instances of it in one
swoop? It cannot be that many as there are 166 calls to pg_malloc() and 62
calls to pg_malloc0() after your patch that need to be looked at.

FWIW, I don't really mind if these changes are proposed gradually, and
this looked fine enough on its own. So applied.

Fair, here is a patch which should handle all uses in the frontend code
so we follow this pattern consistently to encourage new code to use
these macros.

When doing this I found two things which I am ot sure what the cleanest
way to handle would be so I broke them out into separate patches.

1. What should we do about when we allocate a an array of characters?
Would it make sense to use pg_array_alloc() or would that jsut be silly?
For example:

-pad = (char *) pg_malloc(l + 1);
+pad = pg_malloc_array(char, l + 1);

2. I found a small and harmless thinko. The buffer in verify_tar_file()
is actually a char * but for some reason the code did the following:

buffer = pg_malloc(READ_CHUNK_SIZE * sizeof(uint8));

What should we do about it? Just skip the "sizof(uint8)"?

Andreas

Attachments:

v1-0001-Use-pg_malloc_object-and-pg_alloc_array-variants-.patchtext/x-patch; charset=UTF-8; name=v1-0001-Use-pg_malloc_object-and-pg_alloc_array-variants-.patchDownload+152-158
v1-0002-TODO-What-to-do-about-when-we-allocate-an-array-o.patchtext/x-patch; charset=UTF-8; name=v1-0002-TODO-What-to-do-about-when-we-allocate-an-array-o.patchDownload+7-8
v1-0003-TODO-uint8-vs-char-confusion.patchtext/x-patch; charset=UTF-8; name=v1-0003-TODO-uint8-vs-char-confusion.patchDownload+1-2
#5Michael Paquier
michael@paquier.xyz
In reply to: Andreas Karlsson (#4)
Re: Use pg_malloc macros in src/fe_utils

On Fri, Feb 27, 2026 at 02:15:46AM +0100, Andreas Karlsson wrote:

1. What should we do about when we allocate a an array of characters? Would
it make sense to use pg_array_alloc() or would that jsut be silly? For
example:

-pad = (char *) pg_malloc(l + 1);
+pad = pg_malloc_array(char, l + 1);

I can see that tar_get_file_name() has been changed in 0001, which is
fine, so I have merged the change from 0002 in
dir_get_file_name()@walmethods.c into 0001, for consistency. I don't
really have a strong opinion about the rest of 0002, TBH.

2. I found a small and harmless thinko. The buffer in verify_tar_file() is
actually a char * but for some reason the code did the following:

buffer = pg_malloc(READ_CHUNK_SIZE * sizeof(uint8));

What should we do about it? Just skip the "sizof(uint8)"?

This one has already been discussed, see here:
/messages/by-id/aUJ2zxgPCaVsVi2a@ip-10-97-1-34.eu-west-3.compute.internal
The story is a bit larger than this single allocation, as it impacts
the meaning of the surrounding routines with backup manifests.

And applied 0001 after double-checking it. Thanks.
--
Michael

#6Andreas Karlsson
andreas.karlsson@percona.com
In reply to: Michael Paquier (#5)
Re: Use pg_malloc macros in src/fe_utils

On 2/27/26 11:00 AM, Michael Paquier wrote:

On Fri, Feb 27, 2026 at 02:15:46AM +0100, Andreas Karlsson wrote:

1. What should we do about when we allocate a an array of characters? Would
it make sense to use pg_array_alloc() or would that jsut be silly? For
example:

-pad = (char *) pg_malloc(l + 1);
+pad = pg_malloc_array(char, l + 1);

I can see that tar_get_file_name() has been changed in 0001, which is
fine, so I have merged the change from 0002 in
dir_get_file_name()@walmethods.c into 0001, for consistency. I don't
really have a strong opinion about the rest of 0002, TBH.

Then I think we should skip it. If someone else wants to fix it in the
future they are free to do so.

2. I found a small and harmless thinko. The buffer in verify_tar_file() is
actually a char * but for some reason the code did the following:

buffer = pg_malloc(READ_CHUNK_SIZE * sizeof(uint8));

What should we do about it? Just skip the "sizof(uint8)"?

This one has already been discussed, see here:
/messages/by-id/aUJ2zxgPCaVsVi2a@ip-10-97-1-34.eu-west-3.compute.internal
The story is a bit larger than this single allocation, as it impacts
the meaning of the surrounding routines with backup manifests.

Thanks for the link!

And applied 0001 after double-checking it. Thanks.

Thanks!

--
Andreas Karlsson
Percona