public inbox for [email protected]  

help / color / mirror / Atom feed

From: Chao Li <[email protected]>
To: Fujii Masao <[email protected]>
Cc: cca5507 <[email protected]>
Cc: Shinya Kato <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Call EndCopyFrom() after initial table sync in logical replication
Date: Fri, 8 May 2026 13:09:38 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHGQGwHfdbfRRVdQjYRZmT94UMqE1W4MENiaseVtsHWmzkWxBQ@mail.gmail.com>
References: <CAOzEurQKuy3RiPkd=25PEwEzaqHuGvEOf=X7vaVzhgNjaukYzA@mail.gmail.com>
	<CAHGQGwEpwuq9U=Hhx3BbfGY-RrJ5rFvdQopO+0xyAuLVsMdfWA@mail.gmail.com>
	<[email protected]>
	<CAHGQGwHfdbfRRVdQjYRZmT94UMqE1W4MENiaseVtsHWmzkWxBQ@mail.gmail.com>



> On May 8, 2026, at 12:21, Fujii Masao <[email protected]> wrote:
> 
> On Fri, May 8, 2026 at 11:34 AM cca5507 <[email protected]> wrote:
>> 
>> Hi,
>> 
>> Maybe we want to add "free_parsestate(pstate);" after the "EndCopyFrom()" as well?
> 
> What actual issue could occur if free_parsestate() is not called there?
> 
> 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().
> 
> Regards,
> 
> -- 
> Fujii Masao

I don’t 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 = 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/






Attachments:

  [application/octet-stream] tablesync.c.diff (1.9K, 2-tablesync.c.diff)
  download | inline diff:
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,

view thread (11+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Call EndCopyFrom() after initial table sync in logical replication
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox