Thread: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?

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

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

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

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