On Thu, 26 Oct 2023 at 17:00, David Rowley <dgrowleyml@gmail.com> wrote:
> Thanks for looking at this again. I fixed up each of those and pushed
> the result, mentioning the incompatibility in the commit message.
>
> Now that that's done, I've attached a patch which makes use of the new
> initReadOnlyStringInfo initializer function for the original case
> mentioned when I opened this thread. I don't think there are any
> remaining objections to this, but I'll let it sit for a bit to see.
I've just pushed the deserial function optimisation patch.
I was just looking at a few other places where we might want to make
use of initReadOnlyStringInfo.
* parallel.c in HandleParallelMessages():
Drilling into HandleParallelMessage(), I see the PqMsg_BackendKeyData
case just reads a fixed number of bytes. In some of the other
"switch" cases, I see calls pq_getmsgrawstring() either directly or
indirectly. I see the counterpart to pq_getmsgrawstring() is
pq_sendstring() which always appends the NUL char to the StringInfo,
so I don't think not NUL terminating the received bytes is a problem
as cstrings seem to be sent with the NUL terminator.
This case just seems to handle ERROR/NOTICE messages coming from
parallel workers. Not tuples themselves. It may not be that
interesting a case to speed up.
* applyparallelworker.c in HandleParallelApplyMessages():
Drilling into HandleParallelApplyMessage(), I don't see anything there
that needs the input StringInfo to be NUL terminated.
* worker.c in apply_spooled_messages():
Drilling into apply_dispatch() and going through each of the cases, I
see logicalrep_read_tuple() pallocs a new buffer and ensures it's
always NUL terminated which will be required in LOGICALREP_COLUMN_TEXT
mode. (There seems to be further optimisation opportunities there
where we could not do the palloc when in LOGICALREP_COLUMN_BINARY mode
and just point value's buffer directly to the correct portion of the
input StringInfo's buffer).
* walreceiver.c in XLogWalRcvProcessMsg():
Nothing there seems to require the incoming_message StringInfo to have
a NUL terminator. I imagine this one is the most worthwhile to do out
of the 4. I've not tested to see if there are any performance
improvements.
Does anyone see any reason why we can't do the attached?
David