From: | "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Nathan Bossart <nathandbossart(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16 |
Date: | 2023-02-16 12:26:00 |
Message-ID: | c38dd169-b704-8394-4274-0bc9b33a7b9e@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
> On Wed, Feb 15, 2023 at 3:35 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>>
>> On Sat, Jan 21, 2023 at 06:42:08AM +0100, Drouvot, Bertrand wrote:
>>> On 1/20/23 9:01 PM, Nathan Bossart wrote:
>>>> Should we also change the related
>>>> variables (e.g., ndeletable in _hash_vacuum_one_page()) to uint16?
>>>
>>> Yeah, I thought about it too. What I saw is that there is other places that would be good candidates for the same
>>> kind of changes (see the int ntodelete argument in gistXLogDelete() being assigned to gistxlogDelete.ntodelete (uint16) for example).
>>>
>>> So, what do you think about:
>>>
>>> 1) keep this patch as it is (to "only" address the struct field and avoid possible future "useless" padding size increase)
>>> and
>>> 2) create a new patch (once this one is committed) to align the types for variables/arguments with the structs (related to XLOG records) fields when they are not?
>>
>> Okay. I've marked this one as ready-for-committer, then.
>>
>
> LGTM.
Thanks for looking at it!
> I think the padding space we are trying to save here can be used
> for the patch [1], right?
Yes exactly, without the current patch and adding isCatalogRel (from
the patch [1] you mentioned) we would get:
(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 3-byte padding */
/* total size (bytes): 12 */
}
While with the proposed patch:
(gdb) ptype /o struct xl_hash_vacuum_one_page
/* offset | size */ type = struct xl_hash_vacuum_one_page {
/* 0 | 4 */ TransactionId snapshotConflictHorizon;
/* 4 | 2 */ uint16 ntuples;
/* 6 | 1 */ _Bool isCatalogRel;
/* XXX 1-byte padding */
/* total size (bytes): 8 */
}
> BTW, feel free to create the second patch
> (to align the types for variables/arguments) as that would be really
> helpful after we commit this one.
>
Yes, will do.
> I think this would require XLOG_PAGE_MAGIC as it changes the WAL record.
>
Oh, I Was not aware about it, thanks! Will do in V2 (and in the logical
decoding on standby patch as it adds the new field mentioned above).
> BTW, how about a commit message like:
> Change xl_hash_vacuum_one_page.ntuples from int to uint16.
>
> This will create two bytes of padding space in xl_hash_vacuum_one_page
> which can be used for future patches. This makes the datatype of
> xl_hash_vacuum_one_page.ntuples same as gistxlogDelete.ntodelete which
> is advisable as both are used for the same purpose.
>
LGTM, will add it to V2!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From | Date | Subject | |
---|---|---|---|
Next Message | Bharath Rupireddy | 2023-02-16 12:30:00 | Re: pg_walinspect memory leaks |
Previous Message | Etsuro Fujita | 2023-02-16 11:49:57 | Re: Some revises in adding sorting path |