Horton

Thread

Commits

  1. Remove unnecessary pstrdup in fetch_table_list.

  1. remove unneeded pstrdup in fetch_table_list

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2021-01-13T02:40:54Z

    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

  2. Re: remove unneeded pstrdup in fetch_table_list

    Amit Kapila <amit.kapila16@gmail.com> — 2021-01-13T06:10:33Z

    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.

  3. Re: remove unneeded pstrdup in fetch_table_list

    Daniel Gustafsson <daniel@yesql.se> — 2021-01-13T09:25:38Z

    > 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

  4. Re: remove unneeded pstrdup in fetch_table_list

    Amit Kapila <amit.kapila16@gmail.com> — 2021-01-13T13:09:23Z

    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.

  5. Re: remove unneeded pstrdup in fetch_table_list

    Daniel Gustafsson <daniel@yesql.se> — 2021-01-13T13:23:26Z

    > 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

  6. RE: remove unneeded pstrdup in fetch_table_list

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2021-01-14T01:17:57Z

    > >>> 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

  7. Re: remove unneeded pstrdup in fetch_table_list

    Michael Paquier <michael@paquier.xyz> — 2021-01-14T05:10:55Z

    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

  8. Re: remove unneeded pstrdup in fetch_table_list

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-14T05:21:02Z

    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

  9. Re: remove unneeded pstrdup in fetch_table_list

    Amit Kapila <amit.kapila16@gmail.com> — 2021-01-14T09:35:15Z

    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.

  10. Re: remove unneeded pstrdup in fetch_table_list

    Amit Kapila <amit.kapila16@gmail.com> — 2021-01-16T05:09:14Z

    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.