While working on 16fd03e95, I noticed that in each aggregate
deserialization function, in order to "receive" the bytea value that
is the serialized aggregate state, appendBinaryStringInfo is used to
append the bytes of the bytea value onto a temporary StringInfoData.
Using appendBinaryStringInfo seems a bit wasteful here. We could
really just fake up a StringInfoData and point directly to the bytes
of the bytea value.
The best way I could think of to do this was to invent
initStringInfoFromString() which initialises a StringInfoData and has
the ->data field point directly at the specified buffer. This will
mean that it would be unsafe to do any appendStringInfo* operations on
the resulting StringInfoData as enlargeStringInfo would try to
repalloc the data buffer, which might not even point to a palloc'd
string. I thought it might be fine just to mention that in the
comments for the function, but we could probably do a bit better and
set maxlen to something like -1 and Assert() we never see -1 in the
various append functions. I wasn't sure it was worth it, so didn't do
that.
I had a look around for other places that might be following the same
pattern. I only found range_recv() and XLogWalRcvProcessMsg(). I
didn't adjust the range_recv() one as I couldn't see how to do that
without casting away a const. I did adjust the XLogWalRcvProcessMsg()
one and got rid of a global variable in the process.
I've attached the benchmark results I got after testing how the
modification changed the performance of string_agg_deserialize().
I was hoping this would have a slightly more impressive performance
impact, especially for string_agg() and array_agg() as the aggregate
states of those can be large. However, in the test I ran, there's
only a very slight performance gain. I may just not have found the
best case, however.
David
