On Mon, Mar 26, 2018 at 6:07 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Sun, 25 Mar 2018 22:15:52 +0900, "MauMau" <maumau307@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
