Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix stale COPY progress during logical replication table sync

  2. Use stack-allocated StringInfoDatas, where possible

  1. Call EndCopyFrom() after initial table sync in logical replication

    Shinya Kato <shinya11.kato@gmail.com> — 2026-05-04T07:58:12Z

    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

  2. Re: Call EndCopyFrom() after initial table sync in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-05-07T15:04:27Z

    On Mon, May 4, 2026 at 4:58 PM Shinya Kato <shinya11.kato@gmail.com> 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

  3. Re: Call EndCopyFrom() after initial table sync in logical replication

    cca5507 <cca5507@qq.com> — 2026-05-08T02:34:56Z

    Hi,

Maybe we want to add "free_parsestate(pstate);" after the "EndCopyFrom()" as well?

--
Regards,
ChangAo Chen

  4. Re: Call EndCopyFrom() after initial table sync in logical replication

    Chao Li <li.evan.chao@gmail.com> — 2026-05-08T03:32:50Z

    
> On May 8, 2026, at 10:34, cca5507 <cca5507@qq.com> 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/

  5. Re: Call EndCopyFrom() after initial table sync in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-05-08T04:21:31Z

    On Fri, May 8, 2026 at 11:34 AM cca5507 <cca5507@qq.com> 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

  6. Re: Call EndCopyFrom() after initial table sync in logical replication

    cca5507 <cca5507@qq.com> — 2026-05-08T04:46:14Z

    > > 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

  7. Re: Call EndCopyFrom() after initial table sync in logical replication

    Chao Li <li.evan.chao@gmail.com> — 2026-05-08T05:09:38Z

    
> On May 8, 2026, at 12:21, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Fri, May 8, 2026 at 11:34 AM cca5507 <cca5507@qq.com> 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/

  8. Re: Call EndCopyFrom() after initial table sync in logical replication

    Shinya Kato <shinya11.kato@gmail.com> — 2026-05-08T18:05:20Z

    On Fri, May 8, 2026, 14:10 Chao Li <li.evan.chao@gmail.com> 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

>

  9. Re: Call EndCopyFrom() after initial table sync in logical replication

    Chao Li <li.evan.chao@gmail.com> — 2026-05-09T05:57:10Z

    
> On May 9, 2026, at 02:05, Shinya Kato <shinya11.kato@gmail.com> wrote:
> 
> 
> 
> On Fri, May 8, 2026, 14:10 Chao Li <li.evan.chao@gmail.com> 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/

  10. Re: Call EndCopyFrom() after initial table sync in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-05-13T02:49:25Z

    On Fri, May 8, 2026 at 12:04 AM Fujii Masao <masao.fujii@gmail.com> 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

  11. Re: Call EndCopyFrom() after initial table sync in logical replication

    Shinya Kato <shinya11.kato@gmail.com> — 2026-05-13T05:04:04Z

    On Wed, May 13, 2026 at 11:49 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Fri, May 8, 2026 at 12:04 AM Fujii Masao <masao.fujii@gmail.com> 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