From: Chao Li <li.evan.chao@gmail.com>
Message-Id: <E6D725B9-1897-498A-BFDC-A86B84E03546@gmail.com>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_4F37E405-9B49-480C-9046-122F947DDC77"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Call EndCopyFrom() after initial table sync in logical
 replication
Date: Fri, 8 May 2026 13:09:38 +0800
In-Reply-To: 
 <CAHGQGwHfdbfRRVdQjYRZmT94UMqE1W4MENiaseVtsHWmzkWxBQ@mail.gmail.com>
Cc: cca5507 <cca5507@qq.com>,
 Shinya Kato <shinya11.kato@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
To: Fujii Masao <masao.fujii@gmail.com>
References: 
 <CAOzEurQKuy3RiPkd=25PEwEzaqHuGvEOf=X7vaVzhgNjaukYzA@mail.gmail.com>
 <CAHGQGwEpwuq9U=Hhx3BbfGY-RrJ5rFvdQopO+0xyAuLVsMdfWA@mail.gmail.com>
 <tencent_B56532E9785E072776FBA169850EB5032908@qq.com>
 <CAHGQGwHfdbfRRVdQjYRZmT94UMqE1W4MENiaseVtsHWmzkWxBQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/E6D725B9-1897-498A-BFDC-A86B84E03546%40gmail.com>
Precedence: bulk


--Apple-Mail=_4F37E405-9B49-480C-9046-122F947DDC77
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> On May 8, 2026, at 12:21, Fujii Masao <masao.fujii@gmail.com> wrote:
>=20
> On Fri, May 8, 2026 at 11:34=E2=80=AFAM cca5507 <cca5507@qq.com> =
wrote:
>>=20
>> Hi,
>>=20
>> Maybe we want to add "free_parsestate(pstate);" after the =
"EndCopyFrom()" as well?
>=20
> What actual issue could occur if free_parsestate() is not called =
there?
>=20
> Since pstate->p_target_relation does not seem to be used afterward,
> omitting free_parsestate() appears mostly harmless to me. Bascailly
> calling free_parsestate() after make_parsestate() seems intuitive,
> but from a quick grep I found several places that call =
make_parsestate()
> without a corresponding free_parsestate().
>=20
> Regards,
>=20
> --=20
> Fujii Masao

I don=E2=80=99t think this is a serious leak. In this path, pstate and =
attnamelist are allocated in CurTransactionContext, and the transaction =
is committed immediately after copy_table() finishes, so that memory is =
reclaimed at transaction end. Explicitly freeing them would be mostly =
for code readability, not to fix a memory leak. So, I am okay to not =
free them.

While tracing the code, I noticed another issue that is probably more =
worth addressing. copy_table() currently does:
```
    copybuf =3D makeStringInfo();
```

But copybuf is only used by copy_read_data(), and there it's really just =
acting as a small state holder for data, len, and cursor, rather than as =
a normal growable StringInfo. That means we do not need to allocate a =
StringInfo object or its backing buffer at all.

It would be cleaner to use a plain StringInfoData and simply =
reinitialize or zero it in copy_table(). See the attached diff for the =
proposed change.

David Rowley has made several cleanup changes in this area to prefer =
stack-allocated StringInfoData, for example =
a63bbc811d41b3567eb37fe2636e660a852dbbf2. This change seems consistent =
with that direction.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/


--Apple-Mail=_4F37E405-9B49-480C-9046-122F947DDC77
Content-Disposition: attachment;
	filename=tablesync.c.diff
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="tablesync.c.diff"
Content-Transfer-Encoding: 7bit

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index eb718114297..1dee2480e2f 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -126,7 +126,7 @@
 
 List	   *table_states_not_ready = NIL;
 
-static StringInfo copybuf = NULL;
+static StringInfoData copybuf = {0};
 
 /*
  * Wait until the relation sync state is set in the catalog to the expected
@@ -649,13 +649,13 @@ copy_read_data(void *outbuf, int minread, int maxread)
 	int			avail;
 
 	/* If there are some leftover data from previous read, use it. */
-	avail = copybuf->len - copybuf->cursor;
+	avail = copybuf.len - copybuf.cursor;
 	if (avail)
 	{
 		if (avail > maxread)
 			avail = maxread;
-		memcpy(outbuf, &copybuf->data[copybuf->cursor], avail);
-		copybuf->cursor += avail;
+		memcpy(outbuf, &copybuf.data[copybuf.cursor], avail);
+		copybuf.cursor += avail;
 		maxread -= avail;
 		bytesread += avail;
 	}
@@ -680,16 +680,16 @@ copy_read_data(void *outbuf, int minread, int maxread)
 			else
 			{
 				/* Process the data */
-				copybuf->data = buf;
-				copybuf->len = len;
-				copybuf->cursor = 0;
+				copybuf.data = buf;
+				copybuf.len = len;
+				copybuf.cursor = 0;
 
-				avail = copybuf->len - copybuf->cursor;
+				avail = copybuf.len - copybuf.cursor;
 				if (avail > maxread)
 					avail = maxread;
-				memcpy(outbuf, &copybuf->data[copybuf->cursor], avail);
+				memcpy(outbuf, &copybuf.data[copybuf.cursor], avail);
 				outbuf = (char *) outbuf + avail;
-				copybuf->cursor += avail;
+				copybuf.cursor += avail;
 				maxread -= avail;
 				bytesread += avail;
 			}
@@ -1199,7 +1199,7 @@ copy_table(Relation rel)
 						lrel.nspname, lrel.relname, res->err)));
 	walrcv_clear_result(res);
 
-	copybuf = makeStringInfo();
+	memset(&copybuf, 0, sizeof(copybuf));
 
 	pstate = make_parsestate(NULL);
 	(void) addRangeTableEntryForRelation(pstate, rel, AccessShareLock,

--Apple-Mail=_4F37E405-9B49-480C-9046-122F947DDC77--