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(©buf, 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 <[email protected]> wrote: > > > >> On May 9, 2026, at 23:35, Álvaro Herrera <[email protected]> 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>
