Kyotaro HORIGUCHI <horiguchi.kyotaro@oss.ntt.co.jp> writes:
> Hello. This message is a proposal of a pair of patches that
> enables the memory allocator for PGresult in libpq to be
> replaced.
Since there seems to be rough consensus that something like this would
be a good idea, I looked more closely at the details of the patch.
I think the design could use some adjustment.
To start with, the patch proposes exposing some global variables that
affect the behavior of libpq process-wide. This seems like a pretty bad
design, because a single process could contain multiple usages of libpq
with different requirements. As an example, if dblink.c were to set
these variables inside a backend process, it would break usage of libpq
from PL/Perl via DBI. Global variables also tend to be a bad idea
whenever you think about multi-threaded applications --- they require
locking facilities, which are not in this patch.
I think it'd be better to consider the PGresult alloc/free functions to
be a property of a PGconn, which you'd set with a function call along the
lines of PQsetResultAllocator(conn, alloc_func, realloc_func, free_func)
after having successfully opened a connection. Then we just have some
more PGconn fields (and I guess PGresult will need a copy of the
free_func pointer) and no new global variables.
I am also feeling dubious about whether it's a good idea to expect the
functions to have exactly the signature of malloc/free. They are
essentially callbacks, and in most places where a library provides for
callbacks, it's customary to include a "void *" passthrough argument
in case the callback needs some context information. I am not sure that
dblink.c would need such a thing, but if we're trying to design a
general-purpose feature, then we probably should have it. The cost
would be having shim functions inside libpq for the default case, but
it doesn't seem likely that they'd cost enough to notice.
The patch lacks any user documentation, which it surely must have if
we are claiming this is a user-visible feature. And I think it could
use some attention to updating code comments, notably the large block
about PGresult space management near the top of fe-exec.c.
Usually, when writing a feature of this sort, it's a good idea to
implement a prototype use-case to make sure you've not overlooked
anything. So I'd feel happier about the patch if it came along with
a patch to make dblink.c use it to prevent memory leaks.
regards, tom lane
