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

David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 5 Oct 2023 at 21:24, David Rowley <dgrowleyml@gmail.com> wrote:
>> I looked at the patch again and I just couldn't bring myself to change
>> it to that.  If it were a macro going into stringinfo.h then I'd agree
>> with having a macro or inline function as it would allow the reader to
>> conceptualise what's happening after learning what the function does.

> I've pushed this patch.  I didn't go with the macros in the end. I
> just felt it wasn't an improvement and none of the existing code which
> does the same thing bothers with a macro. I got the idea you were not
> particularly for the macro given that you used the word "Perhaps".

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.

			regards, tom lane