On Fri, Oct 27, 2023 at 3:23 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> 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.
>
Both the above calls are used to handle ERROR/NOTICE messages from
parallel workers as you have also noticed. The comment atop
initReadOnlyStringInfo() clearly states that it is used in the
performance-critical path. So, is it worth changing these places? In
the future, this may pose the risk of this API being used
inconsistently.
--
With Regards,
Amit Kapila.