On Tue, Mar 22, 2022 at 9:04 PM Zhihong Yu <z...@yugabyte.com> wrote:
> > > On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: > >> On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <z...@yugabyte.com> wrote: >> > >> > On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <t...@sss.pgh.pa.us> wrote: >> >> >> >> Zhihong Yu <z...@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
