Re: Making aggregate deserialization (and WAL receive) functions slightly faster

On Mon, 9 Oct 2023 at 17:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Sorry for not having paid more attention to this thread ... but
> I'm pretty desperately unhappy with the patch as-pushed.  I agree
> with the criticism that this is a very repetitive coding pattern
> that could have used a macro.  But my real problem with this:
>
> +   buf.data = VARDATA_ANY(sstate);
> +   buf.len = VARSIZE_ANY_EXHDR(sstate);
> +   buf.maxlen = 0;
> +   buf.cursor = 0;
>
> is that it totally breaks the StringInfo API without even
> attempting to fix the API specs that it falsifies,
> particularly this in stringinfo.h:
>
>  *        maxlen  is the allocated size in bytes of 'data', i.e. the maximum
>  *                string size (including the terminating '\0' char) that we can
>  *                currently store in 'data' without having to reallocate
>  *                more space.  We must always have maxlen > len.
>
> I could see inventing a notion of a "read-only StringInfo"
> to legitimize what you've done here, but you didn't bother
> to try.  I do not like this one bit.  This is a fairly
> fundamental API and we shouldn't be so cavalier about
> breaking it.

You originally called the centralised logic a "loaded foot-gun" [1],
but now you're complaining about a lack of loaded foot-gun and want a
macro? Which part did I misunderstand? Enlighten me, please.

Here are some more thoughts on how we could improve this:

1. Adjust the definition of StringInfoData.maxlen to define that -1
means the StringInfoData's buffer is externally managed.
2. Adjust enlargeStringInfo() to add a check for maxlen = -1 and have
it palloc, say, pg_next_pow2(str->len * 2) bytes and memcpy the
existing (externally managed string) into the newly palloc'd buffer.
3. Add a new function along the lines of what I originally proposed to
allow init of a StringInfoData using an existing allocated string
which sets maxlen = -1.
4. Update all the existing places, including the ones I just committed
(plus the ones you committed in ba1e066e4) to make use of the function
added in #3.

Better ideas?

David

[1] https://postgr.es/m/770055.1676183953@sss.pgh.pa.us