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

On Thu, 5 Oct 2023 at 18:23, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Oct 04, 2023 at 07:47:11PM +1300, David Rowley wrote:
> > The original patch had a new function in stringinfo.c which allowed a
> > StringInfoData to be initialised from an existing string with some
> > given length. Tom wasn't a fan of that because there wasn't any
> > protection against someone trying to use the given StringInfoData and
> > then calling appendStringInfo to append another string. That can't be
> > done in this case as we can't repalloc the VARDATA_ANY(state) pointer
> > due to it not pointing directly to a palloc'd chunk. Tom's complaint
> > seemed to be about having a reusable function which could be abused,
> > so I modified the patch to remove the reusable code. I think your
> > macro idea in stringinfo.h would put the patch in the same position as
> > it was initially.
>
> Ahem, well. Based on this argument my own argument does not hold
> much. Perhaps I'd still use a macro at the top of array_userfuncs.c
> and numeric.c, to avoid repeating the same pattern respectively two
> and four times, documenting once on top of both macros that this is a
> fake StringInfo because of the reasons documented in these code paths.

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.
Having multiple macros defined in various C files means that much
harder as there are more macros to learn. Since we're only talking 4
lines of code, I think I'd rather reduce the number of hops the reader
must do to find out what's going on and just leave the patch as is.

I considered if it might be better to reduce the 4 lines down to 3 by
chaining the assignments like:

buf.maxlen = buf.cursor = 0;

but I think I might instead change it so that maxlen gets set to -1 to
follow what's done in LogicalParallelApplyLoop() and
LogicalRepApplyLoop(). In the absence of having a function/macro in
stringinfo.h, it might make grepping for this type of thing easier.

If anyone else has a good argument for having multiple macros for this
purpose then I could reconsider.

David