Change xl_hash_vacuum_one_page.ntuples from int to uint16
Hi hackers,
While working on [1], I noticed that xl_hash_vacuum_one_page.ntuples is an int.
Unless I'm missing something, It seems to me that it would make more sense to use an uint16 (like this is done for
gistxlogDelete.ntodelete for example).
Please find attached a patch proposal to do so.
While that does not currently change the struct size:
No patch:
(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;
/* total size (bytes): 8 */
}
With 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;
/* XXX 2-byte padding */
/* total size (bytes): 8 */
}
It could reduce it when adding new fields (like this is is done in [1]).
We would get:
No patch:
(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 */
}
With 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 */
}
Means saving 4 bytes in that case.
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-xl_hash_vacuum_one_page_ntuples_as_uint16.patchtext/plain; charset=UTF-8; name=v1-0001-xl_hash_vacuum_one_page_ntuples_as_uint16.patchDownload
diff --git a/src/include/access/hash_xlog.h b/src/include/access/hash_xlog.h
index a2f0f39213..9894ab9afe 100644
--- a/src/include/access/hash_xlog.h
+++ b/src/include/access/hash_xlog.h
@@ -251,13 +251,13 @@ typedef struct xl_hash_init_bitmap_page
typedef struct xl_hash_vacuum_one_page
{
TransactionId snapshotConflictHorizon;
- int ntuples;
+ uint16 ntuples;
/* TARGET OFFSET NUMBERS FOLLOW AT THE END */
} xl_hash_vacuum_one_page;
#define SizeOfHashVacuumOnePage \
- (offsetof(xl_hash_vacuum_one_page, ntuples) + sizeof(int))
+ (offsetof(xl_hash_vacuum_one_page, ntuples) + sizeof(uint16))
extern void hash_redo(XLogReaderState *record);
extern void hash_desc(StringInfo buf, XLogReaderState *record);
On Tue, Jan 10, 2023 at 11:08:33AM +0100, Drouvot, Bertrand wrote:
While working on [1], I noticed that xl_hash_vacuum_one_page.ntuples is an int.
Unless I'm missing something, It seems to me that it would make more sense to use an uint16 (like this is done for
gistxlogDelete.ntodelete for example).
I think that is correct. This value is determined by looping through
offsets, which are uint16 as well. Should we also change the related
variables (e.g., ndeletable in _hash_vacuum_one_page()) to uint16?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi,
On 1/20/23 9:01 PM, Nathan Bossart wrote:
On Tue, Jan 10, 2023 at 11:08:33AM +0100, Drouvot, Bertrand wrote:
While working on [1], I noticed that xl_hash_vacuum_one_page.ntuples is an int.
Unless I'm missing something, It seems to me that it would make more sense to use an uint16 (like this is done for
gistxlogDelete.ntodelete for example).I think that is correct. This value is determined by looping through
offsets, which are uint16 as well.
Thanks for the review!
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?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Feb 15, 2023 at 3:35 AM Nathan Bossart <nathandbossart@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. I think the padding space we are trying to save here can be used
for the patch [1]/messages/by-id/2d62f212-fce6-d639-b9eb-2a5bc4bec3b4@gmail.com, right? 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.
I think this would require XLOG_PAGE_MAGIC as it changes the WAL record.
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.
[1]: /messages/by-id/2d62f212-fce6-d639-b9eb-2a5bc4bec3b4@gmail.com
--
With Regards,
Amit Kapila.
Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
On Wed, Feb 15, 2023 at 3:35 AM Nathan Bossart <nathandbossart@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
Hi,
On 2/16/23 1:26 PM, Drouvot, Bertrand wrote:
Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
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!
Please find V2 attached.
The commit message also mention the XLOG_PAGE_MAGIC bump.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Change-xl_hash_vacuum_one_page.ntuples-from-int-t.patchtext/plain; charset=UTF-8; name=v2-0001-Change-xl_hash_vacuum_one_page.ntuples-from-int-t.patchDownload
From a1f627a6e8c347bdb3112033ae08ace57fbf0a10 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Thu, 16 Feb 2023 15:01:29 +0000
Subject: [PATCH v2] 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.
Bump XLOG_PAGE_MAGIC.
---
src/include/access/hash_xlog.h | 4 ++--
src/include/access/xlog_internal.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
100.0% src/include/access/
diff --git a/src/include/access/hash_xlog.h b/src/include/access/hash_xlog.h
index a2f0f39213..9894ab9afe 100644
--- a/src/include/access/hash_xlog.h
+++ b/src/include/access/hash_xlog.h
@@ -251,13 +251,13 @@ typedef struct xl_hash_init_bitmap_page
typedef struct xl_hash_vacuum_one_page
{
TransactionId snapshotConflictHorizon;
- int ntuples;
+ uint16 ntuples;
/* TARGET OFFSET NUMBERS FOLLOW AT THE END */
} xl_hash_vacuum_one_page;
#define SizeOfHashVacuumOnePage \
- (offsetof(xl_hash_vacuum_one_page, ntuples) + sizeof(int))
+ (offsetof(xl_hash_vacuum_one_page, ntuples) + sizeof(uint16))
extern void hash_redo(XLogReaderState *record);
extern void hash_desc(StringInfo buf, XLogReaderState *record);
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 59fc7bc105..8edae7bb07 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -31,7 +31,7 @@
/*
* Each page of XLOG file has a header like this:
*/
-#define XLOG_PAGE_MAGIC 0xD111 /* can be used as WAL version indicator */
+#define XLOG_PAGE_MAGIC 0xD112 /* can be used as WAL version indicator */
typedef struct XLogPageHeaderData
{
--
2.34.1
On Thu, Feb 16, 2023 at 8:39 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
On 2/16/23 1:26 PM, Drouvot, Bertrand wrote:
Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
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!
Please find V2 attached.
The commit message also mention the XLOG_PAGE_MAGIC bump.
Thanks, I was not completely sure about whether we need to bump
XLOG_PAGE_MAGIC for this patch as this makes the additional space just
by changing the datatype of one of the members of the existing WAL
record. We normally change it for the addition/removal of new fields
aka change in WAL record format, or addition of a new type of WAL
record. Does anyone else have an opinion/suggestion on this matter?
--
With Regards,
Amit Kapila.
Hi
On 2023-02-17 08:30:09 +0530, Amit Kapila wrote:
Thanks, I was not completely sure about whether we need to bump
XLOG_PAGE_MAGIC for this patch as this makes the additional space just
by changing the datatype of one of the members of the existing WAL
record. We normally change it for the addition/removal of new fields
aka change in WAL record format, or addition of a new type of WAL
record. Does anyone else have an opinion/suggestion on this matter?
I'd definitely change it - the width of a field still means you can't really
parse the old WAL sensibly anymore.
Greetings,
Andres Freund
Hi,
On 2/16/23 1:26 PM, Drouvot, Bertrand wrote:
Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
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.
Please find attached a patch proposal to do so.
It looks like a Pandora's box as it produces
those cascading changes:
_hash_vacuum_one_page
index_compute_xid_horizon_for_tuples
gistprunepage
PageIndexMultiDelete
gistXLogDelete
PageIndexMultiDelete
spgRedoVacuumRedirect
vacuumRedirectAndPlaceholder
spgPageIndexMultiDelete
moveLeafs
doPickSplit
_bt_delitems_vacuum
btvacuumpage
_bt_delitems_delete
_bt_delitems_delete_check
hash_xlog_move_page_contents
gistvacuumpage
gistXLogUpdate
gistplacetopage
hashbucketcleanup
Which makes me:
- wonder it is not too intrusive (we could reduce the scope and keep the
PageIndexMultiDelete()'s nitems argument as an int for example).
- worry if there is no side effects (like the one I'm mentioning as a comment
in PageIndexMultiDelete()) even if it passes the CI tests.
- wonder if we should not change MaxIndexTuplesPerPage from int to uint16 too (given
the fact that the maximum block size is 32 KB.
I'm sharing this version but I still need to think about it and
I'm curious about your thoughts too.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Change-ndeletable-in-_hash_vacuum_one_page-from-i.patchtext/plain; charset=UTF-8; name=v1-0001-Change-ndeletable-in-_hash_vacuum_one_page-from-i.patchDownload
From dfb3d5cae3163ebc9073cc762adec966f17e2f33 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 17 Feb 2023 11:30:21 +0000
Subject: [PATCH v1] Change ndeletable in _hash_vacuum_one_page() from int to
uint16
deletable in _hash_vacuum_one_page() is assigned to xl_hash_vacuum_one_page.ntuples
which has been changed to uint16 in a previous commit.
This commit ensures that the assigned value has the same type.
Doing so produces those cascading changes:
_hash_vacuum_one_page
index_compute_xid_horizon_for_tuples
gistprunepage
PageIndexMultiDelete
gistXLogDelete
PageIndexMultiDelete
spgRedoVacuumRedirect
vacuumRedirectAndPlaceholder
spgPageIndexMultiDelete
moveLeafs
doPickSplit
_bt_delitems_vacuum
btvacuumpage
_bt_delitems_delete
_bt_delitems_delete_check
hash_xlog_move_page_contents
gistvacuumpage
gistXLogUpdate
gistplacetopage
hashbucketcleanup
which also fixed others missmatch on the way.
Also in some places the types have been changed from OffsetNumber to uint16.
Even if at the time of this commit the OffsetNumber is defined as uint16, it's
better to match the functions arguments types they are linked to.
---
src/backend/access/gist/gist.c | 6 +++---
src/backend/access/gist/gistvacuum.c | 2 +-
src/backend/access/gist/gistxlog.c | 4 ++--
src/backend/access/hash/hash.c | 2 +-
src/backend/access/hash/hash_xlog.c | 8 ++++----
src/backend/access/hash/hashinsert.c | 2 +-
src/backend/access/index/genam.c | 2 +-
src/backend/access/nbtree/nbtpage.c | 14 +++++++-------
src/backend/access/nbtree/nbtree.c | 4 ++--
src/backend/access/spgist/spgdoinsert.c | 8 ++++----
src/backend/access/spgist/spgvacuum.c | 4 ++--
src/backend/access/spgist/spgxlog.c | 2 +-
src/backend/storage/page/bufpage.c | 12 ++++++++----
src/include/access/genam.h | 2 +-
src/include/access/gist_private.h | 4 ++--
src/include/access/nbtree.h | 4 ++--
src/include/access/spgist_private.h | 2 +-
src/include/storage/bufpage.h | 2 +-
18 files changed, 44 insertions(+), 40 deletions(-)
14.8% src/backend/access/gist/
11.6% src/backend/access/hash/
25.0% src/backend/access/nbtree/
11.2% src/backend/access/spgist/
14.3% src/backend/storage/page/
20.1% src/include/access/
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index ba394f08f6..587fb04585 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -573,8 +573,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
{
if (RelationNeedsWAL(rel))
{
- OffsetNumber ndeloffs = 0,
- deloffs[1];
+ uint16 ndeloffs = 0;
+ OffsetNumber deloffs[1];
if (OffsetNumberIsValid(oldoffnum))
{
@@ -1642,7 +1642,7 @@ static void
gistprunepage(Relation rel, Page page, Buffer buffer, Relation heapRel)
{
OffsetNumber deletable[MaxIndexTuplesPerPage];
- int ndeletable = 0;
+ uint16 ndeletable = 0;
OffsetNumber offnum,
maxoff;
diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c
index 3f60d3274d..c72401508c 100644
--- a/src/backend/access/gist/gistvacuum.c
+++ b/src/backend/access/gist/gistvacuum.c
@@ -310,7 +310,7 @@ restart:
else if (GistPageIsLeaf(page))
{
OffsetNumber todelete[MaxOffsetNumber];
- int ntodelete = 0;
+ uint16 ntodelete = 0;
int nremain;
GISTPageOpaque opaque = GistPageGetOpaque(page);
OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index f65864254a..47f9dd0a86 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -631,7 +631,7 @@ gistXLogPageReuse(Relation rel, BlockNumber blkno, FullTransactionId deleteXid)
*/
XLogRecPtr
gistXLogUpdate(Buffer buffer,
- OffsetNumber *todelete, int ntodelete,
+ OffsetNumber *todelete, uint16 ntodelete,
IndexTuple *itup, int ituplen,
Buffer leftchildbuf)
{
@@ -671,7 +671,7 @@ gistXLogUpdate(Buffer buffer,
* standby queries and needs special handling.
*/
XLogRecPtr
-gistXLogDelete(Buffer buffer, OffsetNumber *todelete, int ntodelete,
+gistXLogDelete(Buffer buffer, OffsetNumber *todelete, uint16 ntodelete,
TransactionId snapshotConflictHorizon)
{
gistxlogDelete xlrec;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index eb258337d6..49d70232e4 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -709,7 +709,7 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf,
Buffer next_buf;
Page page;
OffsetNumber deletable[MaxOffsetNumber];
- int ndeletable = 0;
+ uint16 ndeletable = 0;
bool retain_pin = false;
bool clear_dead_marking = false;
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index f38b42efb9..6f7ee8b7ee 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -591,11 +591,11 @@ hash_xlog_move_page_contents(XLogReaderState *record)
if (len > 0)
{
- OffsetNumber *unused;
- OffsetNumber *unend;
+ uint16 *unused;
+ uint16 *unend;
- unused = (OffsetNumber *) ptr;
- unend = (OffsetNumber *) ((char *) ptr + len);
+ unused = (uint16 *) ptr;
+ unend = (uint16 *) ((char *) ptr + len);
if ((unend - unused) > 0)
PageIndexMultiDelete(page, unused, unend - unused);
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index a604e31891..014008f99f 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -372,7 +372,7 @@ static void
_hash_vacuum_one_page(Relation rel, Relation hrel, Buffer metabuf, Buffer buf)
{
OffsetNumber deletable[MaxOffsetNumber];
- int ndeletable = 0;
+ uint16 ndeletable = 0;
OffsetNumber offnum,
maxoff;
Page page = BufferGetPage(buf);
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 722927aeba..c837ff7a2f 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -295,7 +295,7 @@ index_compute_xid_horizon_for_tuples(Relation irel,
Relation hrel,
Buffer ibuf,
OffsetNumber *itemnos,
- int nitems)
+ uint16 nitems)
{
TM_IndexDeleteOp delstate;
TransactionId snapshotConflictHorizon = InvalidTransactionId;
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 3feee28d19..ddc6ad0e45 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -42,8 +42,8 @@ static void _bt_log_reuse_page(Relation rel, BlockNumber blkno,
FullTransactionId safexid);
static void _bt_delitems_delete(Relation rel, Buffer buf,
TransactionId snapshotConflictHorizon,
- OffsetNumber *deletable, int ndeletable,
- BTVacuumPosting *updatable, int nupdatable);
+ OffsetNumber *deletable, uint16 ndeletable,
+ BTVacuumPosting *updatable, uint16 nupdatable);
static char *_bt_delitems_update(BTVacuumPosting *updatable, int nupdatable,
OffsetNumber *updatedoffsets,
Size *updatedbuflen, bool needswal);
@@ -1164,8 +1164,8 @@ _bt_pageinit(Page page, Size size)
*/
void
_bt_delitems_vacuum(Relation rel, Buffer buf,
- OffsetNumber *deletable, int ndeletable,
- BTVacuumPosting *updatable, int nupdatable)
+ OffsetNumber *deletable, uint16 ndeletable,
+ BTVacuumPosting *updatable, uint16 nupdatable)
{
Page page = BufferGetPage(buf);
BTPageOpaque opaque;
@@ -1295,8 +1295,8 @@ _bt_delitems_vacuum(Relation rel, Buffer buf,
static void
_bt_delitems_delete(Relation rel, Buffer buf,
TransactionId snapshotConflictHorizon,
- OffsetNumber *deletable, int ndeletable,
- BTVacuumPosting *updatable, int nupdatable)
+ OffsetNumber *deletable, uint16 ndeletable,
+ BTVacuumPosting *updatable, uint16 nupdatable)
{
Page page = BufferGetPage(buf);
BTPageOpaque opaque;
@@ -1532,7 +1532,7 @@ _bt_delitems_delete_check(Relation rel, Buffer buf, Relation heapRel,
Page page = BufferGetPage(buf);
TransactionId snapshotConflictHorizon;
OffsetNumber postingidxoffnum = InvalidOffsetNumber;
- int ndeletable = 0,
+ uint16 ndeletable = 0,
nupdatable = 0;
OffsetNumber deletable[MaxIndexTuplesPerPage];
BTVacuumPosting updatable[MaxIndexTuplesPerPage];
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 1cc88da032..ba07fd4c7f 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -1151,9 +1151,9 @@ backtrack:
else if (P_ISLEAF(opaque))
{
OffsetNumber deletable[MaxIndexTuplesPerPage];
- int ndeletable;
+ uint16 ndeletable;
BTVacuumPosting updatable[MaxIndexTuplesPerPage];
- int nupdatable;
+ uint16 nupdatable;
OffsetNumber offnum,
minoff,
maxoff;
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index 3554edcc9a..7b59ba87a9 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -130,7 +130,7 @@ cmpOffsetNumbers(const void *a, const void *b)
*/
void
spgPageIndexMultiDelete(SpGistState *state, Page page,
- OffsetNumber *itemnos, int nitems,
+ OffsetNumber *itemnos, uint16 nitems,
int firststate, int reststate,
BlockNumber blkno, OffsetNumber offnum)
{
@@ -389,8 +389,8 @@ moveLeafs(Relation index, SpGistState *state,
SPPageDesc *current, SPPageDesc *parent,
SpGistLeafTuple newLeafTuple, bool isNulls)
{
+ uint16 nDelete;
int i,
- nDelete,
nInsert,
size;
Buffer nbuf;
@@ -711,8 +711,8 @@ doPickSplit(Relation index, SpGistState *state,
char *leafdata,
*leafptr;
SPPageDesc saveCurrent;
- int nToDelete,
- nToInsert,
+ uint16 nToDelete;
+ int nToInsert,
maxToInclude;
in.level = level;
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c
index 3adb18f2d8..64d9803cef 100644
--- a/src/backend/access/spgist/spgvacuum.c
+++ b/src/backend/access/spgist/spgvacuum.c
@@ -493,8 +493,8 @@ vacuumRedirectAndPlaceholder(Relation index, Buffer buffer)
{
Page page = BufferGetPage(buffer);
SpGistPageOpaque opaque = SpGistPageGetOpaque(page);
- OffsetNumber i,
- max = PageGetMaxOffsetNumber(page),
+ uint16 i;
+ OffsetNumber max = PageGetMaxOffsetNumber(page),
firstPlaceholder = InvalidOffsetNumber;
bool hasNonPlaceholder = false;
bool hasUpdate = false;
diff --git a/src/backend/access/spgist/spgxlog.c b/src/backend/access/spgist/spgxlog.c
index b071b59c8a..34b14b01a8 100644
--- a/src/backend/access/spgist/spgxlog.c
+++ b/src/backend/access/spgist/spgxlog.c
@@ -886,7 +886,7 @@ spgRedoVacuumRedirect(XLogReaderState *record)
{
Page page = BufferGetPage(buffer);
SpGistPageOpaque opaque = SpGistPageGetOpaque(page);
- int i;
+ uint16 i;
/* Convert redirect pointers to plain placeholders */
for (i = 0; i < xldata->nToPlaceholder; i++)
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 92994f8f39..f71ab47496 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -1158,7 +1158,7 @@ PageIndexTupleDelete(Page page, OffsetNumber offnum)
* of item numbers to be deleted in item number order!
*/
void
-PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems)
+PageIndexMultiDelete(Page page, OffsetNumber *itemnos, uint16 nitems)
{
PageHeader phdr = (PageHeader) page;
Offset pd_lower = phdr->pd_lower;
@@ -1177,6 +1177,7 @@ PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems)
int nextitm;
OffsetNumber offnum;
bool presorted = true; /* For now */
+ int nbitems = nitems;
Assert(nitems <= MaxIndexTuplesPerPage);
@@ -1188,10 +1189,13 @@ PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems)
*
* TODO: tune the magic number here
*/
- if (nitems <= 2)
+ if (nbitems <= 2)
{
- while (--nitems >= 0)
- PageIndexTupleDelete(page, itemnos[nitems]);
+ /*
+ * Be careful that nbitems has to be an int and not an uint16.
+ */
+ while (--nbitems >= 0)
+ PageIndexTupleDelete(page, itemnos[nbitems]);
return;
}
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 83dbee0fe6..4e9d9dc0a1 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -208,7 +208,7 @@ extern TransactionId index_compute_xid_horizon_for_tuples(Relation irel,
Relation hrel,
Buffer ibuf,
OffsetNumber *itemnos,
- int nitems);
+ uint16 nitems);
/*
* heap-or-index access to system catalogs (in genam.c)
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 8af33d7b40..46e9b3799e 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -444,12 +444,12 @@ extern void gistXLogPageReuse(Relation rel, BlockNumber blkno,
FullTransactionId deleteXid);
extern XLogRecPtr gistXLogUpdate(Buffer buffer,
- OffsetNumber *todelete, int ntodelete,
+ OffsetNumber *todelete, uint16 ntodelete,
IndexTuple *itup, int ituplen,
Buffer leftchildbuf);
extern XLogRecPtr gistXLogDelete(Buffer buffer, OffsetNumber *todelete,
- int ntodelete, TransactionId snapshotConflictHorizon);
+ uint16 ntodelete, TransactionId snapshotConflictHorizon);
extern XLogRecPtr gistXLogSplit(bool page_is_leaf,
SplitedPageLayout *dist,
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 8f48960f9d..a52a8a926f 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1216,8 +1216,8 @@ extern bool _bt_conditionallockbuf(Relation rel, Buffer buf);
extern void _bt_upgradelockbufcleanup(Relation rel, Buffer buf);
extern void _bt_pageinit(Page page, Size size);
extern void _bt_delitems_vacuum(Relation rel, Buffer buf,
- OffsetNumber *deletable, int ndeletable,
- BTVacuumPosting *updatable, int nupdatable);
+ OffsetNumber *deletable, uint16 ndeletable,
+ BTVacuumPosting *updatable, uint16 nupdatable);
extern void _bt_delitems_delete_check(Relation rel, Buffer buf,
Relation heapRel,
TM_IndexDeleteOp *delstate);
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index c6ef46fc20..6a144145d2 100644
--- a/src/include/access/spgist_private.h
+++ b/src/include/access/spgist_private.h
@@ -534,7 +534,7 @@ extern bool spgproperty(Oid index_oid, int attno,
extern void spgUpdateNodeLink(SpGistInnerTuple tup, int nodeN,
BlockNumber blkno, OffsetNumber offset);
extern void spgPageIndexMultiDelete(SpGistState *state, Page page,
- OffsetNumber *itemnos, int nitems,
+ OffsetNumber *itemnos, uint16 nitems,
int firststate, int reststate,
BlockNumber blkno, OffsetNumber offnum);
extern bool spgdoinsert(Relation index, SpGistState *state,
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 424ecba028..293bfd54be 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -500,7 +500,7 @@ extern Size PageGetFreeSpaceForMultipleTuples(Page page, int ntups);
extern Size PageGetExactFreeSpace(Page page);
extern Size PageGetHeapFreeSpace(Page page);
extern void PageIndexTupleDelete(Page page, OffsetNumber offnum);
-extern void PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems);
+extern void PageIndexMultiDelete(Page page, OffsetNumber *itemnos, uint16 nitems);
extern void PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offnum);
extern bool PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
Item newtup, Size newsize);
--
2.34.1
On Fri, Feb 17, 2023 at 9:43 AM Andres Freund <andres@anarazel.de> wrote:
On 2023-02-17 08:30:09 +0530, Amit Kapila wrote:
Thanks, I was not completely sure about whether we need to bump
XLOG_PAGE_MAGIC for this patch as this makes the additional space just
by changing the datatype of one of the members of the existing WAL
record. We normally change it for the addition/removal of new fields
aka change in WAL record format, or addition of a new type of WAL
record. Does anyone else have an opinion/suggestion on this matter?I'd definitely change it - the width of a field still means you can't really
parse the old WAL sensibly anymore.
Okay, thanks for your input. I'll push this patch early next week.
--
With Regards,
Amit Kapila.
On Fri, Feb 17, 2023 at 7:44 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
On 2/16/23 1:26 PM, Drouvot, Bertrand wrote:
Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
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.
Pushed the first patch.
Please find attached a patch proposal to do so.
It looks like a Pandora's box as it produces
those cascading changes:_hash_vacuum_one_page
index_compute_xid_horizon_for_tuples
gistprunepage
PageIndexMultiDelete
gistXLogDelete
PageIndexMultiDelete
spgRedoVacuumRedirect
vacuumRedirectAndPlaceholder
spgPageIndexMultiDelete
moveLeafs
doPickSplit
_bt_delitems_vacuum
btvacuumpage
_bt_delitems_delete
_bt_delitems_delete_check
hash_xlog_move_page_contents
gistvacuumpage
gistXLogUpdate
gistplacetopage
hashbucketcleanupWhich makes me:
- wonder it is not too intrusive (we could reduce the scope and keep the
PageIndexMultiDelete()'s nitems argument as an int for example).- worry if there is no side effects (like the one I'm mentioning as a comment
in PageIndexMultiDelete()) even if it passes the CI tests.- wonder if we should not change MaxIndexTuplesPerPage from int to uint16 too (given
the fact that the maximum block size is 32 KB.I'm sharing this version but I still need to think about it and
I'm curious about your thoughts too.
@@ -591,11 +591,11 @@ hash_xlog_move_page_contents(XLogReaderState *record)
if (len > 0)
{
- OffsetNumber *unused;
- OffsetNumber *unend;
+ uint16 *unused;
+ uint16 *unend;
- unused = (OffsetNumber *) ptr;
- unend = (OffsetNumber *) ((char *) ptr + len);
+ unused = (uint16 *) ptr;
+ unend = (uint16 *) ((char *) ptr + len);
It doesn't seem useful to me to make such changes. About other changes
in the second patch, it is not clear whether there is much value
addition by those even though I don't see anything wrong with them.
So, let's see if Nathan or others see the value in the proposed patch
or any subset of these changes.
--
With Regards,
Amit Kapila.
Hi,
On 2/27/23 6:27 AM, Amit Kapila wrote:
On Fri, Feb 17, 2023 at 7:44 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:On 2/16/23 1:26 PM, Drouvot, Bertrand wrote:
Hi,
On 2/16/23 12:00 PM, Amit Kapila wrote:
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.Pushed the first patch.
Thanks!
Please find attached a patch proposal to do so.
It looks like a Pandora's box as it produces
those cascading changes:_hash_vacuum_one_page
index_compute_xid_horizon_for_tuples
gistprunepage
PageIndexMultiDelete
gistXLogDelete
PageIndexMultiDelete
spgRedoVacuumRedirect
vacuumRedirectAndPlaceholder
spgPageIndexMultiDelete
moveLeafs
doPickSplit
_bt_delitems_vacuum
btvacuumpage
_bt_delitems_delete
_bt_delitems_delete_check
hash_xlog_move_page_contents
gistvacuumpage
gistXLogUpdate
gistplacetopage
hashbucketcleanupWhich makes me:
- wonder it is not too intrusive (we could reduce the scope and keep the
PageIndexMultiDelete()'s nitems argument as an int for example).- worry if there is no side effects (like the one I'm mentioning as a comment
in PageIndexMultiDelete()) even if it passes the CI tests.- wonder if we should not change MaxIndexTuplesPerPage from int to uint16 too (given
the fact that the maximum block size is 32 KB.I'm sharing this version but I still need to think about it and
I'm curious about your thoughts too.@@ -591,11 +591,11 @@ hash_xlog_move_page_contents(XLogReaderState *record)
if (len > 0) { - OffsetNumber *unused; - OffsetNumber *unend; + uint16 *unused; + uint16 *unend;- unused = (OffsetNumber *) ptr; - unend = (OffsetNumber *) ((char *) ptr + len); + unused = (uint16 *) ptr; + unend = (uint16 *) ((char *) ptr + len);It doesn't seem useful to me to make such changes.
Yeah, the OffsetNumber is currently defined as uint16, but I wonder if it's
not better that those matches the functions arguments types they are linked to (should OffsetNumber
or the functions arguments types change).
About other changes
in the second patch, it is not clear whether there is much value
addition by those even though I don't see anything wrong with them.
So, let's see if Nathan or others see the value in the proposed patch
or any subset of these changes.
+1.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com