Re: Proposal: add new API to stringinfo

On Sun, 5 Jan 2025 at 21:03, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> Attached is the v2 patch to reflect above.

A quick review.

I don't see any users of STRINGINFO_SMALL_SIZE, so I question the
value in defining it.

I think it might be worth adding a comment to initStringInfoExt and
makeStringInfoExt which mentions the valid ranges of initsize. 1 to
MaxAllocSize by the looks of it. The reason that it's good to document
this is that if we ever get any bug reports where some code is passing
something like 0 as the size, we'll know right away that it's the
caller's fault rather than the callee.

I'm also curious if this change has much of a meaningful impact in the
size of the compiled binary. The macro idea isn't quite how I'd have
done it as it does mean passing an extra parameter for every current
callsite. If I were doing this, I'd just have created the new
functions in stringinfo.c and have the current functions call the new
external function and rely on the compiler inlining, basically, that's
the same as what makeStringInfo() does today with initStringInfo().

I'd have done it like:

StringInfo
makeStringInfo(void)
{
return makeStringInfoExt(STRINGINFO_DEFAULT_SIZE);
}

Using current master, you can see from the following that
initStringInfo is inlined, despite being an external function. I've no
reason to doubt the compiler won't do the same for makeStringInfo()
and makeStringInfoExt().

$ gcc src/common/stringinfo.c -I src/include -O2 -S | cat stringinfo.s
| grep makeStringInfo -A 10
.globl makeStringInfo
.type makeStringInfo, @function
makeStringInfo:
.LFB94:
.cfi_startproc
endbr64
pushq %r12
.cfi_def_cfa_offset 16
.cfi_offset 12, -16
movl $24, %edi
call palloc(at)PLT
movl $1024, %edi
movq %rax, %r12
--
.size makeStringInfo, .-makeStringInfo
.p2align 4
.globl initStringInfo
.type initStringInfo, @function
initStringInfo:
.LFB95:
.cfi_startproc
endbr64
pushq %rbx
.cfi_def_cfa_offset 16
.cfi_offset 3, -16

Note, there is no "call initStringInfo"

David