On Thu, 5 Oct 2023 at 18:23, Michael Paquier <mich...@paquier.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
