Re: Add null termination to string received in parallel apply worker

On Thu, 12 Oct 2023 at 05:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > I was thinking about this when skimming the other StringInfo thread a
> > couple of days ago. I wondered if it wouldn't be more convenient to
> > change the convention that all StringInfos are null-terminated: what is
> > really the reason to have them all be like that?
>
> It makes sense for StringInfos containing text, not because the
> stringinfo.c routines need it but because callers inspecting the
> string will very likely do something that expects nul-termination.
> When the StringInfo contains binary data, that argument has little
> force of course.

I'd like to know why we're even using StringInfo for receiving bytes
pq_* functions.

It does, unfortunately, seem like we're well down that path now and
changing it would be quite a bit of churn, and not just for core :-(
If we had invented a ByteReceiver or something, then StringInfoData
wouldn't need a cursor field (perhaps with the exception of its use in
string_agg_transfn/string_agg_combine, but that could be done some
other way).

It seems like we're trying to enforce rules that are useful for
StringInfo's likely intended original purpose that are often just not
that relevant to the job it's ended up doing in pq_* functions. It
would be good if we could relax the most-be-NUL-terminated rule. It
seems to be causing quite a bit of ugliness around the code. Just
search for "csave". We often use a variable by that name to save the
char where we temporarily put a NUL so we restore it again. A comment
exec_bind_message() does admit this is "grotty", which I don't
disagree with.

David