On Sun, 12 Feb 2023 at 23:43, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sun, 12 Feb 2023 at 19:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > It could maybe be okay if we added the capability for StringInfoData
> > to understand (and enforce) that its "data" buffer is read-only.
> > However, that'd add overhead to every existing use-case.
>
> I'm not very excited by that. I considered just setting maxlen = -1
> in the new function and adding Asserts to check for that in each of
> the appendStringInfo* functions. However, since the performance gains
> are not so great, I'll probably just drop the whole thing given
> there's resistance.
I know I said I'd drop this, but I was reminded of it again today. I
ended up adjusting the patch so that it no longer adds a helper
function to stringinfo.c and instead just manually assigns the
StringInfo.data field to point to the bytea's buffer. This follows
what's done in some existing places such as
LogicalParallelApplyLoop(), ReadArrayBinary() and record_recv() to
name a few.
I ran a fresh set of benchmarks on today's master with and without the
patch applied. I used the same benchmark as I did in [1]. The average
performance increase from between 0 and 12 workers is about 6.6%.
This seems worthwhile to me. Any objections?
David
[1] https://postgr.es/m/CAApHDvr%3De-YOigriSHHm324a40HPqcUhSp6pWWgjz5WwegR%3DcQ%40mail.gmail.com
