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

Started by Bharath Rupireddyalmost 5 years ago14 messages

bharath.rupireddyforpostgres@gmail.com

almost 5 years ago

1 attachment(s)

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

From 6facda585f8daca545677e56f7bc5d06dc23cf39 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 22 Mar 2021 10:13:50 +0530
Subject: [PATCH v1] Remove extra memset in BloomInitPage, GinInitPage and
 SpGistInitPage
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 contrib/bloom/blutils.c              | 1 -
 src/backend/access/gin/ginutil.c     | 1 -
 src/backend/access/spgist/spgutils.c | 3 +--
 3 files changed, 1 insertion(+), 4 deletions(-)

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/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index d8b1815061..1b333f6db1 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -541,9 +541,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;
 }
-- 
2.25.1