Re: Add counted_by attribute

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" <tristan(at)partin(dot)io> 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 Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)