Обсуждение: Trivial Fix: use palloc_array/repalloc_array for BufFile file arrays

> On Dec 25, 2025, at 11:12, Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi Hackers,
>
> I noticed this error while working on [1].
>
> In BufFile, the fields is claimed as an array:
> ```
> struct BufFile
> {
>     File *files; /* palloc'd array with numFiles entries */
> ```
>
> However, it’s allocated by palloc_object():
> ```
> file->files = palloc_object(File);
> ```
>
> And reallocated by repalloc():
> ```
>     file->files = (File *) repalloc(file->files,
>         (file->numFiles + 1) * sizeof(File));
> ```
>
> This trivial patch just changes to use palloc_array/repalloc_array, which makes the intent clearer.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
> <v1-0001-Use-palloc_array-repalloc_array-for-BufFile-file-.patch>


Sorry for missing the reference of [1]:

[1] https://postgr.es/m/aUStrqoOCDRFAq1M@paquier.xyz

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

> On Dec 25, 2025, at 11:34, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
>> On Dec 25, 2025, at 11:12, Chao Li <li.evan.chao@gmail.com> wrote:
>>
>> Hi Hackers,
>>
>> I noticed this error while working on [1].
>>
>> In BufFile, the fields is claimed as an array:
>> ```
>> struct BufFile
>> {
>>    File *files; /* palloc'd array with numFiles entries */
>> ```
>>
>> However, it’s allocated by palloc_object():
>> ```
>> file->files = palloc_object(File);
>> ```
>>
>> And reallocated by repalloc():
>> ```
>>    file->files = (File *) repalloc(file->files,
>>        (file->numFiles + 1) * sizeof(File));
>> ```
>>
>> This trivial patch just changes to use palloc_array/repalloc_array, which makes the intent clearer.
>>
>> Best regards,
>> --
>> Chao Li (Evan)
>> HighGo Software Co., Ltd.
>> https://www.highgo.com/
>>
>>
>>
>>
>> <v1-0001-Use-palloc_array-repalloc_array-for-BufFile-file-.patch>
>
>
> Sorry for missing the reference of [1]:
>
> [1] https://postgr.es/m/aUStrqoOCDRFAq1M@paquier.xyz
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>

PFA v2:
 * Rebased
 * Updated the commit message

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

Hi,

On Tue, Mar 3, 2026 at 5:31 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Dec 25, 2025, at 11:34, Chao Li <li.evan.chao@gmail.com> wrote:
> >
> >
> >
> >> On Dec 25, 2025, at 11:12, Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >> Hi Hackers,
> >>
> >> I noticed this error while working on [1].
> >>
> >> In BufFile, the fields is claimed as an array:
> >> ```
> >> struct BufFile
> >> {
> >>    File *files; /* palloc'd array with numFiles entries */
> >> ```
> >>
> >> However, it’s allocated by palloc_object():
> >> ```
> >> file->files = palloc_object(File);
> >> ```
> >>
> >> And reallocated by repalloc():
> >> ```
> >>    file->files = (File *) repalloc(file->files,
> >>        (file->numFiles + 1) * sizeof(File));
> >> ```
> >>
> >> This trivial patch just changes to use palloc_array/repalloc_array, which makes the intent clearer.
> >>
> >> Best regards,
> >> --
> >> Chao Li (Evan)
> >> HighGo Software Co., Ltd.
> >> https://www.highgo.com/
> >>
> >>
> >>
> >>
> >> <v1-0001-Use-palloc_array-repalloc_array-for-BufFile-file-.patch>
> >
> >
> > Sorry for missing the reference of [1]:
> >
> > [1] https://postgr.es/m/aUStrqoOCDRFAq1M@paquier.xyz
> >
> > Best regards,
> > --
> > Chao Li (Evan)
> > HighGo Software Co., Ltd.
> > https://www.highgo.com/
> >
>
> PFA v2:
>  * Rebased
>  * Updated the commit message

I've reviewed the v2 patch and here is a comment:

- file->files = palloc_object(File);
+ file->files = palloc_array(File, 1);

I'm not a fan of this change. This change looks like trying to
distinguish allocated memory by palloc_object() and palloc_array()
even though underlying memory is identical. I'm concerned about this
change creating unnecessary coding conventions.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

> On Mar 7, 2026, at 03:33, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Hi,
>
> On Tue, Mar 3, 2026 at 5:31 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>
>>> On Dec 25, 2025, at 11:34, Chao Li <li.evan.chao@gmail.com> wrote:
>>>
>>>
>>>
>>>> On Dec 25, 2025, at 11:12, Chao Li <li.evan.chao@gmail.com> wrote:
>>>>
>>>> Hi Hackers,
>>>>
>>>> I noticed this error while working on [1].
>>>>
>>>> In BufFile, the fields is claimed as an array:
>>>> ```
>>>> struct BufFile
>>>> {
>>>>   File *files; /* palloc'd array with numFiles entries */
>>>> ```
>>>>
>>>> However, it’s allocated by palloc_object():
>>>> ```
>>>> file->files = palloc_object(File);
>>>> ```
>>>>
>>>> And reallocated by repalloc():
>>>> ```
>>>>   file->files = (File *) repalloc(file->files,
>>>>       (file->numFiles + 1) * sizeof(File));
>>>> ```
>>>>
>>>> This trivial patch just changes to use palloc_array/repalloc_array, which makes the intent clearer.
>>>>
>>>> Best regards,
>>>> --
>>>> Chao Li (Evan)
>>>> HighGo Software Co., Ltd.
>>>> https://www.highgo.com/
>>>>
>>>>
>>>>
>>>>
>>>> <v1-0001-Use-palloc_array-repalloc_array-for-BufFile-file-.patch>
>>>
>>>
>>> Sorry for missing the reference of [1]:
>>>
>>> [1] https://postgr.es/m/aUStrqoOCDRFAq1M@paquier.xyz
>>>
>>> Best regards,
>>> --
>>> Chao Li (Evan)
>>> HighGo Software Co., Ltd.
>>> https://www.highgo.com/
>>>
>>
>> PFA v2:
>> * Rebased
>> * Updated the commit message
>
> I've reviewed the v2 patch and here is a comment:
>
> - file->files = palloc_object(File);
> + file->files = palloc_array(File, 1);
>
> I'm not a fan of this change. This change looks like trying to
> distinguish allocated memory by palloc_object() and palloc_array()
> even though underlying memory is identical. I'm concerned about this
> change creating unnecessary coding conventions.
>

Hi Masahiho-san,

Thank you so much for taking the time to review this patch.

Actually, this change was my original motivation for creating the patch. When I first read the code, I was confused why
thefield was named with the plural “files" even though it was allocated with palloc_object(). After reading further, I
sawthat the memory is later enlarged with repalloc, so it became clear that file->files is really meant to be an array. 

To me, the allocation method should reflect the actual meaning of the object. Here, file->files is an array, no matter
howmany elements it currently contains. Even if it only has one element at the moment, semantically it is still an
array.

By “creating unnecessary coding conventions”, are you concerned that this change might encourage people to always use
palloc_array(type,1) when allocating a single object? That concern is understandable, but I think it should depend on
thereal semantics. If the memory is for a single object, then of course palloc_array() should be wrong. 

Given the nature of C, a "File *" can point either to a single File object or to a File array. In that sense,
palloc_array()makes it explicit that this pointer is intended to represent an array, while palloc_object() suggests a
singleobject. So I think changing this to palloc_array() improves readability in this case. 

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

> On Mar 9, 2026, at 08:40, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
>> On Mar 7, 2026, at 03:33, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>
>> Hi,
>>
>> On Tue, Mar 3, 2026 at 5:31 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>
>>>
>>>
>>>> On Dec 25, 2025, at 11:34, Chao Li <li.evan.chao@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Dec 25, 2025, at 11:12, Chao Li <li.evan.chao@gmail.com> wrote:
>>>>>
>>>>> Hi Hackers,
>>>>>
>>>>> I noticed this error while working on [1].
>>>>>
>>>>> In BufFile, the fields is claimed as an array:
>>>>> ```
>>>>> struct BufFile
>>>>> {
>>>>>  File *files; /* palloc'd array with numFiles entries */
>>>>> ```
>>>>>
>>>>> However, it’s allocated by palloc_object():
>>>>> ```
>>>>> file->files = palloc_object(File);
>>>>> ```
>>>>>
>>>>> And reallocated by repalloc():
>>>>> ```
>>>>>  file->files = (File *) repalloc(file->files,
>>>>>      (file->numFiles + 1) * sizeof(File));
>>>>> ```
>>>>>
>>>>> This trivial patch just changes to use palloc_array/repalloc_array, which makes the intent clearer.
>>>>>
>>>>> Best regards,
>>>>> --
>>>>> Chao Li (Evan)
>>>>> HighGo Software Co., Ltd.
>>>>> https://www.highgo.com/
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> <v1-0001-Use-palloc_array-repalloc_array-for-BufFile-file-.patch>
>>>>
>>>>
>>>> Sorry for missing the reference of [1]:
>>>>
>>>> [1] https://postgr.es/m/aUStrqoOCDRFAq1M@paquier.xyz
>>>>
>>>> Best regards,
>>>> --
>>>> Chao Li (Evan)
>>>> HighGo Software Co., Ltd.
>>>> https://www.highgo.com/
>>>>
>>>
>>> PFA v2:
>>> * Rebased
>>> * Updated the commit message
>>
>> I've reviewed the v2 patch and here is a comment:
>>
>> - file->files = palloc_object(File);
>> + file->files = palloc_array(File, 1);
>>
>> I'm not a fan of this change. This change looks like trying to
>> distinguish allocated memory by palloc_object() and palloc_array()
>> even though underlying memory is identical. I'm concerned about this
>> change creating unnecessary coding conventions.
>>
>
> Hi Masahiho-san,
>
> Thank you so much for taking the time to review this patch.
>
> Actually, this change was my original motivation for creating the patch. When I first read the code, I was confused
whythe field was named with the plural “files" even though it was allocated with palloc_object(). After reading
further,I saw that the memory is later enlarged with repalloc, so it became clear that file->files is really meant to
bean array. 
>
> To me, the allocation method should reflect the actual meaning of the object. Here, file->files is an array, no
matterhow many elements it currently contains. Even if it only has one element at the moment, semantically it is still
anarray. 
>
> By “creating unnecessary coding conventions”, are you concerned that this change might encourage people to always use
palloc_array(type,1) when allocating a single object? That concern is understandable, but I think it should depend on
thereal semantics. If the memory is for a single object, then of course palloc_array() should be wrong. 
>
> Given the nature of C, a "File *" can point either to a single File object or to a File array. In that sense,
palloc_array()makes it explicit that this pointer is intended to represent an array, while palloc_object() suggests a
singleobject. So I think changing this to palloc_array() improves readability in this case. 
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/

Hi Masahiko-san,

Would you mind sharing any further thoughts on this?

While working further on this patch, I noticed this code in ginget.c:
```
        key->requiredEntries = palloc(1 * sizeof(GinScanEntry));
```

To me, this seems to use * 1 to make it explicit that key->requiredEntries is intended as an array, even though it
currentlycontains only one element. That feels similar to what I was trying to express with: " file->files =
palloc_array(File,1);”. But if you still feel this is not a good change, I’m happy to revert it. 

Also, while reviewing recent patches, I found a number of additional palloc/repalloc call sites that could be converted
tothe newer helpers, so I expanded the scope of this patch a bit. To avoid making it too large, I included only about
1/3of the cases I found in this version. If this patch moves forward, I’d be happy to clean up the remaining
occurrencesseparately. 

PFA v3.

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