Thread: remove unneeded pstrdup in fetch_table_list
Hi
In function fetch_table_list, it get the table names from publicer and return a list of tablenames.
When append the name to the list, it use the following code:
**
nspname = TextDatumGetCString(slot_getattr(slot, 1, &isnull));
Assert(!isnull);
relname = TextDatumGetCString(slot_getattr(slot, 2, &isnull));
rv = makeRangeVar(pstrdup(nspname), pstrdup(relname), -1);
tablelist = lappend(tablelist, rv);
**
the nspname and relname will be copied which seems unnecessary.
Because nspame and relname is get from TextDatumGetCString.
IMO, TextDatumGetCString returns a newly palloced string.
**
result = (char *) palloc(len + 1);
memcpy(result, VARDATA_ANY(tunpacked), len);
result[len] = '\0';
if (tunpacked != t)
pfree(tunpacked);
return result;
**
It may harm when there are a lot of tables are replicated.
So I try to fix this.
Best regards,
houzj
Attachment
On Wed, Jan 13, 2021 at 8:11 AM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote: > > Hi > > In function fetch_table_list, it get the table names from publicer and return a list of tablenames. > When append the name to the list, it use the following code: > > ** > nspname = TextDatumGetCString(slot_getattr(slot, 1, &isnull)); > Assert(!isnull); > relname = TextDatumGetCString(slot_getattr(slot, 2, &isnull)); > rv = makeRangeVar(pstrdup(nspname), pstrdup(relname), -1); > tablelist = lappend(tablelist, rv); > ** > > the nspname and relname will be copied which seems unnecessary. > Because nspame and relname is get from TextDatumGetCString. > IMO, TextDatumGetCString returns a newly palloced string. > > ** > result = (char *) palloc(len + 1); > memcpy(result, VARDATA_ANY(tunpacked), len); > result[len] = '\0'; > > if (tunpacked != t) > pfree(tunpacked); > > return result; > ** > Your observation seems correct to me, though I have not tried to test your patch. -- With Regards, Amit Kapila.
> On 13 Jan 2021, at 07:10, Amit Kapila <amit.kapila16@gmail.com> wrote: > Your observation seems correct to me, though I have not tried to test > your patch. +1 to apply, this looks correct and passes tests. Scanning around I didn't see any other occurrences of the same pattern. cheers ./daniel
On Wed, Jan 13, 2021 at 2:55 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > > On 13 Jan 2021, at 07:10, Amit Kapila <amit.kapila16@gmail.com> wrote: > > > Your observation seems correct to me, though I have not tried to test > > your patch. > > +1 to apply, this looks correct and passes tests. Scanning around I didn't see > any other occurrences of the same pattern. > Thanks. I am thinking to backpatch this even though there is no problem reported from any production system. What do you think? -- With Regards, Amit Kapila.
> On 13 Jan 2021, at 14:09, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Jan 13, 2021 at 2:55 PM Daniel Gustafsson <daniel@yesql.se> wrote: >> >>> On 13 Jan 2021, at 07:10, Amit Kapila <amit.kapila16@gmail.com> wrote: >> >>> Your observation seems correct to me, though I have not tried to test >>> your patch. >> >> +1 to apply, this looks correct and passes tests. Scanning around I didn't see >> any other occurrences of the same pattern. > > Thanks. I am thinking to backpatch this even though there is no > problem reported from any production system. What do you think? No objections from me. cheers ./daniel
> >>> Your observation seems correct to me, though I have not tried to > >>> test your patch. > >> > >> +1 to apply, this looks correct and passes tests. Scanning around I > >> +didn't see > >> any other occurrences of the same pattern. > > > > Thanks. I am thinking to backpatch this even though there is no > > problem reported from any production system. What do you think? > > No objections from me. +1 Best regards, houzj
On Thu, Jan 14, 2021 at 01:17:57AM +0000, Hou, Zhijie wrote: >>> Thanks. I am thinking to backpatch this even though there is no >>> problem reported from any production system. What do you think? >> >> No objections from me. > > +1 text_to_cstring() indeed allocates a new string, so the extra allocation is useless. FWIW, I don't see much point in poking at the stable branches here. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jan 14, 2021 at 01:17:57AM +0000, Hou, Zhijie wrote:
>>>> Thanks. I am thinking to backpatch this even though there is no
>>>> problem reported from any production system. What do you think?
> text_to_cstring() indeed allocates a new string, so the extra
> allocation is useless. FWIW, I don't see much point in poking at
> the stable branches here.
Yeah, unless there's some reason to think that this creates a
meaningful memory leak, I wouldn't bother back-patching.
regards, tom lane
On Thu, Jan 14, 2021 at 10:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Michael Paquier <michael@paquier.xyz> writes: > > On Thu, Jan 14, 2021 at 01:17:57AM +0000, Hou, Zhijie wrote: > >>>> Thanks. I am thinking to backpatch this even though there is no > >>>> problem reported from any production system. What do you think? > > > text_to_cstring() indeed allocates a new string, so the extra > > allocation is useless. FWIW, I don't see much point in poking at > > the stable branches here. > > Yeah, unless there's some reason to think that this creates a > meaningful memory leak, I wouldn't bother back-patching. > The only case where it might impact as per the report of Zhijie Hou is where the user is subscribed to the publication that has a lot of tables like Create Publication ... For All Tables. Even though for such a case the memory consumed could be high but all the memory is allocated in the Portal context and will be released at the statement end. I was not sure if that could create a meaningful leak to any user so to be on the safer side proposed to backpatch it. However, if others don't think we need to backpatch this then I am fine doing it just for HEAD. -- With Regards, Amit Kapila.
On Thu, Jan 14, 2021 at 3:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Jan 14, 2021 at 10:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Michael Paquier <michael@paquier.xyz> writes: > > > On Thu, Jan 14, 2021 at 01:17:57AM +0000, Hou, Zhijie wrote: > > >>>> Thanks. I am thinking to backpatch this even though there is no > > >>>> problem reported from any production system. What do you think? > > > > > text_to_cstring() indeed allocates a new string, so the extra > > > allocation is useless. FWIW, I don't see much point in poking at > > > the stable branches here. > > > > Yeah, unless there's some reason to think that this creates a > > meaningful memory leak, I wouldn't bother back-patching. > > > > The only case where it might impact as per the report of Zhijie Hou is > where the user is subscribed to the publication that has a lot of > tables like Create Publication ... For All Tables. Even though for > such a case the memory consumed could be high but all the memory is > allocated in the Portal context and will be released at the statement > end. I was not sure if that could create a meaningful leak to any user > so to be on the safer side proposed to backpatch it. However, if > others don't think we need to backpatch this then I am fine doing it > just for HEAD. > Hearing no further suggestions, pushed just to HEAD. -- With Regards, Amit Kapila.