Re: Making aggregate deserialization (and WAL receive) functions slightly faster

David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> I've attached a patch which builds on the previous patch and relaxes
> the rule that the StringInfo must be NUL-terminated. The rule is
> only relaxed for StringInfos that are initialized with
> initReadOnlyStringInfo.

Yeah, that's probably a reasonable way to frame it.

> There's also an existing confusing comment in logicalrep_read_tuple()
> which seems to think we're just setting the NUL terminator to conform
> to StringInfo's practises. This is misleading as the NUL is required
> for LOGICALREP_COLUMN_TEXT mode as we use the type's input function
> instead of the receive function. You don't have to look very hard to
> find an input function that needs a NUL terminator.

Right, input functions are likely to expect this.

> I'm a bit less confident that the type's receive function will never
> need to be NUL terminated. cstring_recv() came to mind as one I should
> look at, but on looking I see it's not required as it just reads the
> remaining bytes from the input StringInfo. Is it safe to assume this?

I think that we can make that assumption starting with v17.
Back-patching it would be hazardous perhaps; but if there's some
function out there that depends on NUL termination, testing should
expose it before too long. Wouldn't hurt to mention this explicitly
as a possible incompatibility in the commit message.

Looking over the v5 patch, I have some nits:

* In logicalrep_read_tuple,
s/input function require that/input functions require that/
(or fix the grammatical disagreement some other way)

* In exec_bind_message, you removed the comment pointing out that
we are scribbling directly on the message buffer, even though
we still are. This patch does nothing to make that any safer,
so I object to removing the comment.

* In stringinfo.h, I'd suggest adding text more or less like this
within or at the end of the "As a special case, ..." para in
the first large comment block:

* Also, it is caller's option whether a read-only string buffer has
* a terminating '\0' or not. This depends on the intended usage.

That's partially redundant with some other comments, but this para
is defining the API for read-only buffers, so I think it would
be good to include it here.

regards, tom lane