Обсуждение: Consistently use palloc_object() and palloc_array()

Hi hackers,

I've changed all code to use the "new" palloc_object(), palloc_array(),
palloc0_object(), palloc0_array, repalloc_array() and repalloc0_array()
macros. This makes the code more readable and more consistent.

The patch is pretty big but potential merge conflicts should be easy to
resolve. If preferred, I can also further split up the patch, e.g.
directory by directory or high impact files first.

The patch is passing "meson test" and I've additionally wrote a script
that parses the patch file and verifies that every two corresponding +
and - lines match (e.g. palloc0() replaced by palloc0_array() or
palloc0_object(), the same for palloc() and repalloc(), additionally
some checks to make sure the conversion to the _array() variant is
correct).

--
David Geier

> On Nov 27, 2025, at 06:09, David Geier <geidav.pg@gmail.com> wrote:
>
> Hi hackers,
>
> I've changed all code to use the "new" palloc_object(), palloc_array(),
> palloc0_object(), palloc0_array, repalloc_array() and repalloc0_array()
> macros. This makes the code more readable and more consistent.
>
> The patch is pretty big but potential merge conflicts should be easy to
> resolve. If preferred, I can also further split up the patch, e.g.
> directory by directory or high impact files first.
>
> The patch is passing "meson test" and I've additionally wrote a script
> that parses the patch file and verifies that every two corresponding +
> and - lines match (e.g. palloc0() replaced by palloc0_array() or
> palloc0_object(), the same for palloc() and repalloc(), additionally
> some checks to make sure the conversion to the _array() variant is
> correct).
>
> --
> David Geier<v1-0001-Consistently-use-palloc_object-and-palloc_array.patch>

This is a large patch, I just take a quick look, and found that:

```
-        *phoned_word = palloc(sizeof(char) * strlen(word) + 1);
+        *phoned_word = palloc_array(char, strlen(word) + 1);
```

And

```
-        params = (const char **) palloc(sizeof(char *));
+        params = palloc_object(const char *);
```

Applying palloc_array and palloc_object to char type doesn’t seem to improve anything.

Best reagards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Wed, Nov 26, 2025 at 11:09:31PM +0100, David Geier wrote:
> I've changed all code to use the "new" palloc_object(), palloc_array(),
> palloc0_object(), palloc0_array, repalloc_array() and repalloc0_array()
> macros. This makes the code more readable and more consistent.
>
> The patch is pretty big but potential merge conflicts should be easy to
> resolve. If preferred, I can also further split up the patch, e.g.
> directory by directory or high impact files first.

The backpatching extra-pain argument indeed comes into mind first when
it comes to such changes, but perhaps we should just bite the bullet
and encourage the new allocation styles across the tree, as you are
doing here.  I'm not completely sure if it would make sense to split
things up, if we do I would do it on a subdirectory basis like to
suggest, perhaps, like contrib/, src/backend/executor/, etc. to
balance the blast damage.  Did you use some kind of automation to find
all of these?  If yes, what did you use?

> The patch is passing "meson test" and I've additionally wrote a script
> that parses the patch file and verifies that every two corresponding +
> and - lines match (e.g. palloc0() replaced by palloc0_array() or
> palloc0_object(), the same for palloc() and repalloc(), additionally
> some checks to make sure the conversion to the _array() variant is
> correct).

It may be an idea to share that as well, so as your checks could be
replicated rather than partially re-guessed.
--
Michael

On Thu, Nov 27, 2025 at 11:10 AM David Geier <geidav.pg@gmail.com> wrote:
> I've changed all code to use the "new" palloc_object(), palloc_array(),
> palloc0_object(), palloc0_array, repalloc_array() and repalloc0_array()
> macros. This makes the code more readable and more consistent.

I wondered about this in the context of special alignment
requirements[1].  palloc() aligns to MAXALIGN, which we artificially
constrain for various reasons that we can't easily change (at least
not without splitting on-disk MAXALIGN from allocation MAXALIGN, and
if we do that we'll waste more memory).  That's less than
alignof(max_align_t) on common systems, so then we have to do some
weird stuff to handle __int128 that doesn't fit too well into modern
<stdalign.h> thinking and also disables optimal codegen.

This isn't a fully-baked thought, just a thought that occurred to me
while looking into that:  If palloc_object(Int128AggState) were smart
enough to detect that alignof(T) > MAXALIGN and redirect to
palloc_aligned(sizeof(T), alignof(T), ...) at compile time, then
Int128AggState would naturally propagate the layout requirements of
its __int128 member, and we wouldn't need to do that weird stuff, and
it wouldn't be error-prone if usage of __int128 spreads to more
structs.  That really only makes sense if we generalise
palloc_object() as a programming style and consider direct use of
palloc() to be a rarer low-level interface that triggers human
reviewers to think about alignment, I guess.  I think you'd also want
a variant that can deal with structs ending in a flexible array
member, but that seems doable...  palloc_flexible_object(T,
flexible_member, flexible_elements) or whatever.  But I might also be
missing some parts of that puzzle, for example it wouldn't make sense
if __int128 is ever stored.

[1] https://www.postgresql.org/message-id/CA%2BhUKGLQUivg-NC7dHdbRAPmG0Hapg1gGnygM5KgDfDM2a_TMg%40mail.gmail.com

Thomas Munro <thomas.munro@gmail.com> writes:
> This isn't a fully-baked thought, just a thought that occurred to me
> while looking into that:  If palloc_object(Int128AggState) were smart
> enough to detect that alignof(T) > MAXALIGN and redirect to
> palloc_aligned(sizeof(T), alignof(T), ...) at compile time, then
> Int128AggState would naturally propagate the layout requirements of
> its __int128 member, and we wouldn't need to do that weird stuff, and
> it wouldn't be error-prone if usage of __int128 spreads to more
> structs.  That really only makes sense if we generalise
> palloc_object() as a programming style and consider direct use of
> palloc() to be a rarer low-level interface that triggers human
> reviewers to think about alignment, I guess.

Hmm ... I had the same doubts as Michael about whether this change
could possibly be worth the ensuing back-patching pain.  But if
it leads to an improvement in type-safety, that'd be a reason to
take on the work.

            regards, tom lane

On Wed, Nov 26, 2025 at 10:25:12PM -0500, Tom Lane wrote:
> Hmm ... I had the same doubts as Michael about whether this change
> could possibly be worth the ensuing back-patching pain.  But if
> it leads to an improvement in type-safety, that'd be a reason to
> take on the work.

Yeah, that sounds like a reason convincing enough to do the jump.  I
didn't really feel strongly against the original proposal to begin
with as it improves the code style, FWIW.

The backpatching bits like these are always annoying, of course.  Now,
these fancy palloc() routines exist since v16 so that would just mean
two years and two branches that would need special handling.  Even
better, why not just backpatch into v15 and v14 the macros in palloc.h
and fe_memutils.h to avoid backpatch hazards due to some new code?
--
Michael

Hi!

Thanks for taking a look.

On 27.11.2025 00:03, Chao Li wrote:
> 
> This is a large patch, I just take a quick look, and found that:
> 
> ```
> -        *phoned_word = palloc(sizeof(char) * strlen(word) + 1);
> +        *phoned_word = palloc_array(char, strlen(word) + 1);
> ```
> 
> And
> 
> ```
> -        params = (const char **) palloc(sizeof(char *));
> +        params = palloc_object(const char *);
> ```
> 
> Applying palloc_array and palloc_object to char type doesn’t seem to improve anything.
> 

You mean because sizeof(char) is always 1 and hence we could instead
simply write:

*phoned_word = palloc(strlen(word) + 1);
params = palloc(1);

I think the _array and _object variants are more expressive and for sure
don't make the code less readable.

--
David Geier

David Geier <geidav.pg@gmail.com> writes:
> On 27.11.2025 00:03, Chao Li wrote:
>> This is a large patch, I just take a quick look, and found that:
>> -        *phoned_word = palloc(sizeof(char) * strlen(word) + 1);
>> +        *phoned_word = palloc_array(char, strlen(word) + 1);
>> And
>> -        params = (const char **) palloc(sizeof(char *));
>> +        params = palloc_object(const char *);
>> Applying palloc_array and palloc_object to char type doesn’t seem to improve anything.

> You mean because sizeof(char) is always 1 and hence we could instead
> simply write:
>         *phoned_word = palloc(strlen(word) + 1);
>         params = palloc(1);
> I think the _array and _object variants are more expressive and for sure
> don't make the code less readable.

Yeah, I agree these particular changes seem fine.  When you're doing
address arithmetic for a memcpy or such, it may be fine to wire in an
assumption that sizeof(char) == 1, but I think doing that in other
contexts is not particularly good style.

Another thing to note is that the proposed patch effectively changes
the expression evaluation order:

-        *phoned_word = palloc((sizeof(char) * strlen(word)) + 1);
+        *phoned_word = palloc(sizeof(char) * (strlen(word) + 1));

Now, there's not actually any difference because sizeof(char) is 1,
but if it hypothetically weren't, the new version is likely more
correct.  Presumably the +1 is meant to allow room for a trailing \0,
which is a char.

It'd be a good idea to review the patch to see if there are any
places where semantics are changed in a less benign fashion...

            regards, tom lane

Hi Michael!

On 27.11.2025 01:24, Michael Paquier wrote:
> On Wed, Nov 26, 2025 at 11:09:31PM +0100, David Geier wrote:
>> I've changed all code to use the "new" palloc_object(), palloc_array(),
>> palloc0_object(), palloc0_array, repalloc_array() and repalloc0_array()
>> macros. This makes the code more readable and more consistent.
>>
>> The patch is pretty big but potential merge conflicts should be easy to
>> resolve. If preferred, I can also further split up the patch, e.g.
>> directory by directory or high impact files first.
> 
> The backpatching extra-pain argument indeed comes into mind first when
> it comes to such changes, but perhaps we should just bite the bullet
> and encourage the new allocation styles across the tree, as you are
> doing here.  I'm not completely sure if it would make sense to split
> things up, if we do I would do it on a subdirectory basis like to
> suggest, perhaps, like contrib/, src/backend/executor/, etc. to
> balance the blast damage.  Did you use some kind of automation to find
> all of these?  If yes, what did you use?
I thought again about splitting up the patch. I'm not sure how useful
this really is. If a single committer takes this on, then it doesn't
really matter. He can apply the patch but then commit directory by
directory or in any other way he deems best. If we want to divide the
work among multiple committers splitting might be more useful.

Just tell me what you prefer and I'll provide the patch accordingly.

>> The patch is passing "meson test" and I've additionally wrote a script
>> that parses the patch file and verifies that every two corresponding +
>> and - lines match (e.g. palloc0() replaced by palloc0_array() or
>> palloc0_object(), the same for palloc() and repalloc(), additionally
>> some checks to make sure the conversion to the _array() variant is
>> correct).
> 
> It may be an idea to share that as well, so as your checks could be
> replicated rather than partially re-guessed.
I've attached the script. You can run it via

python3 verify_palloc_pairs.py patch_file

Disclaimer: The script is a bit of a mess. It doesn't check repalloc()
and it still reports five conversions as erroneous while they're
actually correct. I checked them manually. I left it at that because the
vast majority of changes it processes correctly and all tests pass. The
repalloc() occurrences I also checked manually.

--
David Geier

Hi Thomas!

On 27.11.2025 03:53, Thomas Munro wrote:
> On Thu, Nov 27, 2025 at 11:10 AM David Geier <geidav.pg@gmail.com> wrote:
>> I've changed all code to use the "new" palloc_object(), palloc_array(),
>> palloc0_object(), palloc0_array, repalloc_array() and repalloc0_array()
>> macros. This makes the code more readable and more consistent.
> 
> I wondered about this in the context of special alignment
> requirements[1].  palloc() aligns to MAXALIGN, which we artificially
> constrain for various reasons that we can't easily change (at least
> not without splitting on-disk MAXALIGN from allocation MAXALIGN, and
> if we do that we'll waste more memory).  That's less than
> alignof(max_align_t) on common systems, so then we have to do some
> weird stuff to handle __int128 that doesn't fit too well into modern
> <stdalign.h> thinking and also disables optimal codegen.
> 
> This isn't a fully-baked thought, just a thought that occurred to me
> while looking into that:  If palloc_object(Int128AggState) were smart
> enough to detect that alignof(T) > MAXALIGN and redirect to
> palloc_aligned(sizeof(T), alignof(T), ...) at compile time, then
> Int128AggState would naturally propagate the layout requirements of
> its __int128 member, and we wouldn't need to do that weird stuff, and
> it wouldn't be error-prone if usage of __int128 spreads to more
> structs.  That really only makes sense if we generalise
> palloc_object() as a programming style and consider direct use of
> palloc() to be a rarer low-level interface that triggers human
> reviewers to think about alignment, I guess.  I think you'd also want
> a variant that can deal with structs ending in a flexible array
> member, but that seems doable...  palloc_flexible_object(T,
> flexible_member, flexible_elements) or whatever.  But I might also be
> missing some parts of that puzzle, for example it wouldn't make sense
> if __int128 is ever stored.
> 
> [1] https://www.postgresql.org/message-id/CA%2BhUKGLQUivg-NC7dHdbRAPmG0Hapg1gGnygM5KgDfDM2a_TMg%40mail.gmail.com

These are some interesting ideas but I would consider them for now as
possible follow-up work, once this refactoring is merged.

--
David Geier

On 28.11.2025 22:28, Tom Lane wrote:
> David Geier <geidav.pg@gmail.com> writes:
>> On 27.11.2025 00:03, Chao Li wrote:
>>> This is a large patch, I just take a quick look, and found that:
>>> -        *phoned_word = palloc(sizeof(char) * strlen(word) + 1);
>>> +        *phoned_word = palloc_array(char, strlen(word) + 1);
>>> And
>>> -        params = (const char **) palloc(sizeof(char *));
>>> +        params = palloc_object(const char *);
>>> Applying palloc_array and palloc_object to char type doesn’t seem to improve anything.
> 
>> You mean because sizeof(char) is always 1 and hence we could instead
>> simply write:
>>         *phoned_word = palloc(strlen(word) + 1);
>>         params = palloc(1);
>> I think the _array and _object variants are more expressive and for sure
>> don't make the code less readable.
> 
> Yeah, I agree these particular changes seem fine.  When you're doing
> address arithmetic for a memcpy or such, it may be fine to wire in an
> assumption that sizeof(char) == 1, but I think doing that in other
> contexts is not particularly good style.
> 
> Another thing to note is that the proposed patch effectively changes
> the expression evaluation order:
> 
> -        *phoned_word = palloc((sizeof(char) * strlen(word)) + 1);
> +        *phoned_word = palloc(sizeof(char) * (strlen(word) + 1));
> 
> Now, there's not actually any difference because sizeof(char) is 1,
> but if it hypothetically weren't, the new version is likely more
> correct.  Presumably the +1 is meant to allow room for a trailing \0,
> which is a char.

Oh right. Good catch!

> It'd be a good idea to review the patch to see if there are any
> places where semantics are changed in a less benign fashion...

I intentionally tried to avoid any semantic changes but it's of course
possible something slipped through by accident.

It's some ~1700 occurrences in the patch. If it takes ~10 seconds to
check a single, it would take ~4.7h to review the entire patch.

If no one is willing to take this on, I could split up the patch in 4 to
5 that each can be reviewed by someone else.

--
David Geier

On Sat, Nov 29, 2025 at 10:47 AM David Geier <geidav.pg@gmail.com> wrote:
> I intentionally tried to avoid any semantic changes but it's of course
> possible something slipped through by accident.

Do you expect the generated code to be identical?  Is it?

On 28.11.2025 23:31, Thomas Munro wrote:
> On Sat, Nov 29, 2025 at 10:47 AM David Geier <geidav.pg@gmail.com> wrote:
>> I intentionally tried to avoid any semantic changes but it's of course
>> possible something slipped through by accident.
> 
> Do you expect the generated code to be identical?  Is it?

In the majority of cases yes. However, there are a few cases where a
small change in the C code can yield to differences in the generated code:

I used the following bash script to create the disassembly of all object
files in the build directory. I ran this script twice, once on master
and once on the patched branch. The directory needs to be adapted
accordingly in the script.

find . -name "*.o" -print0 | while IFS= read -r -d '' file; do
    mkdir -p ~/Desktop/master/"$(dirname "$file")"

    if objdump -drwC -Mintel "$file" > ~/Desktop/master/"$file".dis
2>/dev/null; then
        echo "  ✓ Disassembled to ~/Desktop/master/$file.dis"
    else
        echo "  ✗ Failed to disassemble $file"
    fi
done

There are 54 files that show changes in the generated code. I didn't
look through all files but the changes are largely caused by the same
reasons:

1) If the refactoring introduces a change in the number of lines, the
__LINE__ macro used by elog.h will cause a change in the disassembly.
This is the reason for the majority of changes.

2) fuzzystrmatch.c: contains a functional change. We allocate 1 byte
more now but that is safe, see [1].

3) pg_buffercache_pages.c: Functional change. Type used in
palloc_array() changed from uint64 to int, as the rest of the code only
uses an integer.

4) trgm_op.c: Contains arithmetic expressions in the calls to
palloc_array() where now the brackets are placed differently, e.g.
sizeof(type) * a * b vs sizeof(type) * (a * b). That's the reason for
many differences.

So reviewing this patch can now be done by only going through all files
that have changes in the disassembly. This is only 54 out of which most
are because of changes in the number of LOC or where the brackets are
placed.

[1] https://www.postgresql.org/message-id/524587.1764365323%40sss.pgh.pa.us

--
David Geier

On Tue, Dec 02, 2025 at 04:13:01PM +0100, David Geier wrote:
> So reviewing this patch can now be done by only going through all files
> that have changes in the disassembly. This is only 54 out of which most
> are because of changes in the number of LOC or where the brackets are
> placed.

It may be a good idea to split the patch into two parts, at least:
- One for the bulk of the changes, for the straight-forward changes.
Most of what you are suggesting are that for palloc_object and
palloc_array which are dropped-in replacements.  Checking that these
assemble the same before and after offers one extra layer of
confidence.
- Second one for the more dubious changes.

It does not change that all these need to be looked with human eyes.
For the first one, splitting things based on the code area is simpler
With more than 1.7k places changed, splitting by area and checking
them individually would be the best course, at least for me when it
comes to such mechanical changes.  It comes down with dealing with
individual doses that are not so large that they cause one's head to
spin in the middle of checking the diffs (did that a few times in the
past for this code tree, splitting and dose balance helps a lot).

I cannot say for the others, but I find the type-safety argument
mentioned upthread good enough to do a switch and encourage more the
new style moving forward.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> It may be a good idea to split the patch into two parts, at least:
> - One for the bulk of the changes, for the straight-forward changes.
> Most of what you are suggesting are that for palloc_object and
> palloc_array which are dropped-in replacements.  Checking that these
> assemble the same before and after offers one extra layer of
> confidence.
> - Second one for the more dubious changes.

Yeah, I was thinking the same.  Some of those might perhaps be bugs
that we want to back-patch, so they need to be looked at in a
different way.

            regards, tom lane

On 27.11.25 03:53, Thomas Munro wrote:
> I wondered about this in the context of special alignment
> requirements[1].  palloc() aligns to MAXALIGN, which we artificially
> constrain for various reasons that we can't easily change (at least
> not without splitting on-disk MAXALIGN from allocation MAXALIGN, and
> if we do that we'll waste more memory).  That's less than
> alignof(max_align_t) on common systems, so then we have to do some
> weird stuff to handle __int128 that doesn't fit too well into modern
> <stdalign.h> thinking and also disables optimal codegen.

On macOS ARM, I have MAXALIGN == alignof(max_align_t) == 8, but 
alignof(__int128) == 16.  (macOS Intel has 16/16.)  Also, as a 
consequence of that, the result of malloc() is not guaranteed to be 
aligned sufficiently for __int128 (need to use aligned_alloc()).  So it 
seems to me that the current behavior of palloc() is pretty consistent 
with that.

On 03.12.2025 01:40, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> It may be a good idea to split the patch into two parts, at least:
>> - One for the bulk of the changes, for the straight-forward changes.
>> Most of what you are suggesting are that for palloc_object and
>> palloc_array which are dropped-in replacements.  Checking that these
>> assemble the same before and after offers one extra layer of
>> confidence.
>> - Second one for the more dubious changes.
> 
> Yeah, I was thinking the same.  Some of those might perhaps be bugs
> that we want to back-patch, so they need to be looked at in a
> different way.
> 
>             regards, tom lane

Attached are the two patches, rebased on latest master.

The first one contains all changes that either result in no changes to
the disassembly, or only in changes due to the line counts; because
elog.h makes use of __LINE__.

The second patch contains the remaining changes, which is only 11 files.

contrib/fuzzystrmatch/fuzzystrmatch.c: semantic changes, e.g.
  palloc(sizeof(char) * strlen(word) + 1);
  =>
  palloc_array(char, strlen(word) + 1);

contrib/pg_buffercache/pg_buffercache_pages.c: semantic change
  palloc(sizeof(uint64) * os_page_count);
  =>
  palloc_array(int, os_page_count);

src/test/modules/test_rbtree/test_rbtree.c: different way to get pointer
  return (RBTNode *) palloc(sizeof(IntRBTreeNode));
  =>
  return &palloc_object(IntRBTreeNode)->rbtnode;

contrib/hstore/hstore_gin.c
contrib/hstore/hstore_op.c
contrib/pg_trgm/trgm_op.c
contrib/postgres_fdw/postgres_fdw.c
src/backend/executor/execPartition.c
src/backend/partitioning/partprune.c
src/backend/statistics/mvdistinct.c
src/backend/storage/buffer/bufmgr.c
  All files contain arithmetic expressions for the count argument of
palloc[0]_array(). The parentheses can change the order in which the
arguments are evaluated which changes the generated code.

--
David Geier

On Thu, Dec 04, 2025 at 10:31:41AM +0100, David Geier wrote:
> Attached are the two patches, rebased on latest master.
>
> The first one contains all changes that either result in no changes to
> the disassembly, or only in changes due to the line counts; because
> elog.h makes use of __LINE__.

Thanks.  That's a lot to digest.

I have generated some reports by comparing builds of HEAD and
HEAD+0001 to indeed note that a lot of noise is caused by the line
number changes.  And then, I have begun looking at contrib/ to start
with something as I had a bit of time today.  One part which we need
to be careful about is that the allocations map with the actual
declarations, and I'm doubting my compiler to detect all of them, so
visual check for each path it is...

-   void       *out = palloc(sizeof(float4KEY));
+   void       *out = palloc_object(float4KEY);

btree_gist/ has a bunch of these, which I guess don't really matter in
terms of type safety.

-   sv = palloc(sizeof(bytea *) * (maxoff + 1));
+   sv = palloc_array(bytea *, (maxoff + 1));

This one also in gbt_var_picksplit@btree_utils_var.c.  We have a
GBT_VARKEY.

+   keys = (char **) palloc_array(char *, key_count);
+   values = (char **) palloc_array(char *, val_count);

These two should not need casts.

-               stack = palloc(sizeof(*stack));
+               stack = palloc_object(sepgsql_proc_stack);

This one in sepgsql was wrong, that can be detected with a compilation
of the module.

And done with the contrib/ part for the trival changes.
--
Michael

On 05.12.2025 08:41, Michael Paquier wrote:
> On Thu, Dec 04, 2025 at 10:31:41AM +0100, David Geier wrote:
>> Attached are the two patches, rebased on latest master.
>>
>> The first one contains all changes that either result in no changes to
>> the disassembly, or only in changes due to the line counts; because
>> elog.h makes use of __LINE__.
> 
> Thanks.  That's a lot to digest.
> 
> I have generated some reports by comparing builds of HEAD and
> HEAD+0001 to indeed note that a lot of noise is caused by the line
> number changes.  And then, I have begun looking at contrib/ to start
> with something as I had a bit of time today.  One part which we need
> to be careful about is that the allocations map with the actual
> declarations, and I'm doubting my compiler to detect all of them, so
> visual check for each path it is...
> 
> -   void       *out = palloc(sizeof(float4KEY));
> +   void       *out = palloc_object(float4KEY);
> 
> btree_gist/ has a bunch of these, which I guess don't really matter in
> terms of type safety.
> 
> -   sv = palloc(sizeof(bytea *) * (maxoff + 1));
> +   sv = palloc_array(bytea *, (maxoff + 1));
> 
> This one also in gbt_var_picksplit@btree_utils_var.c.  We have a
> GBT_VARKEY.
> 
> +   keys = (char **) palloc_array(char *, key_count);
> +   values = (char **) palloc_array(char *, val_count);
> 
> These two should not need casts.
> 
> -               stack = palloc(sizeof(*stack));
> +               stack = palloc_object(sepgsql_proc_stack);
> 
> This one in sepgsql was wrong, that can be detected with a compilation
> of the module.

My bad. I hadn't realized that - obviously - not necessarily all code is
actually compiled by default.

Will the build system enable any target for which all dependencies (e.g.
libraries) are met, or are there targets that need to be enabled
explicitly? Is there a way to enable all of them so I can easily make
sure, I actually compile all code?

> And done with the contrib/ part for the trival changes.

Thanks for applying the first batch of changes.

--
David Geier

David Geier <geidav.pg@gmail.com> writes:
> My bad. I hadn't realized that - obviously - not necessarily all code is
> actually compiled by default.

> Will the build system enable any target for which all dependencies (e.g.
> libraries) are met, or are there targets that need to be enabled
> explicitly? Is there a way to enable all of them so I can easily make
> sure, I actually compile all code?

There's a good deal of platform-specific code in PG, particularly for
Windows, and obviously none of that will be built unless you're on the
right platform.  (You can reach some of the Windows-specific code on
other systems with -DEXEC_BACKEND, but that goes only so far.)

As for optional-feature stuff, I think the meson build system will by
default build every option it can find the supporting libraries for.
But the other side of that coin is that if you didn't install the
right packages it will silently not build it.

I think the most reliable way is to look at "./configure --help"
and select all the options you think should work on your platform.
Then, if you forgot to install libfoo-devel or whatever, you'll
get configure failures.

            regards, tom lane

On Fri, Dec 05, 2025 at 04:41:41PM +0900, Michael Paquier wrote:
> Thanks.  That's a lot to digest.

Digesting a bit more now..

-   char       *ret = palloc(sizeof(buf));
+   char       *ret = palloc_array(char, sizeof(buf))

This one in dumputils.c is right, but I am not sure that it is an
improvement compared to the statu-quo.

Here is a list of the files where I have noticed tha addition of
casts, which are not required anymore:
brin_tuple.c.
gistbuildbuffers.c.
genam.c.
nbtdedup.c.
nbtree.c.
nbtsort.c.
index.c
execGrouping.c
execMain.c
relnode.c
partbounds.c
mcv.c
spell.c
arrayfuncs.c
partcache.c

Perhaps you have used a semi-automatic process and missed these?
Okay, some of these relied on a "Data" structure for the size vs
pointer for the allocation, but the results are the same.

-       IndexOrderByDistance *distances =
-           palloc(sizeof(distances[0]) * so->numberOfOrderBys);

Okay, this one in spgscan.c is not the usual project style.  Correct,
still funky.

            b_checkargnulls =
-               palloc(sizeof(LLVMBasicBlockRef *) * op->d.func.nargs);
+               palloc_array(LLVMBasicBlockRef *, op->d.func.nargs);

This one in llvmjit_expr.c was causing a compilation failure.  I am
not exactly sure why, but discarded for now.  I got a reproduction
locally as well as in the CI.

-   node->tsm_state = palloc0(sizeof(BernoulliSamplerData));
+   node->tsm_state = palloc0_object(BernoulliSamplerData);

One can argue that this one in bernouilli.c is not really necessary,
tsm_state is actually a void *.

Among the 300-ish files changed in the backend, 48 of them had their
builds slightly change.  The list of them is attached.

So that's everything for the trivial changes, with 4601e7f1c708 for
src/backend/ and 0c3c5c3b06a3 for the rest.
--
Michael

On Wed, Dec 10, 2025 at 11:38 AM Michael Paquier <michael@paquier.xyz> wrote:
>             b_checkargnulls =
> -               palloc(sizeof(LLVMBasicBlockRef *) * op->d.func.nargs);
> +               palloc_array(LLVMBasicBlockRef *, op->d.func.nargs);
>
> This one in llvmjit_expr.c was causing a compilation failure.  I am
> not exactly sure why, but discarded for now.  I got a reproduction
> locally as well as in the CI.

I think the original code is wrong, it should have been
sizeof(LLVMBasicBlockRef)?  It'll be the same size anyway (these
LLVM*Ref types are just pointers), but that'd explain why the
transformation didn't compile.

On Wed, Dec 10, 2025 at 11:41:25AM +1300, Thomas Munro wrote:
> On Wed, Dec 10, 2025 at 11:38 AM Michael Paquier <michael@paquier.xyz> wrote:
>>             b_checkargnulls =
>> -               palloc(sizeof(LLVMBasicBlockRef *) * op->d.func.nargs);
>> +               palloc_array(LLVMBasicBlockRef *, op->d.func.nargs);
>>
>> This one in llvmjit_expr.c was causing a compilation failure.  I am
>> not exactly sure why, but discarded for now.  I got a reproduction
>> locally as well as in the CI.
>
> I think the original code is wrong, it should have been
> sizeof(LLVMBasicBlockRef)?  It'll be the same size anyway (these
> LLVM*Ref types are just pointers), but that'd explain why the
> transformation didn't compile.

Yes, perhaps, I would need to double-check to be sure, and you make it
sound like this one should be back-patched.  I was planning to get
back to this one with the batch of non-trivial changes that Bryan has
posted now that I've cleared the path for the stright-forward ones.
--
Michael

Hi Michael!

Thanks for continuing applying the patches.

On 09.12.2025 23:37, Michael Paquier wrote:
> On Fri, Dec 05, 2025 at 04:41:41PM +0900, Michael Paquier wrote:
>> Thanks.  That's a lot to digest.
> 
> Digesting a bit more now..
> 
> -   char       *ret = palloc(sizeof(buf));
> +   char       *ret = palloc_array(char, sizeof(buf))
> 
> This one in dumputils.c is right, but I am not sure that it is an
> improvement compared to the statu-quo.

Apart from saving casts, using the _array() and _object() variants
improves readability as it's clear from the used macro what the code
intents to do.

For example. without knowing the type of buf, it's not immediately clear
if this is allocating an array of the size of buf, or if it's allocating
an object (e.g. if buf happened to be some struct that represents a
buffer object).

The amount of allocated memory would of course be the same, but how it's
going to be used can be nicely derived from the used macro.

> Here is a list of the files where I have noticed tha addition of
> casts, which are not required anymore:
> brin_tuple.c.
> gistbuildbuffers.c.
> genam.c.
> nbtdedup.c.
> nbtree.c.
> nbtsort.c.
> index.c
> execGrouping.c
> execMain.c
> relnode.c
> partbounds.c
> mcv.c
> spell.c
> arrayfuncs.c
> partcache.c
> 
> Perhaps you have used a semi-automatic process and missed these?

Do you mean in these files I forgot removing casts that got unnecessary
after using _array() / _object()? It's possible that I missed some,
given the large amount. Please fix them as you see fit.

> One can argue that this one in bernouilli.c is not really necessary,
> tsm_state is actually a void *.

As stated above: this change is not only about saving casts but the
macros convey the intent much better than a call to palloc().

> Among the 300-ish files changed in the backend, 48 of them had their
> builds slightly change.  The list of them is attached.

Do you mean the disassembly because the number of lines of code changes?

--
David Geier

On Wed, Dec 10, 2025 at 12:48:35PM +0100, David Geier wrote:
> On 09.12.2025 23:37, Michael Paquier wrote:
>> On Fri, Dec 05, 2025 at 04:41:41PM +0900, Michael Paquier wrote:
> Do you mean in these files I forgot removing casts that got unnecessary
> after using _array() / _object()? It's possible that I missed some,
> given the large amount. Please fix them as you see fit.

Yes, your patch did not remove casts in all the files I have listed
upthread.  I have fixed them already in the tree, for all the trivial
changes.

>> One can argue that this one in bernouilli.c is not really necessary,
>> tsm_state is actually a void *.
>
> As stated above: this change is not only about saving casts but the
> macros convey the intent much better than a call to palloc().

I got that, but I could not convinced myself that such cases are worth
it.  We don't have many of them in the tree anyway.  They count for
less than 1% of all the changes dealt with here

>> Among the 300-ish files changed in the backend, 48 of them had their
>> builds slightly change.  The list of them is attached.
>
> Do you mean the disassembly because the number of lines of code changes?

Yes, I have cross-checked the reports generated between before and
after the patches, to see that they matched with the formulas
changing.  A trick that I have used here and that was rather painful
is to manually change the files where the formulas got shorter to make
their build match..  But well..

I still need to get through the remaining dubious changes you have
posted, including the llvm one that was wrong.  It seems like some of
these things warrant a backpatch.
--
Michael

On Thu, Dec 11, 2025 at 08:01:49AM +0900, Michael Paquier wrote:
> I still need to get through the remaining dubious changes you have
> posted, including the llvm one that was wrong.  It seems like some of
> these things warrant a backpatch.

I have been looking at the rest of these changes with some -O2, and I
have been puzzled by the differences in hstore_gin.c and hstore_op.c.
In all these cases we generate more instructions with the patch than
without the patch.  Anyway, I assume that these are the ones that
matter:
entries = (Datum *) palloc(sizeof(Datum) * 2 * count);
out_datums = palloc_array(Datum, count * 2);

pg_trgm.c was less puzzling.  For example:
-   trg1 = (trgm *) palloc(sizeof(trgm) * (slen1 / 2 + 1) * 3);
+   trg1 = palloc_array(trgm, (slen1 / 2 + 1) * 3);
This one leads to something like that before vs after:
<     1418:     83 c0 01                add    eax,0x1
>     1418:     8d 44 40 03             lea    eax,[rax+rax*2+0x3]

In execPartition.c and partprune.c, as far as I can see we are cutting
a few mov, leading to less instructions overall.

For mvdistinct.c, we are cutting things overall.  I am seeing less.

All the fuzz in postgres_fdw.c is caused by this one:
-   p_values = (const char **) palloc(sizeof(char *) * fmstate->p_nums
* numSlots);
+   p_values = palloc_array(const char *, fmstate->p_nums * numSlots);

bufmgr.c did not matter, same before and after.

I am not completely sure about the one in fuzzystrmatch, I would need
to study more the metaphone code.  :)

One formula in llvmjit_expr.c has been wrong since v10, so I have
backpatched a fix for it.

In pg_buffercache_pages.c, the difference is here:
-           os_page_status = palloc(sizeof(uint64) * os_page_count);
+           os_page_status = palloc_array(int, os_page_count);
Your formula is correct, the previous one was not by using a uint64.
So it allocated twice too much memory.  Backpatched this one down to
v18.

And that should close this thread, at least from my side..
--
Michael

On 05.12.2025 15:47, Tom Lane wrote:
> David Geier <geidav.pg@gmail.com> writes:
>> My bad. I hadn't realized that - obviously - not necessarily all code is
>> actually compiled by default.
> 
>> Will the build system enable any target for which all dependencies (e.g.
>> libraries) are met, or are there targets that need to be enabled
>> explicitly? Is there a way to enable all of them so I can easily make
>> sure, I actually compile all code?
> 
> There's a good deal of platform-specific code in PG, particularly for
> Windows, and obviously none of that will be built unless you're on the
> right platform.  (You can reach some of the Windows-specific code on
> other systems with -DEXEC_BACKEND, but that goes only so far.)
> 
> As for optional-feature stuff, I think the meson build system will by
> default build every option it can find the supporting libraries for.
> But the other side of that coin is that if you didn't install the
> right packages it will silently not build it.
> 
> I think the most reliable way is to look at "./configure --help"
> and select all the options you think should work on your platform.
> Then, if you forgot to install libfoo-devel or whatever, you'll
> get configure failures.

Thanks for the pointers.

Do we know what code each build animal actually has enabled? The
platform specific code is clear but anything that depends on specific
libraries being installed is not.

Do build animal owners try to enable as much code as possible, or is
this completely up to what the owner happened to do when setting up the
build animal?

I'm wondering how much more code coverage we could get on each animal if
maximized the amount of code that is enabled.

--
David Geier

On 11.12.2025 06:33, Michael Paquier wrote:
> On Thu, Dec 11, 2025 at 08:01:49AM +0900, Michael Paquier wrote:
>> I still need to get through the remaining dubious changes you have
>> posted, including the llvm one that was wrong.  It seems like some of
>> these things warrant a backpatch.
> 
> I have been looking at the rest of these changes with some -O2, and I
> have been puzzled by the differences in hstore_gin.c and hstore_op.c.
> In all these cases we generate more instructions with the patch than
> without the patch.  Anyway, I assume that these are the ones that
> matter:
> entries = (Datum *) palloc(sizeof(Datum) * 2 * count);
> out_datums = palloc_array(Datum, count * 2);

Yeah, in a few cases the different bracketing has some surprisingly big
influence on the generated assmebly.

> pg_trgm.c was less puzzling.  For example:
> -   trg1 = (trgm *) palloc(sizeof(trgm) * (slen1 / 2 + 1) * 3);
> +   trg1 = palloc_array(trgm, (slen1 / 2 + 1) * 3);
> This one leads to something like that before vs after:
> <     1418:     83 c0 01                add    eax,0x1
>>     1418:     8d 44 40 03             lea    eax,[rax+rax*2+0x3]
> 
> In execPartition.c and partprune.c, as far as I can see we are cutting
> a few mov, leading to less instructions overall.
> 
> For mvdistinct.c, we are cutting things overall.  I am seeing less.
> 
> All the fuzz in postgres_fdw.c is caused by this one:
> -   p_values = (const char **) palloc(sizeof(char *) * fmstate->p_nums
> * numSlots);
> +   p_values = palloc_array(const char *, fmstate->p_nums * numSlots);  
> 
> bufmgr.c did not matter, same before and after.
> 
> I am not completely sure about the one in fuzzystrmatch, I would need
> to study more the metaphone code.  :)
Actually, the change in fuzzystrmatch.c is incorrect.

With the new code, palloc(0) will be called for strlen(word) == 0. Then
no space for the null-terminator byte will be left.

Even if palloc(0) returns memory that can be dereferenced, I don't think
it's a good idea to rely on that.

> One formula in llvmjit_expr.c has been wrong since v10, so I have
> backpatched a fix for it.
> 
> In pg_buffercache_pages.c, the difference is here:
> -           os_page_status = palloc(sizeof(uint64) * os_page_count);
> +           os_page_status = palloc_array(int, os_page_count);
> Your formula is correct, the previous one was not by using a uint64.
> So it allocated twice too much memory.  Backpatched this one down to
> v18.
> 
> And that should close this thread, at least from my side..

Thanks Michael for taking care of this patch!

--
David Geier

David Geier <geidav.pg@gmail.com> writes:
> Do we know what code each build animal actually has enabled?

Of course.  The configuration is reported in the logs of every
buildfarm run.  For instance, in the most recent run at this
moment,

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2025-12-11%2015%3A00%3A05

we can see

  $ ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
        --with-python --with-tcl --with-gssapi --with-openssl --with-ldap \
        --with-libxml --with-libxslt --with-pam --with-selinux \
        --with-systemd --with-liburing --with-libcurl --with-libnuma \
        --with-lz4 --with-zstd --prefix=/repos/client-code-REL_20/HEAD/inst \
        --with-pgport=5678 --cache-file=/repos/client-code-REL_20/accache-caiman/config-HEAD.cache

and you can drill down to the "configure" step if you want more
detail.

> Do build animal owners try to enable as much code as possible, or is
> this completely up to what the owner happened to do when setting up the
> build animal?

It's the owner's choice.  The buildfarm client's sample config file
has a list of suggested options, and I wouldn't be too surprised
if a lot of people just left that list alone.  I think the ones
paying closer attention probably try to enable as much as they can.

            regards, tom lane