Re: Add counted_by attribute

On Fri Sep 25, 2026 at 4:02 PM UTC, Japin Li wrote:
>
> Hi
> Hi Tristan,
>
> On Thu, 24 Sep 2026 at 16:17, "Tristan Partin" <tristan(at)partin(dot)io> 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" <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!
>
> Thanks for the clear explanation.
>
> I agree with you on the cautious approach — especially for structures that
> may be populated from the filesystem, or where the count field does not
> strictly represent the allocated/accessible length of the flexible array
> member.
>
> While reviewing remaining FAMs that do not yet use pg_attribute_counted_by(),
> two cases looked like solid candidates:
>
> 1. BackgroundWorkerArray in bgworker.c
>
> typedef struct BackgroundWorkerArray
> {
> int total_slots;
> uint32 parallel_register_count;
> uint32 parallel_terminate_count;
> BackgroundWorkerSlot slot[FLEXIBLE_ARRAY_MEMBER];
> } BackgroundWorkerArray;
>
> The total_slots is set once at shared-memory initialization (based on
> max_worker_processes) and never changes afterwards. All accesses are of the
> form slot[i] with i < total_slots. The invariant is therefore trivial to
> maintain.
>
> 2. vbits in contrib/pg_visibility/pg_visibility.c
>
> typedef struct vbits
> {
> BlockNumber next;
> BlockNumber count;
> uint8 bits[FLEXIBLE_ARRAY_MEMBER];
> } vbits;
>
> The structure is allocated and filled in one place (collect_visibility_data),
> with count equal to the number of blocks. Subsequent use is strictly read-only
> and always checks next < count before indexing into bits[].
>
> Both are purely internal, the count field matches the actual array bound, and
> the update order is safe. I think it would be worthwhile to annotate them.
>
> Thoughts?

Great catches. I have included those in the next version of commits.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)