Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t43879psql -h localhost -U postgresBuilt from patchset v12 (message #12), September 20, 2026 at 03:22 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t43879_12 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t43879_12 && git checkout t43879_12Patchset v12 (message #12) is on t43879_12
Hi,
We are memset-ting the special space page that's already set to zeros
by PageInit in BloomInitPage, GinInitPage and SpGistInitPage. We have
already removed the memset after PageInit in gistinitpage (see the
comment there). Unless I'm missing something, IMO they are redundant.
I'm attaching a small patch that gets rid of the extra memset calls.
While on it, I removed MAXALIGN(sizeof(SpGistPageOpaqueData)) in
SpGistInitPage because the PageInit will anyways align the
specialSize. This change is inline with other places (such as
BloomInitPage, brin_page_init GinInitPage, gistinitpage,
_hash_pageinit and so on) where we just pass the size of special space
data structure.
I didn't see any regression test failure on my dev system with the
attached patch.
Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Remove-extra-memset-in-BloomInitPage-GinInitPage-.patchapplication/octet-stream; name=v1-0001-Remove-extra-memset-in-BloomInitPage-GinInitPage-.patchDownload+1-5
On Mon, 22 Mar 2021 at 10:16, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Hi,
We are memset-ting the special space page that's already set to zeros
by PageInit in BloomInitPage, GinInitPage and SpGistInitPage. We have
already removed the memset after PageInit in gistinitpage (see the
comment there). Unless I'm missing something, IMO they are redundant.
I'm attaching a small patch that gets rid of the extra memset calls.While on it, I removed MAXALIGN(sizeof(SpGistPageOpaqueData)) in
SpGistInitPage because the PageInit will anyways align the
specialSize. This change is inline with other places (such as
BloomInitPage, brin_page_init GinInitPage, gistinitpage,
_hash_pageinit and so on) where we just pass the size of special space
data structure.I didn't see any regression test failure on my dev system with the
attached patch.Thoughts?
Your changes look to fine me and I am also not getting any failure. I
think we should back-patch all the branches.
Patch is applying to all the branches(till v95) and there is no failure.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
On Mon, Mar 22, 2021 at 10:16 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Hi,
We are memset-ting the special space page that's already set to zeros
by PageInit in BloomInitPage, GinInitPage and SpGistInitPage. We have
already removed the memset after PageInit in gistinitpage (see the
comment there). Unless I'm missing something, IMO they are redundant.
I'm attaching a small patch that gets rid of the extra memset calls.While on it, I removed MAXALIGN(sizeof(SpGistPageOpaqueData)) in
SpGistInitPage because the PageInit will anyways align the
specialSize. This change is inline with other places (such as
BloomInitPage, brin_page_init GinInitPage, gistinitpage,
_hash_pageinit and so on) where we just pass the size of special space
data structure.I didn't see any regression test failure on my dev system with the
attached patch.Thoughts?
The changes look fine to me.
Regards,
Vignesh
On Sat, Apr 3, 2021 at 3:09 PM vignesh C <vignesh21@gmail.com> wrote:
On Mon, Mar 22, 2021 at 10:16 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
We are memset-ting the special space page that's already set to zeros
by PageInit in BloomInitPage, GinInitPage and SpGistInitPage. We have
already removed the memset after PageInit in gistinitpage (see the
comment there). Unless I'm missing something, IMO they are redundant.
I'm attaching a small patch that gets rid of the extra memset calls.While on it, I removed MAXALIGN(sizeof(SpGistPageOpaqueData)) in
SpGistInitPage because the PageInit will anyways align the
specialSize. This change is inline with other places (such as
BloomInitPage, brin_page_init GinInitPage, gistinitpage,
_hash_pageinit and so on) where we just pass the size of special space
data structure.I didn't see any regression test failure on my dev system with the
attached patch.Thoughts?
The changes look fine to me.
Thanks!
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Mon, Mar 22, 2021 at 10:58:17AM +0530, Mahendra Singh Thalor wrote:
Your changes look to fine me and I am also not getting any failure. I
think we should back-patch all the branches.Patch is applying to all the branches(till v95) and there is no failure.
Er, no. This is just some duplicated code with no extra effect. I
have no objection to simplify a bit the whole on readability and
consistency grounds (will do so tomorrow), including the removal of
the commented-out memset call in gistinitpage, but this is not
something that should be backpatched.
--
Michael
On Tue, Apr 6, 2021 at 6:09 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Mar 22, 2021 at 10:58:17AM +0530, Mahendra Singh Thalor wrote:
Your changes look to fine me and I am also not getting any failure. I
think we should back-patch all the branches.Patch is applying to all the branches(till v95) and there is no failure.
Er, no. This is just some duplicated code with no extra effect. I
have no objection to simplify a bit the whole on readability and
consistency grounds (will do so tomorrow), including the removal of
the commented-out memset call in gistinitpage, but this is not
something that should be backpatched.
+1 to not backport this patch because it's not a bug or not even a
critical issue. Having said that removal of these unnecessary memsets
would not only be better for readability and consistency but also can
reduce few extra function call costs(although minimal) while adding
new index pages.
Please find the v3 patch that removed the commented-out memset call in
gistinitpage.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v3-0001-Remove-extra-memset-in-BloomInitPage-GinInitPage-.patchapplication/x-patch; name=v3-0001-Remove-extra-memset-in-BloomInitPage-GinInitPage-.patchDownload+2-9
On Tue, 6 Apr 2021 at 19:14, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Tue, Apr 6, 2021 at 6:09 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Mar 22, 2021 at 10:58:17AM +0530, Mahendra Singh Thalor wrote:
Your changes look to fine me and I am also not getting any failure. I
think we should back-patch all the branches.Patch is applying to all the branches(till v95) and there is no failure.
Er, no. This is just some duplicated code with no extra effect. I
have no objection to simplify a bit the whole on readability and
consistency grounds (will do so tomorrow), including the removal of
the commented-out memset call in gistinitpage, but this is not
something that should be backpatched.+1 to not backport this patch because it's not a bug or not even a
critical issue. Having said that removal of these unnecessary memsets
would not only be better for readability and consistency but also can
reduce few extra function call costs(although minimal) while adding
new index pages.Please find the v3 patch that removed the commented-out memset call in
gistinitpage.
Thanks Bharath for updated patch.
+++ b/src/backend/storage/page/bufpage.c
@@ -51,7 +51,7 @@ PageInit(Page page, Size pageSize, Size specialSize)
/* Make sure all fields of page are zero, as well as unused space */
MemSet(p, 0, pageSize);- p->pd_flags = 0;
+ /* p->pd_flags = 0; done by above MemSet */I think, for readability we can keep old code here or we can remove
new added comment also.
Apart from this, all other changes looks good to me.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
On Wed, Apr 7, 2021 at 12:07 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
+++ b/src/backend/storage/page/bufpage.c @@ -51,7 +51,7 @@ PageInit(Page page, Size pageSize, Size specialSize) /* Make sure all fields of page are zero, as well as unused space */ MemSet(p, 0, pageSize);- p->pd_flags = 0; + /* p->pd_flags = 0; done by above MemSet */I think, for readability we can keep old code here or we can remove
new added comment also.
Setting p->pd_flags = 0; is unnecessary and redundant after memsetting
the page to zeros. Also, see the existing code for pd_prune_xid,
similarly I've done that for pd_flags. I think it's okay with /*
p->pd_flags = 0; done by above MemSet */.
Apart from this, all other changes looks good to me.
Thanks for taking a look at the patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Wed, Apr 07, 2021 at 06:31:19AM +0530, Bharath Rupireddy wrote:
Setting p->pd_flags = 0; is unnecessary and redundant after memsetting
the page to zeros. Also, see the existing code for pd_prune_xid,
similarly I've done that for pd_flags. I think it's okay with /*
p->pd_flags = 0; done by above MemSet */.
Sure, but this one does not hurt much either as-is, so I have left it
out, and applied the rest.
--
Michael
On Wed, Apr 7, 2021 at 11:44 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Apr 07, 2021 at 06:31:19AM +0530, Bharath Rupireddy wrote:
Setting p->pd_flags = 0; is unnecessary and redundant after memsetting
the page to zeros. Also, see the existing code for pd_prune_xid,
similarly I've done that for pd_flags. I think it's okay with /*
p->pd_flags = 0; done by above MemSet */.Sure, but this one does not hurt much either as-is, so I have left it
out, and applied the rest.
Thanks for pushing the patch.
I wanted to comment out p->pd_flags = 0; in PageInit similar to the
pd_prune_xid just for consistency.
/* p->pd_prune_xid = InvalidTransactionId; done by above MemSet */
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
ср, 7 апр. 2021 г. в 10:18, Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com>:
On Wed, Apr 7, 2021 at 11:44 AM Michael Paquier <michael@paquier.xyz>
wrote:On Wed, Apr 07, 2021 at 06:31:19AM +0530, Bharath Rupireddy wrote:
Setting p->pd_flags = 0; is unnecessary and redundant after memsetting
the page to zeros. Also, see the existing code for pd_prune_xid,
similarly I've done that for pd_flags. I think it's okay with /*
p->pd_flags = 0; done by above MemSet */.Sure, but this one does not hurt much either as-is, so I have left it
out, and applied the rest.Thanks for pushing the patch.
I wanted to comment out p->pd_flags = 0; in PageInit similar to the
pd_prune_xid just for consistency.
/* p->pd_prune_xid = InvalidTransactionId; done by above MemSet
*/With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
I've investigated the commit, and I think there is just one more thing that
can make the page init more even. I propose my very small patch on this in
another discussion branch:
/messages/by-id/CALT9ZEFFq2-n5Lmfg59L6Hm3ZrgCexyhR9eqme7v1jodtXGg1A@mail.gmail.com
If you want, feel free to discuss it and push if consider the change
relevant.
--
Best regards,
Pavel Borisov
Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>
On Wed, Apr 7, 2021 at 11:47 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Wed, Apr 7, 2021 at 11:44 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Apr 07, 2021 at 06:31:19AM +0530, Bharath Rupireddy wrote:
Setting p->pd_flags = 0; is unnecessary and redundant after memsetting
the page to zeros. Also, see the existing code for pd_prune_xid,
similarly I've done that for pd_flags. I think it's okay with /*
p->pd_flags = 0; done by above MemSet */.Sure, but this one does not hurt much either as-is, so I have left it
out, and applied the rest.Thanks for pushing the patch.
I wanted to comment out p->pd_flags = 0; in PageInit similar to the
pd_prune_xid just for consistency.
/* p->pd_prune_xid = InvalidTransactionId; done by above MemSet */
As I said above, just for consistency, I would like to see if the
attached one line patch can be taken, even though it doesn't have any
impact.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Thu, Apr 08, 2021 at 07:45:25AM +0530, Bharath Rupireddy wrote:
On Wed, Apr 7, 2021 at 11:47 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:I wanted to comment out p->pd_flags = 0; in PageInit similar to the
pd_prune_xid just for consistency.
/* p->pd_prune_xid = InvalidTransactionId; done by above MemSet */As I said above, just for consistency, I would like to see if the
attached one line patch can be taken, even though it doesn't have any
impact.
FWIW, I tend to prefer the existing style to keep around this code
rather than commenting it out, as one could think to remove it, but I
think that it can be important in terms of code comprehension when
reading the area. So I quite like what 96ef3b8 has undone for
pd_flags, but not much what cc59049 did back in 2007. That's a matter
of taste, really.
--
Michael
On Thu, Apr 8, 2021 at 1:22 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Apr 08, 2021 at 07:45:25AM +0530, Bharath Rupireddy wrote:
On Wed, Apr 7, 2021 at 11:47 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:I wanted to comment out p->pd_flags = 0; in PageInit similar to the
pd_prune_xid just for consistency.
/* p->pd_prune_xid = InvalidTransactionId; done by above MemSet */As I said above, just for consistency, I would like to see if the
attached one line patch can be taken, even though it doesn't have any
impact.FWIW, I tend to prefer the existing style to keep around this code
rather than commenting it out, as one could think to remove it, but I
think that it can be important in terms of code comprehension when
reading the area. So I quite like what 96ef3b8 has undone for
pd_flags, but not much what cc59049 did back in 2007. That's a matter
of taste, really.
Thanks! Since the main patch is committed I will go ahead and close
the CF entry.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com