Re: remove unneeded pstrdup in fetch_table_list
On Thu, Jan 14, 2021 at 3:05 PM Amit Kapila wrote: > > On Thu, Jan 14, 2021 at 10:51 AM Tom Lane wrote: > > > > Michael Paquier writes: > > > On Thu, Jan 14, 2021 at 01:17:57AM +, 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.
Re: remove unneeded pstrdup in fetch_table_list
On Thu, Jan 14, 2021 at 10:51 AM Tom Lane wrote: > > Michael Paquier writes: > > On Thu, Jan 14, 2021 at 01:17:57AM +, 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.
Re: remove unneeded pstrdup in fetch_table_list
Michael Paquier writes: > On Thu, Jan 14, 2021 at 01:17:57AM +, 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
Re: remove unneeded pstrdup in fetch_table_list
On Thu, Jan 14, 2021 at 01:17:57AM +, 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 signature.asc Description: PGP signature
RE: remove unneeded pstrdup in fetch_table_list
> >>> 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
Re: remove unneeded pstrdup in fetch_table_list
> On 13 Jan 2021, at 14:09, Amit Kapila wrote: > > On Wed, Jan 13, 2021 at 2:55 PM Daniel Gustafsson wrote: >> >>> On 13 Jan 2021, at 07:10, Amit Kapila 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
Re: remove unneeded pstrdup in fetch_table_list
On Wed, Jan 13, 2021 at 2:55 PM Daniel Gustafsson wrote: > > > On 13 Jan 2021, at 07:10, Amit Kapila 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.
Re: remove unneeded pstrdup in fetch_table_list
> On 13 Jan 2021, at 07:10, Amit Kapila 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
Re: remove unneeded pstrdup in fetch_table_list
On Wed, Jan 13, 2021 at 8:11 AM Hou, Zhijie 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, )); > Assert(!isnull); > relname = TextDatumGetCString(slot_getattr(slot, 2, )); > 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.
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, )); Assert(!isnull); relname = TextDatumGetCString(slot_getattr(slot, 2, )); 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 0001-remove-unneeded-pstrdup.patch Description: 0001-remove-unneeded-pstrdup.patch
