A spot of redundant initialization of brin memtuple

Started by Richard Guoabout 4 years ago6 messages

guofenglinux@gmail.com

about 4 years ago

Happened to notice this when reading around the codes. The BrinMemTuple
would be initialized in brin_new_memtuple(), right after being created.
So we don't need to initialize it again outside.

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959..67a277e1f9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1261,8 +1261,6 @@ initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap,
        state->bs_bdesc = brin_build_desc(idxRel);
        state->bs_dtuple = brin_new_memtuple(state->bs_bdesc);

- brin_memtuple_initialize(state->bs_dtuple, state->bs_bdesc);
-
return state;
}

Thanks
Richard

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

about 4 years ago

In reply to: Richard Guo (#1)

Re: A spot of redundant initialization of brin memtuple

On Fri, Nov 19, 2021 at 1:13 PM Richard Guo <guofenglinux@gmail.com> wrote:

Happened to notice this when reading around the codes. The BrinMemTuple
would be initialized in brin_new_memtuple(), right after being created.
So we don't need to initialize it again outside.
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959..67a277e1f9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1261,8 +1261,6 @@ initialize_brin_buildstate(Relation idxRel, BrinRevmap *revmap,
state->bs_bdesc = brin_build_desc(idxRel);
state->bs_dtuple = brin_new_memtuple(state->bs_bdesc);
- brin_memtuple_initialize(state->bs_dtuple, state->bs_bdesc);
-
return state;
}

Good catch. +1 for the change. Please submit a patch.

Regards,
Bharath Rupireddy.

Richard Guo

guofenglinux@gmail.com

about 4 years ago

In reply to: Bharath Rupireddy (#2)

1 attachment(s)

Re: A spot of redundant initialization of brin memtuple

On Sat, Nov 20, 2021 at 12:23 AM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Nov 19, 2021 at 1:13 PM Richard Guo <guofenglinux@gmail.com>
wrote:
Happened to notice this when reading around the codes. The BrinMemTuple
would be initialized in brin_new_memtuple(), right after being created.
So we don't need to initialize it again outside.
diff --git a/src/backend/access/brin/brin.c
b/src/backend/access/brin/brin.c
index ccc9fa0959..67a277e1f9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1261,8 +1261,6 @@ initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap,

state->bs_bdesc = brin_build_desc(idxRel);
state->bs_dtuple = brin_new_memtuple(state->bs_bdesc);

- brin_memtuple_initialize(state->bs_dtuple, state->bs_bdesc);
-
return state;
}

Good catch. +1 for the change. Please submit a patch.

Thanks for the review. Attached is the patch.

Thanks
Richard

Attachments:

v1-0001-Remove-redundant-initialization-of-brin-memtuple.patchapplication/octet-stream; name=v1-0001-Remove-redundant-initialization-of-brin-memtuple.patchDownload

From b792a9371335d225f4f021cf18a734097939e1a2 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 22 Nov 2021 11:06:58 +0800
Subject: [PATCH v1] Remove redundant initialization of brin memtuple

---
 src/backend/access/brin/brin.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959..67a277e1f9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1261,8 +1261,6 @@ initialize_brin_buildstate(Relation idxRel, BrinRevmap *revmap,
 	state->bs_bdesc = brin_build_desc(idxRel);
 	state->bs_dtuple = brin_new_memtuple(state->bs_bdesc);
 
-	brin_memtuple_initialize(state->bs_dtuple, state->bs_bdesc);
-
 	return state;
 }
 
-- 
2.31.0

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

about 4 years ago

In reply to: Richard Guo (#3)

Re: A spot of redundant initialization of brin memtuple

On Mon, Nov 22, 2021 at 8:53 AM Richard Guo <guofenglinux@gmail.com> wrote:

On Sat, Nov 20, 2021 at 12:23 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
On Fri, Nov 19, 2021 at 1:13 PM Richard Guo <guofenglinux@gmail.com> wrote:
Happened to notice this when reading around the codes. The BrinMemTuple
would be initialized in brin_new_memtuple(), right after being created.
So we don't need to initialize it again outside.
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959..67a277e1f9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1261,8 +1261,6 @@ initialize_brin_buildstate(Relation idxRel, BrinRevmap *revmap,
state->bs_bdesc = brin_build_desc(idxRel);
state->bs_dtuple = brin_new_memtuple(state->bs_bdesc);
- brin_memtuple_initialize(state->bs_dtuple, state->bs_bdesc);
-
return state;
}
Good catch. +1 for the change. Please submit a patch.
Thanks for the review. Attached is the patch.

Thanks. The patch looks good to me. Let's add it to the commitfest to
not lose track of it.

Regards,
Bharath Rupireddy.

Richard Guo

guofenglinux@gmail.com

about 4 years ago

In reply to: Bharath Rupireddy (#4)

Re: A spot of redundant initialization of brin memtuple

On Mon, Nov 22, 2021 at 12:52 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Nov 22, 2021 at 8:53 AM Richard Guo <guofenglinux@gmail.com>
wrote:

On Sat, Nov 20, 2021 at 12:23 AM Bharath Rupireddy <

bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Nov 19, 2021 at 1:13 PM Richard Guo <guofenglinux@gmail.com>

wrote:

Happened to notice this when reading around the codes. The

BrinMemTuple

would be initialized in brin_new_memtuple(), right after being

created.
So we don't need to initialize it again outside.
diff --git a/src/backend/access/brin/brin.c
b/src/backend/access/brin/brin.c
index ccc9fa0959..67a277e1f9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1261,8 +1261,6 @@ initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap,

state->bs_bdesc = brin_build_desc(idxRel);
state->bs_dtuple = brin_new_memtuple(state->bs_bdesc);

- brin_memtuple_initialize(state->bs_dtuple, state->bs_bdesc);
-
return state;
}

Good catch. +1 for the change. Please submit a patch.

Thanks for the review. Attached is the patch.

Thanks. The patch looks good to me. Let's add it to the commitfest to
not lose track of it.

Done. Here it is:
https://commitfest.postgresql.org/36/3424/

Thanks again for the review.

Thanks
Richard

Tom Lane

tgl@sss.pgh.pa.us

about 4 years ago

In reply to: Richard Guo (#5)

Re: A spot of redundant initialization of brin memtuple

Richard Guo <guofenglinux@gmail.com> writes:

On Mon, Nov 22, 2021 at 12:52 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

Thanks. The patch looks good to me. Let's add it to the commitfest to
not lose track of it.

Done. Here it is:
https://commitfest.postgresql.org/36/3424/

Pushed, thanks for the patch.

regards, tom lane