MIME-Version: 1.0
References: 
 <CAOzEurQKuy3RiPkd=25PEwEzaqHuGvEOf=X7vaVzhgNjaukYzA@mail.gmail.com>
 <CAHGQGwEpwuq9U=Hhx3BbfGY-RrJ5rFvdQopO+0xyAuLVsMdfWA@mail.gmail.com>
 <tencent_B56532E9785E072776FBA169850EB5032908@qq.com>
 <CAHGQGwHfdbfRRVdQjYRZmT94UMqE1W4MENiaseVtsHWmzkWxBQ@mail.gmail.com>
 <E6D725B9-1897-498A-BFDC-A86B84E03546@gmail.com>
In-Reply-To: <E6D725B9-1897-498A-BFDC-A86B84E03546@gmail.com>
From: Shinya Kato <shinya11.kato@gmail.com>
Date: Sat, 9 May 2026 03:05:20 +0900
Message-ID: 
 <CAOzEurTDq0_jDnFo64-r-ggt=ykXv-rhotcGDvjaktw+O6VV1g@mail.gmail.com>
Subject: Re: Call EndCopyFrom() after initial table sync in logical
 replication
To: Chao Li <li.evan.chao@gmail.com>
Cc: Fujii Masao <masao.fujii@gmail.com>, cca5507 <cca5507@qq.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000fe19a20651523da7"
Archived-At: 
 <https://www.postgresql.org/message-id/CAOzEurTDq0_jDnFo64-r-ggt%3DykXv-rhotcGDvjaktw%2BO6VV1g%40mail.gmail.com>
Precedence: bulk

--000000000000fe19a20651523da7
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 8, 2026, 14:10 Chao Li <li.evan.chao@gmail.com> wrote:

>
> I don=E2=80=99t think this is a serious leak. In this path, pstate and at=
tnamelist
> 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 =3D 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 wi=
th
> that direction.
>

Thanks for the suggestion.

The copybuf change looks worthwhile, but perhaps it=E2=80=99s better discus=
sed in a
separate thread.

--
Shinya Kato

>

--000000000000fe19a20651523da7
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"auto"><div><br><br><div class=3D"gmail_quote gmail_quote_contai=
ner"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, May 8, 2026, 14:10 Chao =
Li &lt;<a href=3D"mailto:li.evan.chao@gmail.com">li.evan.chao@gmail.com</a>=
&gt; wrote:</div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8=
ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I don=E2=80=99t think this is a serious leak. In this path, pstate and attn=
amelist are allocated in CurTransactionContext, and the transaction is comm=
itted immediately after copy_table() finishes, so that memory is reclaimed =
at transaction end. Explicitly freeing them would be mostly for code readab=
ility, not to fix a memory leak. So, I am okay to not free them.<br></block=
quote></div></div><div dir=3D"auto"><br></div><div dir=3D"auto">I agree tha=
t no additional memory cleanup is=E3=80=80needed here.</div><div dir=3D"aut=
o"><br></div><div dir=3D"auto"><div class=3D"gmail_quote gmail_quote_contai=
ner"><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-le=
ft:1px #ccc solid;padding-left:1ex">
<br>
While tracing the code, I noticed another issue that is probably more worth=
 addressing. copy_table() currently does:<br>
```<br>
=C2=A0 =C2=A0 copybuf =3D makeStringInfo();<br>
```<br>
<br>
But copybuf is only used by copy_read_data(), and there it&#39;s really jus=
t 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 Strin=
gInfo object or its backing buffer at all.<br>
<br>
It would be cleaner to use a plain StringInfoData and simply reinitialize o=
r zero it in copy_table(). See the attached diff for the proposed change.<b=
r>
<br>
David Rowley has made several cleanup changes in this area to prefer stack-=
allocated StringInfoData, for example a63bbc811d41b3567eb37fe2636e660a852db=
bf2. This change seems consistent with that direction.<br></blockquote></di=
v></div><div dir=3D"auto"><br></div><div dir=3D"auto">Thanks for the sugges=
tion.</div><div dir=3D"auto"><br></div><div dir=3D"auto">The copybuf change=
 looks worthwhile, but perhaps it=E2=80=99s better discussed in a separate =
thread.</div><div dir=3D"auto"><br></div><div dir=3D"auto">--</div><div dir=
=3D"auto">Shinya Kato</div><div dir=3D"auto"><div class=3D"gmail_quote gmai=
l_quote_container"><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 =
.8ex;border-left:1px #ccc solid;padding-left:1ex">
</blockquote></div></div></div>

--000000000000fe19a20651523da7--