Horton

Thread

Commits

  1. Remove some useless free calls.

  1. Re: freeing bms explicitly

    Zhihong Yu <zyu@yugabyte.com> — 2022-03-21T21:30:52Z

    >
> Hi,
> I was looking at calls to bms_free() in PG code.
>
> e.g. src/backend/commands/publicationcmds.c line 362
>
>         bms_free(bms);
>
> The above is just an example, there're other calls to bms_free().
> Since the bms is allocated from some execution context, I wonder why this
> call is needed.
>
> When the underlying execution context wraps up, isn't the bms freed ?
>
> Cheers
>
>
>

  2. Re: freeing bms explicitly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-21T22:05:19Z

    Zhihong Yu <zyu@yugabyte.com> writes:
>> I was looking at calls to bms_free() in PG code.
>> e.g. src/backend/commands/publicationcmds.c line 362
>> 	bms_free(bms);
>> The above is just an example, there're other calls to bms_free().
>> Since the bms is allocated from some execution context, I wonder why this
>> call is needed.
>> 
>> When the underlying execution context wraps up, isn't the bms freed ?

Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even
more pointless, since it'll free only the top node of that expression
tree.  Not to mention the string returned by TextDatumGetCString, and
whatever might be leaked during the underlying catalog accesses.

If we were actually worried about transient space consumption of this
function, it'd be necessary to do a lot more than this.  It doesn't
look to me like it's worth worrying about though -- it doesn't seem
like it could be hit more than once per query in normal cases.

			regards, tom lane

  3. Re: freeing bms explicitly

    Zhihong Yu <zyu@yugabyte.com> — 2022-03-21T22:13:18Z

    On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Zhihong Yu <zyu@yugabyte.com> writes:
> >> I was looking at calls to bms_free() in PG code.
> >> e.g. src/backend/commands/publicationcmds.c line 362
> >>      bms_free(bms);
> >> The above is just an example, there're other calls to bms_free().
> >> Since the bms is allocated from some execution context, I wonder why
> this
> >> call is needed.
> >>
> >> When the underlying execution context wraps up, isn't the bms freed ?
>
> Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even
> more pointless, since it'll free only the top node of that expression
> tree.  Not to mention the string returned by TextDatumGetCString, and
> whatever might be leaked during the underlying catalog accesses.
>
> If we were actually worried about transient space consumption of this
> function, it'd be necessary to do a lot more than this.  It doesn't
> look to me like it's worth worrying about though -- it doesn't seem
> like it could be hit more than once per query in normal cases.
>
>                         regards, tom lane
>

Thanks Tom for replying.

What do you think of the following patch ?

Cheers

  4. Re: freeing bms explicitly

    Amit Kapila <amit.kapila16@gmail.com> — 2022-03-23T03:45:27Z

    On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>
> On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Zhihong Yu <zyu@yugabyte.com> writes:
>> >> I was looking at calls to bms_free() in PG code.
>> >> e.g. src/backend/commands/publicationcmds.c line 362
>> >>      bms_free(bms);
>> >> The above is just an example, there're other calls to bms_free().
>> >> Since the bms is allocated from some execution context, I wonder why this
>> >> call is needed.
>> >>
>> >> When the underlying execution context wraps up, isn't the bms freed ?
>>
>> Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even
>> more pointless, since it'll free only the top node of that expression
>> tree.  Not to mention the string returned by TextDatumGetCString, and
>> whatever might be leaked during the underlying catalog accesses.
>>
>> If we were actually worried about transient space consumption of this
>> function, it'd be necessary to do a lot more than this.  It doesn't
>> look to me like it's worth worrying about though -- it doesn't seem
>> like it could be hit more than once per query in normal cases.
>>
>>                         regards, tom lane
>
>
> Thanks Tom for replying.
>
> What do you think of the following patch ?
>

Your patch looks good to me. I have found one more similar instance in
the same file and changed that as well accordingly. Let me know what
you think of the attached?

-- 
With Regards,
Amit Kapila.

  5. Re: freeing bms explicitly

    Zhihong Yu <zyu@yugabyte.com> — 2022-03-23T04:04:03Z

    On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> >
> > On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>
> >> Zhihong Yu <zyu@yugabyte.com> writes:
> >> >> I was looking at calls to bms_free() in PG code.
> >> >> e.g. src/backend/commands/publicationcmds.c line 362
> >> >>      bms_free(bms);
> >> >> The above is just an example, there're other calls to bms_free().
> >> >> Since the bms is allocated from some execution context, I wonder why
> this
> >> >> call is needed.
> >> >>
> >> >> When the underlying execution context wraps up, isn't the bms freed ?
> >>
> >> Yeah, that's kind of pointless --- and the pfree(rfnode) after it is
> even
> >> more pointless, since it'll free only the top node of that expression
> >> tree.  Not to mention the string returned by TextDatumGetCString, and
> >> whatever might be leaked during the underlying catalog accesses.
> >>
> >> If we were actually worried about transient space consumption of this
> >> function, it'd be necessary to do a lot more than this.  It doesn't
> >> look to me like it's worth worrying about though -- it doesn't seem
> >> like it could be hit more than once per query in normal cases.
> >>
> >>                         regards, tom lane
> >
> >
> > Thanks Tom for replying.
> >
> > What do you think of the following patch ?
> >
>
> Your patch looks good to me. I have found one more similar instance in
> the same file and changed that as well accordingly. Let me know what
> you think of the attached?
>
> --
> With Regards,
> Amit Kapila.
>

Hi, Amit:
The patch looks good to me.

Cheers

  6. Re: freeing bms explicitly

    Zhihong Yu <zyu@yugabyte.com> — 2022-03-23T16:42:21Z

    On Tue, Mar 22, 2022 at 9:04 PM Zhihong Yu <zyu@yugabyte.com> wrote:

>
>
> On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>
>> On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>> >
>> > On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >>
>> >> Zhihong Yu <zyu@yugabyte.com> writes:
>> >> >> I was looking at calls to bms_free() in PG code.
>> >> >> e.g. src/backend/commands/publicationcmds.c line 362
>> >> >>      bms_free(bms);
>> >> >> The above is just an example, there're other calls to bms_free().
>> >> >> Since the bms is allocated from some execution context, I wonder
>> why this
>> >> >> call is needed.
>> >> >>
>> >> >> When the underlying execution context wraps up, isn't the bms freed
>> ?
>> >>
>> >> Yeah, that's kind of pointless --- and the pfree(rfnode) after it is
>> even
>> >> more pointless, since it'll free only the top node of that expression
>> >> tree.  Not to mention the string returned by TextDatumGetCString, and
>> >> whatever might be leaked during the underlying catalog accesses.
>> >>
>> >> If we were actually worried about transient space consumption of this
>> >> function, it'd be necessary to do a lot more than this.  It doesn't
>> >> look to me like it's worth worrying about though -- it doesn't seem
>> >> like it could be hit more than once per query in normal cases.
>> >>
>> >>                         regards, tom lane
>> >
>> >
>> > Thanks Tom for replying.
>> >
>> > What do you think of the following patch ?
>> >
>>
>> Your patch looks good to me. I have found one more similar instance in
>> the same file and changed that as well accordingly. Let me know what
>> you think of the attached?
>>
>> --
>> With Regards,
>> Amit Kapila.
>>
>
> Hi, Amit:
> The patch looks good to me.
>
> Cheers
>

Tom:
 Do you mind taking a look at the latest patch ?

Thanks

  7. Re: freeing bms explicitly

    Amit Kapila <amit.kapila16@gmail.com> — 2022-03-24T02:13:37Z

    On Wed, Mar 23, 2022 at 9:30 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>
> On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>>
>> Your patch looks good to me. I have found one more similar instance in
>> the same file and changed that as well accordingly. Let me know what
>> you think of the attached?
>>
>
> Hi, Amit:
> The patch looks good to me.
>

Thanks. I'll push this tomorrow unless Tom or someone else wants to
look at it or would like to commit.

-- 
With Regards,
Amit Kapila.

  8. Re: freeing bms explicitly

    Amit Kapila <amit.kapila16@gmail.com> — 2022-03-25T06:35:55Z

    On Thu, Mar 24, 2022 at 7:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 23, 2022 at 9:30 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> >
> > On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >> On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> >>
> >> Your patch looks good to me. I have found one more similar instance in
> >> the same file and changed that as well accordingly. Let me know what
> >> you think of the attached?
> >>
> >
> > Hi, Amit:
> > The patch looks good to me.
> >
>
> Thanks. I'll push this tomorrow unless Tom or someone else wants to
> look at it or would like to commit.
>

Pushed.

-- 
With Regards,
Amit Kapila.