I have some comments about the first three patches, that deal with
memory management.
v24-0001-Revert-ECPG-s-use-of-pnstrdup.patch
This looks right.
I suppose another approach would be to put a full replacement for
strndup() into src/port/. But since there is currently only one user,
and most other users should be using pnstrdup(), the presented approach
seems ok.
We should take the check for exit() calls from libpq and expand it to
cover the other libraries as well. Maybe there are other problems like
this?
v24-0002-Remove-fe_memutils-from-libpgcommon_shlib.patch
I don't quite understand how this problem can arise. The description says
"""
libpq appears to have no need for this, and the exit() references cause
our libpq-refs-stamp test to fail if the linker doesn't strip out the
unused code.
"""
But under what circumstances does "the linker doesn't strip out" happen?
If this happens accidentally, then we should have seen some buildfarm
failures or something?
Also, one could look further and notice that restricted_token.c and
sprompt.c both a) are not needed by libpq and b) can trigger exit()
calls. Then it's not clear why those are not affected.
v24-0003-common-jsonapi-support-libpq-as-a-client.patch
I'm reminded of thread [0]. I think there is quite a bit of confusion
about the pqexpbuffer vs. stringinfo APIs, and they are probably used
incorrectly quite a bit. There are now also programs that use both of
them! This patch now introduces another layer on top of them. I fear,
at the end, nobody is going to understand any of this anymore. Also,
changing all the programs to link in libpq for pqexpbuffer seems like
the opposite direction from what was suggested in [0].
I think we need to do some deeper thinking here about how we want the
memory management on the client side to work. Maybe we could just use
one API but have some flags or callbacks to control the out-of-memory
behavior.
[0]:
https://www.postgresql.org/message-id/flat/16d0beac-a141-e5d3-60e9-323da75f49bf%40eisentraut.org
