On Fri, Oct 27, 2023 at 3:23 AM David Rowley <dgrowle...@gmail.com> wrote: > > On Thu, 26 Oct 2023 at 17:00, David Rowley <dgrowle...@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.
