Thread: A spot of redundant initialization of brin memtuple
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
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
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.
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
Attachment
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.
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:
Thanks again for the review.
Thanks
Richard
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