Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
On Wed, Feb 15, 2023 at 3:35 AM Nathan Bossart <nathandboss...@gmail.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
