On Tue, Apr 6, 2021 at 6:09 PM Michael Paquier <mich...@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
From fe18cdc930933415c02f6eda82fbb1646831ba0a Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Tue, 6 Apr 2021 19:06:51 +0530
Subject: [PATCH v3] Remove extra memset in
BloomInitPage,GinInitPage,SpGistInitPage
We are memset-ting the special space page that's already set to
zeros by PageInit in BloomInitPage, GinInitPage, SpGistInitPage
and initCachedPage. We have already removed the memset after
PageInit in gistinitpage (see the comment there). It looks like
they are redundant. This patch that gets rid of the extra memset
calls.
While on it, this patch also 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.
While on it, this patch also removed an unnecessary assignment of
pd_flags to 0 in PageInit as it is anyways being done by MemSet.
---
contrib/bloom/blinsert.c | 1 -
contrib/bloom/blutils.c | 1 -
src/backend/access/gin/ginutil.c | 1 -
src/backend/access/gist/gistutil.c | 2 --
src/backend/access/spgist/spgutils.c | 3 +--
src/backend/storage/page/bufpage.c | 2 +-
6 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index d37ceef753..c34a640d1c 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -63,7 +63,6 @@ flushCachedPage(Relation index, BloomBuildState *buildstate)
static void
initCachedPage(BloomBuildState *buildstate)
{
- memset(buildstate->data.data, 0, BLCKSZ);
BloomInitPage(buildstate->data.data, 0);
buildstate->count = 0;
}
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index 1e505b1da5..754de008d4 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -411,7 +411,6 @@ BloomInitPage(Page page, uint16 flags)
PageInit(page, BLCKSZ, sizeof(BloomPageOpaqueData));
opaque = BloomPageGetOpaque(page);
- memset(opaque, 0, sizeof(BloomPageOpaqueData));
opaque->flags = flags;
opaque->bloom_page_id = BLOOM_PAGE_ID;
}
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 6b9b04cf42..cdd626ff0a 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -348,7 +348,6 @@ GinInitPage(Page page, uint32 f, Size pageSize)
PageInit(page, pageSize, sizeof(GinPageOpaqueData));
opaque = GinPageGetOpaque(page);
- memset(opaque, 0, sizeof(GinPageOpaqueData));
opaque->flags = f;
opaque->rightlink = InvalidBlockNumber;
}
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 1ff1bf816f..8dcd53c457 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -761,8 +761,6 @@ gistinitpage(Page page, uint32 f)
PageInit(page, pageSize, sizeof(GISTPageOpaqueData));
opaque = GistPageGetOpaque(page);
- /* page was already zeroed by PageInit, so this is not needed: */
- /* memset(&(opaque->nsn), 0, sizeof(GistNSN)); */
opaque->rightlink = InvalidBlockNumber;
opaque->flags = f;
opaque->gist_page_id = GIST_PAGE_ID;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 72cbde7e0b..8d99c9b762 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -677,9 +677,8 @@ SpGistInitPage(Page page, uint16 f)
{
SpGistPageOpaque opaque;
- PageInit(page, BLCKSZ, MAXALIGN(sizeof(SpGistPageOpaqueData)));
+ PageInit(page, BLCKSZ, sizeof(SpGistPageOpaqueData));
opaque = SpGistPageGetOpaque(page);
- memset(opaque, 0, sizeof(SpGistPageOpaqueData));
opaque->flags = f;
opaque->spgist_page_id = SPGIST_PAGE_ID;
}
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 5d5989c2f5..9f77532a4f 100644
--- a/src/backend/storage/page/bufpage.c
+++ 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 */
p->pd_lower = SizeOfPageHeaderData;
p->pd_upper = pageSize - specialSize;
p->pd_special = pageSize - specialSize;
--
2.25.1
