Re: Avoid unnecessary StringInfo allocation in tablesync COPY buffer

Original
>From: Denis Smirnov <darthunix(at)gmail(dot)com>
>Date: 2026-07-18 15:42
>To: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
>Cc: álvaro Herrera <alvherre(at)kurilemu(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, David Rowley <dgrowleyml(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>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(&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

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) 
44973863(at)qq(dot)com