Thread: A spot of redundant initialization of brin memtuple

A spot of redundant initialization of brin memtuple

From
Richard Guo
Date:
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

Re: A spot of redundant initialization of brin memtuple

From
Bharath Rupireddy
Date:
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.



Re: A spot of redundant initialization of brin memtuple

From
Richard Guo
Date:

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

Re: A spot of redundant initialization of brin memtuple

From
Bharath Rupireddy
Date:
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.



Re: A spot of redundant initialization of brin memtuple

From
Richard Guo
Date:

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

Re: A spot of redundant initialization of brin memtuple

From
Tom Lane
Date:
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