Hi,
On 2026-04-06 14:18:59 -0700, Jacob Champion wrote:
> On Mon, Apr 6, 2026 at 11:46 AM Andres Freund <andres@anarazel.de> wrote:
> > I guess getting rid of the stringinfo/pqexpbuffer split is not that easy, but
> > at least the common memory allocation stuff seems like it should be doable to
> > put through through the same wrappers for both FE/BE, handling whether we want
> > to error out or not by passing MCXT_ALLOC_NO_OOM or not.
>
> We can spell the abstraction layer differently, but how does that help
> us hide the complexity of the OOM behavior? IMO the difference in
> returning NULLs is the entire reason this code is difficult; the
> abstraction layer must necessarily leak [1].
It's not all the complexity, but I think the various indirections do add to
making it hard to understand.
> > How is the only sane answer here not to avoid ever freeing NULLs?
>
> Maybe I didn't parse this question correctly, but I don't want to
> avoid freeing NULLs. It's entirely reasonable and normal to write code
> that frees NULLs.
I think it's a bad idea ever call free(), realloc() etc with a NULL. It's imo
quite the code smell indicating that code lost of track of whether something
was allocated or not.
> > Particularly because this code started out as backend code. Yea, yea, we
> > probably didn't have NULLs due to erroring out on allocation failure, but
> > still.
> >
> > Kinda seems like the fe_memutils.c pfree() should assert that the argument is
> > not null.
>
> Maybe... but if we want to change this, I hope that we'll instead
> consider not naming a function "pfree" when sometimes it is actually
> "free"?
The whole point of having pfree() in FE code is to make it possible to write
common code that doesn't need ifdef around every allocation. I don't see what
the gain of this would be.
> Or else make pfree() behave like free() [2] so that we don't
> have to have that particular papercut at all anymore?
-many
It's also not a path I want to add any unnecessary instructions to.
> It still doesn't help the OOM abstraction leak between libpq and the
> backend, as far as I can tell.
If the code were to use a JsonLexContext field to decide whether to pass
MCXT_ALLOC_NO_OOM to the allocation functions etc it'd at least would make the
code more similar between FE/BE due to both having to deal with NULLs.
That would require adding some optionally OOM safe functions to stringinfo,
but I suspect that would be a good thing anyway (might not be able to do it
with the existing functions, some paths that use stringinfo are quite perf
sensitive, and it does make some code nontrivially more complicated).
Greetings,
Andres Freund
