Kyotaro HORIGUCHI <horiguchi.kyot...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers