public inbox for [email protected]
help / color / mirror / Atom feedCall EndCopyFrom() after initial table sync in logical replication
11+ messages / 4 participants
[nested] [flat]
* Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-04 07:58 Shinya Kato <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Shinya Kato @ 2026-05-04 07:58 UTC (permalink / raw)
To: pgsql-hackers
Hi hackers,
While reading the logical replication initial table sync code, I
noticed that copy_table() calls BeginCopyFrom() and CopyFrom() but
never calls the matching EndCopyFrom().
EndCopyFrom() calls pgstat_progress_end_command(), which resets
st_progress_command to PROGRESS_COMMAND_INVALID. Without that call,
the backend status entry continues to report an active COPY operation
while the tablesync worker proceeds to WAL catchup. As a result,
pg_stat_progress_copy shows a stale entry for the entire WAL catchup
phase.
Attached patch adds EndCopyFrom(cstate) immediately after
CopyFrom(cstate) returns.
--
Best regards,
Shinya Kato
NTT OSS Center
Attachments:
[application/octet-stream] v1-0001-Call-EndCopyFrom-during-initial-table-sync-in-log.patch (1.4K, 2-v1-0001-Call-EndCopyFrom-during-initial-table-sync-in-log.patch)
download | inline diff:
From 14f523601a44f816833dd5e4e0f2326edf533278 Mon Sep 17 00:00:00 2001
From: Shinya Kato <[email protected]>
Date: Tue, 21 Apr 2026 10:29:07 +0900
Subject: [PATCH v1] Call EndCopyFrom() during initial table sync in logical
replication
Previously, copy_table() called BeginCopyFrom() and CopyFrom() but
omitted the matching EndCopyFrom() call. Without it, the backend
status entry continued to report an active COPY operation while the
tablesync worker proceeded to WAL catchup, causing pg_stat_progress_copy
to show a stale entry for the entire WAL catchup phase, which can be
significant for large tables.
This commit adds the missing EndCopyFrom() call, which invokes
pgstat_progress_end_command() to reset st_progress_command to
PROGRESS_COMMAND_INVALID as soon as the COPY phase completes.
Author: Shinya Kato <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/replication/logical/tablesync.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index eb718114297..a04b84ebc1d 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1210,6 +1210,7 @@ copy_table(Relation rel)
/* Do the copy */
(void) CopyFrom(cstate);
+ EndCopyFrom(cstate);
logicalrep_rel_close(relmapentry, NoLock);
}
--
2.47.3
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-07 15:04 Fujii Masao <[email protected]>
parent: Shinya Kato <[email protected]>
0 siblings, 2 replies; 11+ messages in thread
From: Fujii Masao @ 2026-05-07 15:04 UTC (permalink / raw)
To: Shinya Kato <[email protected]>; +Cc: pgsql-hackers
On Mon, May 4, 2026 at 4:58 PM Shinya Kato <[email protected]> wrote:
>
> Hi hackers,
>
> While reading the logical replication initial table sync code, I
> noticed that copy_table() calls BeginCopyFrom() and CopyFrom() but
> never calls the matching EndCopyFrom().
>
> EndCopyFrom() calls pgstat_progress_end_command(), which resets
> st_progress_command to PROGRESS_COMMAND_INVALID. Without that call,
> the backend status entry continues to report an active COPY operation
> while the tablesync worker proceeds to WAL catchup. As a result,
> pg_stat_progress_copy shows a stale entry for the entire WAL catchup
> phase.
>
> Attached patch adds EndCopyFrom(cstate) immediately after
> CopyFrom(cstate) returns.
Thanks for the patch! It looks good to me.
Barring any objections, I will commit the patch.
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-08 02:34 =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
parent: Fujii Masao <[email protected]>
1 sibling, 2 replies; 11+ messages in thread
From: =?utf-8?B?Y2NhNTUwNw==?= @ 2026-05-08 02:34 UTC (permalink / raw)
To: =?utf-8?B?RnVqaWkgTWFzYW8=?= <[email protected]>; =?utf-8?B?U2hpbnlhIEthdG8=?= <[email protected]>; +Cc: pgsql-hackers
Hi,
Maybe we want to add "free_parsestate(pstate);" after the "EndCopyFrom()" as well?
--
Regards,
ChangAo Chen
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-08 03:32 Chao Li <[email protected]>
parent: =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
1 sibling, 0 replies; 11+ messages in thread
From: Chao Li @ 2026-05-08 03:32 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Fujii Masao <[email protected]>; Shinya Kato <[email protected]>; pgsql-hackers
> On May 8, 2026, at 10:34, cca5507 <[email protected]> wrote:
>
> Hi,
>
> Maybe we want to add "free_parsestate(pstate);" after the "EndCopyFrom()" as well?
>
I agree. While here, looks like attnamelist can also be freed.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-08 04:21 Fujii Masao <[email protected]>
parent: =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
1 sibling, 2 replies; 11+ messages in thread
From: Fujii Masao @ 2026-05-08 04:21 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Shinya Kato <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-08 04:46 =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
parent: Fujii Masao <[email protected]>
1 sibling, 0 replies; 11+ messages in thread
From: =?utf-8?B?Y2NhNTUwNw==?= @ 2026-05-08 04:46 UTC (permalink / raw)
To: =?utf-8?B?RnVqaWkgTWFzYW8=?= <[email protected]>; +Cc: =?utf-8?B?U2hpbnlhIEthdG8=?= <[email protected]>; pgsql-hackers
> > 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().
Yeah, I agree that it's harmless. I just noticed the comment above make_parsestate():
Caller should eventually release the ParseState via free_parsestate().
Not sure whether it's worth to fix all of these places.
--
Regards,
ChangAo Chen
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-08 05:09 Chao Li <[email protected]>
parent: Fujii Masao <[email protected]>
1 sibling, 1 reply; 11+ messages in thread
From: Chao Li @ 2026-05-08 05:09 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: cca5507 <[email protected]>; Shinya Kato <[email protected]>; pgsql-hackers
> 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, ©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,
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-08 18:05 Shinya Kato <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Shinya Kato @ 2026-05-08 18:05 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Fujii Masao <[email protected]>; cca5507 <[email protected]>; pgsql-hackers
On Fri, May 8, 2026, 14:10 Chao Li <[email protected]> wrote:
>
> 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.
>
I agree that no additional memory cleanup is needed here.
> 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.
>
Thanks for the suggestion.
The copybuf change looks worthwhile, but perhaps it’s better discussed in a
separate thread.
--
Shinya Kato
>
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-09 05:57 Chao Li <[email protected]>
parent: Shinya Kato <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Chao Li @ 2026-05-09 05:57 UTC (permalink / raw)
To: Shinya Kato <[email protected]>; +Cc: Fujii Masao <[email protected]>; cca5507 <[email protected]>; pgsql-hackers
> On May 9, 2026, at 02:05, Shinya Kato <[email protected]> wrote:
>
>
>
> On Fri, May 8, 2026, 14:10 Chao Li <[email protected]> wrote:
>
> 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.
>
> I agree that no additional memory cleanup is needed here.
>
>
> 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.
>
> Thanks for the suggestion.
>
> The copybuf change looks worthwhile, but perhaps it’s better discussed in a separate thread.
>
Sound fair. Let me post it to a separate thread.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-13 02:49 Fujii Masao <[email protected]>
parent: Fujii Masao <[email protected]>
1 sibling, 1 reply; 11+ messages in thread
From: Fujii Masao @ 2026-05-13 02:49 UTC (permalink / raw)
To: Shinya Kato <[email protected]>; +Cc: pgsql-hackers
On Fri, May 8, 2026 at 12:04 AM Fujii Masao <[email protected]> wrote:
> Thanks for the patch! It looks good to me.
> Barring any objections, I will commit the patch.
I've pushed the patch. Thanks!
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Call EndCopyFrom() after initial table sync in logical replication
@ 2026-05-13 05:04 Shinya Kato <[email protected]>
parent: Fujii Masao <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Shinya Kato @ 2026-05-13 05:04 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers
On Wed, May 13, 2026 at 11:49 AM Fujii Masao <[email protected]> wrote:
>
> On Fri, May 8, 2026 at 12:04 AM Fujii Masao <[email protected]> wrote:
> > Thanks for the patch! It looks good to me.
> > Barring any objections, I will commit the patch.
>
> I've pushed the patch. Thanks!
Thanks for committing this, Fujii-san!
--
Best regards,
Shinya Kato
NTT OSS Center
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-05-13 05:04 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-04 07:58 Call EndCopyFrom() after initial table sync in logical replication Shinya Kato <[email protected]>
2026-05-07 15:04 ` Fujii Masao <[email protected]>
2026-05-08 02:34 ` =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
2026-05-08 03:32 ` Chao Li <[email protected]>
2026-05-08 04:21 ` Fujii Masao <[email protected]>
2026-05-08 04:46 ` =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
2026-05-08 05:09 ` Chao Li <[email protected]>
2026-05-08 18:05 ` Shinya Kato <[email protected]>
2026-05-09 05:57 ` Chao Li <[email protected]>
2026-05-13 02:49 ` Fujii Masao <[email protected]>
2026-05-13 05:04 ` Shinya Kato <[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