public inbox for [email protected]
help / color / mirror / Atom feedAvoid unnecessary StringInfo allocation in tablesync COPY buffer
3+ messages / 2 participants
[nested] [flat]
* Avoid unnecessary StringInfo allocation in tablesync COPY buffer
@ 2026-05-09 06:10 Chao Li <[email protected]>
2026-05-09 15:35 ` Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Chao Li @ 2026-05-09 06:10 UTC (permalink / raw)
To: pgsql-hackers; +Cc: David Rowley <[email protected]>; Fujii Masao <[email protected]>
Hi,
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. 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 patch 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.
[1] https://postgr.es/m/CAOzEurQKuy3RiPkd=25PEwEzaqHuGvEOf=X7vaVzhgNjaukYzA@mail.gmail.com
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-Avoid-unnecessary-StringInfo-allocation-in-tables.patch (2.6K, 2-v1-0001-Avoid-unnecessary-StringInfo-allocation-in-tables.patch)
download | inline diff:
From cffb27e65afaf3b7b0202f4e14518a424cf52ae3 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Sat, 9 May 2026 14:00:02 +0800
Subject: [PATCH v1] Avoid unnecessary StringInfo allocation in tablesync COPY
buffer
copybuf is only used to track the data pointer, length, and cursor for
leftover COPY data. The buffer allocated by makeStringInfo() is not used,
because copybuf->data is later replaced by the buffer returned from
walrcv_receive().
Use a static StringInfoData instead and reset it before starting COPY.
Author: Chao Li <[email protected]>
---
src/backend/replication/logical/tablesync.c | 22 ++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
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, ©buf->data[copybuf->cursor], avail);
- copybuf->cursor += avail;
+ memcpy(outbuf, ©buf.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, ©buf->data[copybuf->cursor], avail);
+ memcpy(outbuf, ©buf.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(©buf, 0, sizeof(copybuf));
pstate = make_parsestate(NULL);
(void) addRangeTableEntryForRelation(pstate, rel, AccessShareLock,
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer
2026-05-09 06:10 Avoid unnecessary StringInfo allocation in tablesync COPY buffer Chao Li <[email protected]>
@ 2026-05-09 15:35 ` Álvaro Herrera <[email protected]>
2026-05-11 07:09 ` Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer Chao Li <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Álvaro Herrera @ 2026-05-09 15:35 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: pgsql-hackers; David Rowley <[email protected]>; Fujii Masao <[email protected]>
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?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"We have labored long to build a heaven, only (Prof. Milton Glass)
to find it populated with horrors" (Watchmen, Alan Moore)
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer
2026-05-09 06:10 Avoid unnecessary StringInfo allocation in tablesync COPY buffer Chao Li <[email protected]>
2026-05-09 15:35 ` Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer Álvaro Herrera <[email protected]>
@ 2026-05-11 07:09 ` Chao Li <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Chao Li @ 2026-05-11 07:09 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: pgsql-hackers; David Rowley <[email protected]>; Fujii Masao <[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, ©buf->data[copybuf->cursor], avail);
- copybuf->cursor += avail;
+ memcpy(outbuf, ©buf.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, ©buf->data[copybuf->cursor], avail);
+ memcpy(outbuf, ©buf.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(©buf, 0, sizeof(copybuf));
pstate = make_parsestate(NULL);
(void) addRangeTableEntryForRelation(pstate, rel, AccessShareLock,
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-05-11 07:09 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-09 06:10 Avoid unnecessary StringInfo allocation in tablesync COPY buffer Chao Li <[email protected]>
2026-05-09 15:35 ` Álvaro Herrera <[email protected]>
2026-05-11 07:09 ` Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox