Optimize duplicate code and fix memory leak in function fetch_remote_table_info()
This patch fixes two things in the function fetch_remote_table_info().
(1) *pfree(pub_names.data)* to avoid potential memory leaks.
(2) 2 pieces of code can do the same work,
``` C
foreach(lc, MySubscription->publications)
{
if (foreach_current_index(lc) > 0)
appendStringInfoString(&pub_names, ", ");
appendStringInfoString(&pub_names,
quote_literal_cstr(strVal(lfirst(lc))));
}
```
and
``` C
foreach_node(String, pubstr, MySubscription->publications)
{
char *pubname = strVal(pubstr);
if (foreach_current_index(pubstr) > 0)
appendStringInfoString(&pub_names, ", ");
appendStringInfoString(&pub_names, quote_literal_cstr(pubname));
}
```
I wanna integrate them into one function `make_pubname_list()` to make the
code neater.
Thanks for your time.
Regards
Yongtao Huang
Attachments:
0001-Optimize-duplicate-code-and-fix-memory-leak-in-table.patchapplication/octet-stream; name=0001-Optimize-duplicate-code-and-fix-memory-leak-in-table.patchDownload
From c650b3025c622bd98436ca5ca33ab8912f833bd0 Mon Sep 17 00:00:00 2001
From: Yongtao Huang <yongtah2022@gmail.com>
Date: Fri, 19 Jan 2024 22:35:00 +0800
Subject: [PATCH] Optimize duplicate code and fix memory leak in tablesync.c
---
src/backend/replication/logical/tablesync.c | 44 ++++++++++++++---------------
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 06d5b3d..29b97f9 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -777,6 +777,23 @@ copy_read_data(void *outbuf, int minread, int maxread)
return bytesread;
}
+/* Build the publications name list. */
+static StringInfoData
+make_pubname_list(List* publications)
+{
+ StringInfoData pub_names;
+ initStringInfo(&pub_names);
+
+ foreach_node(String, pubstr, publications)
+ {
+ if (foreach_current_index(pubstr) > 0)
+ appendStringInfoString(&pub_names, ", ");
+
+ appendStringInfoString(&pub_names, quote_literal_cstr(strVal(pubstr)));
+ }
+
+ return pub_names;
+}
/*
* Get information about remote relation in similar fashion the RELATION
@@ -795,7 +812,6 @@ fetch_remote_table_info(char *nspname, char *relname,
Oid qualRow[] = {TEXTOID};
bool isnull;
int natt;
- ListCell *lc;
Bitmapset *included_cols = NULL;
lrel->nspname = nspname;
@@ -849,15 +865,7 @@ fetch_remote_table_info(char *nspname, char *relname,
WalRcvExecResult *pubres;
TupleTableSlot *tslot;
Oid attrsRow[] = {INT2VECTOROID};
- StringInfoData pub_names;
-
- initStringInfo(&pub_names);
- foreach(lc, MySubscription->publications)
- {
- if (foreach_current_index(lc) > 0)
- appendStringInfoString(&pub_names, ", ");
- appendStringInfoString(&pub_names, quote_literal_cstr(strVal(lfirst(lc))));
- }
+ StringInfoData pub_names = make_pubname_list(MySubscription->publications);
/*
* Fetch info about column lists for the relation (from all the
@@ -1032,19 +1040,7 @@ fetch_remote_table_info(char *nspname, char *relname,
*/
if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
{
- StringInfoData pub_names;
-
- /* Build the pubname list. */
- initStringInfo(&pub_names);
- foreach_node(String, pubstr, MySubscription->publications)
- {
- char *pubname = strVal(pubstr);
-
- if (foreach_current_index(pubstr) > 0)
- appendStringInfoString(&pub_names, ", ");
-
- appendStringInfoString(&pub_names, quote_literal_cstr(pubname));
- }
+ StringInfoData pub_names = make_pubname_list(MySubscription->publications);
/* Check for row filters. */
resetStringInfo(&cmd);
@@ -1094,6 +1090,8 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(slot);
walrcv_clear_result(res);
+
+ pfree(pub_names.data);
}
pfree(cmd.data);
--
1.8.3.1
On Fri, Jan 19, 2024 at 10:42:46PM +0800, Yongtao Huang wrote:
This patch fixes two things in the function fetch_remote_table_info().
(1) *pfree(pub_names.data)* to avoid potential memory leaks.
True that this code puts some effort in cleaning up the memory used
locally.
(2) 2 pieces of code can do the same work,
```
I wanna integrate them into one function `make_pubname_list()` to make the
code neater.
It does not strike me as a huge problem to let the code be as it is on
HEAD when building the lists, FWIW, as we are talking about two places
and there is clarity in keeping the code as it is.
--
Michael
Thanks for your review.
(1) I think *pfree(pub_names.data)* is necessary.
(2) Agree with you. Considering that the new function is only called
twice, not encapsulating it into a function is not a huge problem.
Best wishes
Yongtao Huang
Michael Paquier <michael@paquier.xyz> 于2024年1月20日周六 11:13写道：
Show quoted text
On Fri, Jan 19, 2024 at 10:42:46PM +0800, Yongtao Huang wrote:
This patch fixes two things in the function fetch_remote_table_info().
(1) *pfree(pub_names.data)* to avoid potential memory leaks.
True that this code puts some effort in cleaning up the memory used
locally.(2) 2 pieces of code can do the same work,
```
I wanna integrate them into one function `make_pubname_list()` to makethe
code neater.
It does not strike me as a huge problem to let the code be as it is on
HEAD when building the lists, FWIW, as we are talking about two places
and there is clarity in keeping the code as it is.
--
Michael
Attachments:
0001-Fix-potential-memory-leak-in-tablesync.c.patchapplication/octet-stream; name=0001-Fix-potential-memory-leak-in-tablesync.c.patchDownload
From a30bf932b6b95b61b7e8788ada99a076f1f0de9d Mon Sep 17 00:00:00 2001
From: Yongtao Huang <yongtah2022@gmail.com>
Date: Sat, 20 Jan 2024 12:01:46 +0800
Subject: [PATCH] Fix potential memory leak in tablesync.c
---
src/backend/replication/logical/tablesync.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 06d5b3d..9b19695 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1094,6 +1094,8 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(slot);
walrcv_clear_result(res);
+
+ pfree(pub_names.data);
}
pfree(cmd.data);
--
1.8.3.1
Yongtao Huang <yongtaoh2022@gmail.com> writes:
(1) I think *pfree(pub_names.data)* is necessary.
Really?
It looks to me like copy_table, and thence fetch_remote_table_info,
is called once within a transaction. So whatever it leaks will be
released at transaction end. This is a good thing, because it's
messy enough that I seriously doubt that there aren't other leaks
in it, or that it'd be practical to expect that it can be made
to never leak anything.
If anything, I'd be inclined to remove the random pfree's that
are in it now. It's unlikely that they constitute a net win
compared to allowing memory context reset to clean things up.
regards, tom lane
Hi，
So whatever it leaks will be released at the transaction end.
I learned it. thank you very much for your explanation.
Regards,
Yongtao Huang
Tom Lane <tgl@sss.pgh.pa.us> 于2024年1月20日周六 12:34写道：
Show quoted text
Yongtao Huang <yongtaoh2022@gmail.com> writes:
(1) I think *pfree(pub_names.data)* is necessary.
Really?
It looks to me like copy_table, and thence fetch_remote_table_info,
is called once within a transaction. So whatever it leaks will be
released at transaction end. This is a good thing, because it's
messy enough that I seriously doubt that there aren't other leaks
in it, or that it'd be practical to expect that it can be made
to never leak anything.If anything, I'd be inclined to remove the random pfree's that
are in it now. It's unlikely that they constitute a net win
compared to allowing memory context reset to clean things up.regards, tom lane