Re: freeing bms explicitly

On Tue, Mar 22, 2022 at 9:04 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:

>
>
> On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
>> >
>> > On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> >>
>> >> Zhihong Yu <zyu(at)yugabyte(dot)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