Обсуждение: A spot of redundant initialization of brin memtuple

Поиск
Список
Период
Сортировка

A spot of redundant initialization of brin memtuple

От
Richard Guo
Дата:
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

От
Bharath Rupireddy
Дата:
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

От
Richard Guo
Дата:

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 
Вложения

Re: A spot of redundant initialization of brin memtuple

От
Bharath Rupireddy
Дата:
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

От
Richard Guo
Дата:

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

От
Tom Lane
Дата:
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