On Mon, Mar 26, 2018 at 6:07 PM, Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote: > At Sun, 25 Mar 2018 22:15:52 +0900, "MauMau" <maumau...@gmail.com> wrote in > <B3BEB35436E3471095762969E2FCEDD6@tunaPC> >> And thank you for your review. All modifications are done. > > Thank you for the new version. I marked this as "Ready for > Committer" with one change.
Hi Tsunakawa-san and Horiguchi-san, I would like to get this committed soon, but I'm not sure about backpatching -- see below. Here's a new version with a couple of minor changes: 1. Small changes to the documentation. 2. I see that you added #include <pgtypes.h> to pgtypes_numeric.h and pgtypes_interval.h. They have functions returning char *. I experimented with removing those and including <pgtypes.h> directly in the .pgc test files, but then I saw why you did that: it changes all the line numbers in the expected output files making the patch much larger. No strong objection there. But I figured we should at least be consistent, so I added #include <pgtypes.h> to pgtypes_timestamp.h and pgtypes_date.h (they also declare functions that return new strings). 3. It seemed unnecessary to declare the new function in extern.h *and* in pgtypes.h. I added #include "pgtypes.h" to common.c instead, and a comment to introduce the section of that file that defines functions from pgtypes.h. 4. I found a place in src/interfaces/ecpg/test/sql/sqlda.pgc where you missed a free() call. Are these changes OK? Why is it OK that we do "free(outp_sqlda)" having got that pointer from a statement like "exec sql fetch 1 from mycur1 into descriptor outp_sqlda"? Isn't that memory allocated by libecpg.dll? The files in this area all seem to lack our standard boilerplate, copyright message, blaming everything on UC Berkeley etc. Your new header fits the existing pattern, so I can't really complain about that. The examples in the documentation call a bunch of _to_asc() functions and then don't free the result, which is a leak, but that isn't your patch's fault. (Example: printf("numeric = %s\n", PGTYPESnumeric_to_asc(num, 0))). One question I have is whether it is against project policy to backport a new file and a new user-facing function. It doesn't seem like a great idea, because it means that client code would need to depend on a specific patch release. Even if we found an existing header to declare this function in, you'd still need to do conditional magic before using it. So... how inconvenient do you think it would be if we did this for 11+ only? Does anyone see a better way to do an API evolution here? It's not beautiful but I suppose one work-around that end-user applications could use while they are stuck on older releases might be something like this, in their own tree, conditional on major version: #define PGTYPESchar_free(x) PGTYPESdate_free((date *)(x)) -- Thomas Munro http://www.enterprisedb.com
pgtypes_freemem_v5.patch
Description: Binary data