> From: Thomas Munro [mailto:thomas.mu...@enterprisedb.com] > 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:
Thank you for taking care of this patch. > 1. Small changes to the documentation. I agree with Tom on this. > 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). The reason I added pgtypes.h only in pgtypes_numeric.h and pgtypes_interval.h is that the opgtypes_date.h includes pgtypes_timestamp.h and pgtypes_timestamp.h in turn includes pgtypes_interval.h. So additional inclusion of pgtypes.h was not necessary. But I'm OK with your patch for consistency. > 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. Agreed, thanks. > 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? My colleague is now preparing a patch for that, which adds a function ECPGFreeSQLDA() in libecpg.so. That thread is here: https://www.postgresql.org/message-id/25C1C6B2E7BE044889E4FE8643A58BA963A42097@G01JPEXMBKW03 > 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)) I want some remedy for older releases. Our customer worked around this problem by getting a libpq connection in their ECPG application and calling PQfreemem(). That's an ugly kludge, and I don't want other users to follow it. I don't see a problem with back-patching as-is, because existing users who just call free() or don't call free() won't be affected. I think that most serious applications somehow state their supported minor releases like "this application supports (or is certified against) PostgreSQL 10.5 or later", just like other applications support "RHEL 6.2 or later" or "Windows XP Sp2 or later." Regards Takayuki Tsunakawa
