Original >From: Denis Smirnov <[email protected]> >Date: 2026-07-18 15:42 >To: Chao Li <[email protected]> >Cc: álvaro Herrera <[email protected]>, PostgreSQL-development ><[email protected]>, David Rowley <[email protected]>, Fujii >Masao <[email protected]> >Subject: 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(©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
Hi Denis and álvaro, Thanks for the reviews and suggestions. I revised the patch based on the discussion. Instead of introducing a separate CopyBuf abstraction, v3 reuses the existing StringInfoData mechanism. copybuf is now a function-scope static StringInfoData in copy_read_data(), initialized to zero and re-initialized with initReadOnlyStringInfo() for each incoming buffer. This avoids the heap allocation from makeStringInfo() while preserving the existing handling of leftover unconsumed data across function invocations. Using StringInfoData as a read-only buffer is already an established pattern in the PostgreSQL codebase. I found it being used in several places, including: backend/replication/logical/applyparallelworker.c backend/replication/logical/worker.c backend/replication/walreceiver.c backend/tcop/postgres.c backend/utils/adt/arrayfuncs.c backend/utils/adt/array_userfuncs.c backend/utils/adt/numeric.c backend/utils/adt/rowtypes.c backend/utils/adt/timestamp.c backend/utils/adt/varlena.c Therefore, I think reusing StringInfoData with initReadOnlyStringInfo() is preferable to introducing another CopyBuf abstraction, while also keeping the buffer-related state local to copy_read_data(). The updated patch is attached for review. Thanks again for the feedback. regards, -- ZizhuanLiu (X-MAN) [email protected]
v3-0001-Avoid-unnecessary-allocation-for-tablesync.c-COPY.patch
Description: Binary data
