Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer

Hi,

I am not sure that introducing a separate CopyBuf type is an
improvement over v1.

StringInfoData is not limited to text strings. It is also used for
arbitrary binary data, and initReadOnlyStringInfo() exists specifically
for wrapping an externally owned, possibly non-NUL-terminated buffer.
It initializes data and len, sets cursor to zero, and marks the buffer
as read-only by setting maxlen to zero.

The same pattern is already used in nearby replication code. For
example, walreceiver.c and logical/worker.c use
initReadOnlyStringInfo() to process external binary protocol messages.

Therefore, I think v1 could keep the static StringInfoData and use:

initReadOnlyStringInfo(&copybuf, buf, len);

when a new buffer is received. The memset before starting COPY can
remain to clear any previous state.

V1 already removes both allocations performed by makeStringInfo().
Since copybuf is static, the unused maxlen field occupies only a few
bytes in the worker process; it does not cause a per-table or
per-callback allocation. I would not expect any measurable performance
difference between v1 and v2.

A separate structure can be useful when it represents additional state
or invariants that are not covered by an existing abstraction. In this
case, however, CopyBuf contains only a subset of StringInfoData, while
the documented read-only StringInfoData representation already matches
the required ownership and cursor semantics.

For these reasons, my preference would be to keep the approach from v1
and initialize each received buffer with initReadOnlyStringInfo().

Best regards,
Denis Smirnov

> On 11 May 2026, at 14:09, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
>> On May 9, 2026, at 23:35, Álvaro Herrera <alvherre(at)kurilemu(dot)de> 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/
>
>
>
>
> <v2-0001-Use-simple-struct-for-table-sync-COPY-buffer-stat.patch>