public inbox for [email protected]  

help / color / mirror / Atom feed

From: Chao Li <[email protected]>
To: Álvaro Herrera <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Fujii Masao <[email protected]>
Subject: Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer
Date: Mon, 11 May 2026 15:09:21 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>



> On May 9, 2026, at 23:35, Álvaro Herrera <[email protected]> wrote:
> 
> Hello,
> 
> On 2026-May-09, Chao Li wrote:
> 
>> I found this issue while reviewing the patch [1] and was suggested use
>> a separate thread for the issue.
>> 
>> In tablesync.c, 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.
> 
> I find this coding pattern weird and ugly and confusing.  If what we
> need is three variables, shouldn't we have three variables instead of
> this strange misuse of the StringInfo abstraction?
> 

Yep, I first considered adding a file-local structure, but decided to keep the changes minimal in the first version.

In v2, I switched to using a small file-local CopyBuf structure.

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






Attachments:

  [application/octet-stream] v2-0001-Use-simple-struct-for-table-sync-COPY-buffer-stat.patch (2.8K, 2-v2-0001-Use-simple-struct-for-table-sync-COPY-buffer-stat.patch)
  download | inline diff:
From f506b5bc16802434db1999c3d06a4ec95a3c6f18 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Sat, 9 May 2026 14:00:02 +0800
Subject: [PATCH v2] Use simple struct for table sync COPY buffer state
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

copy_read_data() only needs to track the COPY buffer's data pointer,
length, and cursor position.  Replace the StringInfo with a small local
struct containing just those fields.

Author: Chao Li <[email protected]>
Reviewed-by: Álvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 src/backend/replication/logical/tablesync.c | 29 +++++++++++++--------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index eb718114297..e2fc37ae2c9 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -126,7 +126,14 @@
 
 List	   *table_states_not_ready = NIL;
 
-static StringInfo copybuf = NULL;
+typedef struct CopyBuf
+{
+	char	   *data;
+	int			len;
+	int			cursor;
+}			CopyBuf;
+
+static CopyBuf copybuf;
 
 /*
  * Wait until the relation sync state is set in the catalog to the expected
@@ -649,13 +656,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 +687,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 +1206,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,
-- 
2.50.1 (Apple Git-155)

view thread (3+ messages)

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: Avoid unnecessary StringInfo allocation in tablesync COPY buffer
  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