> 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/
tablesync.c.diff
Description: Binary data
