Обсуждение: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Bharath Rupireddy
Дата:
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
Вложения
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Mahendra Singh Thalor
Дата:
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
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Bharath Rupireddy
Дата:
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
Вложения
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Bharath Rupireddy
Дата:
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
Вложения
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Mahendra Singh Thalor
Дата:
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
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Bharath Rupireddy
Дата:
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
Вложения
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Bharath Rupireddy
Дата:
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
https://www.postgresql.org/message-id/CALT9ZEFFq2-n5Lmfg59L6Hm3ZrgCexyhR9eqme7v1jodtXGg1A@mail.gmail.com
--
If you want, feel free to discuss it and push if consider the change relevant.
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Bharath Rupireddy
Дата:
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
Вложения
Re: Can we remove extra memset in BloomInitPage, GinInitPage and SpGistInitPage when we have it in PageInit?
От
Bharath Rupireddy
Дата:
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