Hi,
On 12/22/22 8:50 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 12/21/22 10:06 AM, Drouvot, Bertrand wrote:
>> Hi,
>>
>> On 12/20/22 10:41 PM, Robert Haas wrote:
>>> On Tue, Dec 20, 2022 at 3:39 PM Robert Haas <robertmhaas@gmail.com> wrote:
>>> I guess whatever else we
>>> do here, we should fix the comments.
>>>
>>
>> Agree, please find attached a patch proposal doing so.
>>
>>
>>> Bottom line is that I think the two cases that have alignment issues
>>> as coded are xl_hash_vacuum_one_page and gistxlogDelete. Everything
>>> else is OK, as far as I can tell right now.
>>>
>>
>> Thanks a lot for the repro(s) and explanations! That's very useful/helpful.
>>
>> Based on your discovery about the wrong comments above, I'm now tempted to fix those 2 alignment issues
>> by using a FLEXIBLE_ARRAY_MEMBER within those structs (as you proposed in [1]) (as that should also prevent
>> any possible wrong comments about where the array is located).
>>
>> What do you think?
>
> As mentioned above, It looks to me that making use of a FLEXIBLE_ARRAY_MEMBER is a good choice.
> So, please find attached v35 making use of a FLEXIBLE_ARRAY_MEMBER in xl_hash_vacuum_one_page and gistxlogDelete
(your2 repros are not failing anymore).
> I've also added a few words in the commit message in 0001 about it.
>
> So, we end up with:
>
> (gdb) ptype /o struct xl_hash_vacuum_one_page
> /* offset | size */ type = struct xl_hash_vacuum_one_page {
> /* 0 | 4 */ TransactionId snapshotConflictHorizon;
> /* 4 | 4 */ int ntuples;
> /* 8 | 1 */ _Bool isCatalogRel;
> /* XXX 1-byte hole */
> /* 10 | 0 */ OffsetNumber offsets[];
> /* XXX 2-byte padding */
>
> /* total size (bytes): 12 */
> }
>
> (gdb) ptype /o struct gistxlogDelete
> /* offset | size */ type = struct gistxlogDelete {
> /* 0 | 4 */ TransactionId snapshotConflictHorizon;
> /* 4 | 2 */ uint16 ntodelete;
> /* 6 | 1 */ _Bool isCatalogRel;
> /* XXX 1-byte hole */
> /* 8 | 0 */ OffsetNumber offsets[];
>
> /* total size (bytes): 8 */
> }
>
> While looking at it, I've a question: xl_hash_vacuum_one_page.ntuples is an int, do you see any reason why it is not
anuint16? (we would get rid of 4 bytes in the struct).
>
Please find attached v36, tiny rebase due to 1de58df4fe.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com