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
Attachments:
0001-remove-unneeded-pstrdup.patchapplication/octet-stream; name=0001-remove-unneeded-pstrdup.patchDownload
From 3c17c42b2ca636a393d8f2c73ec3c88d8543384f Mon Sep 17 00:00:00 2001
From: root <root@localhost.localdomain>
Date: Tue, 12 Jan 2021 21:25:44 -0500
Subject: [PATCH] remove unneeded pstrdup
---
src/backend/commands/subscriptioncmds.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 490e935..082f785 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1267,7 +1267,7 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
relname = TextDatumGetCString(slot_getattr(slot, 2, &isnull));
Assert(!isnull);
- rv = makeRangeVar(pstrdup(nspname), pstrdup(relname), -1);
+ rv = makeRangeVar(nspname, relname, -1);
tablelist = lappend(tablelist, rv);
ExecClearTuple(slot);
--
1.8.3.1
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
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.