> On Sep 24, 2026, at 12:17 PM, Tristan Partin <[email protected]> wrote:
>
> On Wed Sep 23, 2026 at 6:33 AM CDT, Japin Li wrote:
>>
>> Hi, Tristan
>>
>> Thanks for updating the patches.
>>
>> On Wed, 23 Sep 2026 at 07:26, "Tristan Partin" <[email protected]> wrote:
>>> On Tue Sep 22, 2026 at 5:28 AM UTC, Peter Eisentraut wrote:
>>>> On 30.07.26 00:07, Tristan Partin wrote:
>>>>> The counted_by[0] compiler attribute is fairly new. It was added in GCC
>>>>> 15 and Clang 18. It has been used fairly extensively in the Linux
>>>>> kernel[0].
>>>>>
>>>>> To summarize the benefits of the attribute:
>>>>>
>>>>> - Runtime bounds checking with -DFORTIFY_SOURCE=3 and -fsanitize-bounds
>>>>> - Accurate reporting of __builtin_dynamic_object_size()
>>>>>
>>>>> While we don't use __builtin_dynamic_object_size(), I think the runtime
>>>>> bounds checking improvements are easily worth the little bit of effort
>>>>> to add the attribute in various locations and review the code. I think
>>>>> it will improve things for buildfarm animals using ASan due to expanded
>>>>> coverage.
>>>>
>>>> I took a closer look at this. There are several problems with the
>>>> proposed patches.
>>>>
>>>> 1) In C++, both gcc and clang have __has_attribute(counted_by) return 1
>>>> (true), but the compiler actually rejects the attribute with a warning.
>>>> This is not immediately evident in your patch, but it would show up
>>>> under cpluspluscheck and whenever we extend this attribute to header
>>>> files that happen to get pulled in by C++ source files.
>>>> (access/tupdesc.h is an obvious candidate.) Therefore, there needs to be
>>>> some #ifndef __cplusplus somewhere.
>>>
>>> I would love to understand the rationale for returning 1 when the
>>> compiler will just throw a warning anyway. Fixed.
>>>
>>>> 2) gcc 15 and clang 18 accept the counted_by attribute only for flexible
>>>> array members, not for pointers members. (Using it on a pointer causes
>>>> an error.) If you want to apply this to pointer members, as your patch
>>>> does in buffile.c, you'd have to write a configure test. Or else
>>>> restrict it to flexible array members for now.
>>>
>>> I like the idea of restricting it to flexible array members for now.
>>> It'll make for an easier review. Maybe in a subsequent patch we can
>>> raise the minimum compiler versions of using pg_attribute_counted_by()
>>> to GCC 16 and Clang 21.
>>>
>>>> 3) The counted_by attribute requires that, when extending the counted
>>>> array, the count field is increased before writing into the new element
>>>> at the end. The code dealing with struct BufFile currently doesn't do
>>>> that, and so your change in buffile.c fails under -fsanitize=bounds:
>>>>
>>>> ../src/backend/storage/file/buffile.c:919:3: runtime error: index 1 out
>>>> of bounds for type 'File * __counted_by(numFiles)' (aka 'int *')
>>>>
>>>> (Reproduce with meson configure -Db_sanitize=bounds and meson test ...
>>>> --suite regress.)
>>>>
>>>> The code needs to be carefully analyzed and adjusted to fix this. (The
>>>> code for the tuplesort.c change appears to be ok.)
>>>
>>> Good catch. In the upcoming changes, I ran test suites with
>>> -fsanitize=bounds, and found one place that needed a fix. Note that
>>> changes to buffile.c are not currently in scope for this patchset since
>>> it wasn't a flexible array member.
>>>
>>>> 4) Although the compilers are flexible with the placement, the most
>>>> correct placement of the attribute is at the beginning of the
>>>> declaration, like
>>>>
>>>> pg_attribute_counted_by(nTapes) TapeShare tapes[FLEXIBLE_ARRAY_MEMBER];
>>>>
>>>> (Note that the gcc documentation effectively writes it this way.)
>>>
>>> The second patch uses postfix notation, but subsequent patches enable
>>> support for prefix notation. I'll let you be the judge of whether to
>>> commit prefix or postfix. Commits 3 & 4 are genuine improvements, though
>>> they do also enable prefix support.
>>>
>>>> Additionally, with this arrangement, we could also make use of the MSVC
>>>> _Field_size_ annotation.
>>>
>>> This is good motivation.
>>>
>>>> 5) Minor: The counted_by attribute only takes a single argument, so the
>>>> use of __VA_ARGS__ seems excessive.
>>>
>>> I think I just blindly copied surrounding macro code and forgot to
>>> change it. Fixed in this new version.
>>>
>>>> 6) Minor: Awkward wording in comment: "This provides the compiler to
>>>> improve ..." -> "enables the compiler ..."?
>>>
>>> Fixed.
>>>
>>>> Suggestion:
>>>> - Add C++ guard. (Maybe add annotation in access/tupdesc.h to test.)
>>>> - Skip use of the attribute on pointer members for now.
>>>> - Make sure cpluspluscheck and -fsanitize=bounds pass.
>>>> - Consider the cosmetic adjustments mentioned.
>>>
>>> Thanks for the review.
>>>
>>
>> I tested it locally, and all tests passed.
>>
>> I found some places that could use the new pg_attribute_counted_by()
>> attribute
>> e.g., in heapam_xlog.h, xact.h, etc. Are those intentional omissions?
>>
>> Was this an oversight, or is pg_attribute_counted_by not needed here?
>>
>> Haven't checked everywhere yet. If it is oversight, I'll check it later.
>
> I originally had these as well, but my understanding is that these
> structures are populated from the filesystem, so in the event of data
> corruption, the count field may no longer be accurate and could cause
> a runtime failure similar to what Peter mentioned above in his review.
> It's possible that I am being too cautious though. Let me know what you
> think. I should have brought this up in my last email, but now is as
> good a time as any to discuss!
Tristan, nice work. I'm +1 for these changes. Is there a way to codify
that difference so that future hackers don't fall into this same trap?
-greg
> --
> Tristan Partin
> PostgreSQL Contributors Team
> AWS (https://aws.amazon.com)
