Re: [bug fix] ECPG: freeing memory for pgtypes crashes on Windows - Mailing list pgsql-hackers

Hello.

The objective of this patch looks reasonable and this doesn't
affect ecpg applications except for the problematic case that
happens only on Windows. So the points here are only the
documentation, the new function name and the how we should place
the new defintion.

At Mon, 5 Feb 2018 00:53:47 +0000, "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> wrote in
<0A3221C70F24FB45833433255569204D1F8AE06B@G01JPEXMBYT05>
> From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
> > +#ifndef PGTYPES_FREE
> > +#define PGTYPES_FREE
> > + extern void PGTYPES_free(void *ptr);
> > +#endif
> > 
> > It seems quite strange to repeat this in pgtypes_date.h, pgtypes_interval.h
> > and pgtypes_numeric.h.  I guess you might not want to introduce a new common
> > header file so that his can be back-patched more easily?  Not sure if there
> > is a project policy about that, but it seems unfortunate to introduce
> > maintenance burden by duplicating this.
> 
> Your guess is correct.  I wanted to avoid the duplication, but there was no good place to put this without requiring
existingapplications to change their #include directives.  
 
> 
> 
> > +   <function>PGTYPES_free()/<function> instead of
> > <function>free()</function>.
> > 
> > The "/" needs to move inside then closing tag.
> 
> Thanks, fixed.

The types PGTYPES* has corresponding PGTYPES*_free() functions. I
found it a bit strange that the function is named as
PGTYPES_free(), which looks as if it is generic for all PGTYPES*
types. Perhaps PGTYPESchar_free() or PGTYPEStext_free() would be
preferable.

The documentation for PQescapeByteaConn is saying that:

https://www.postgresql.org/docs/10/static/libpq-exec.html#LIBPQ-PQESCAPEBYTEACONN

> PQescapeByteaConn returns an escaped version of the from
> parameter binary string in memory allocated with malloc(). This
> memory should be freed using PQfreemem() when the result is no
> longer needed.

The similar description is needed for the four counterparts of
the new free function nor users doesn't have a definite
suggestion on where to use it.

I cannot (or am quite reluctant to^^;) replay the problem,
instead, I looked over the test/* files and the replacement looks
correct.

I agree to Thomas on the opinion that the definition of
PGTYPES_free shouldn't be scattered around. There's some header
files that has only one or two function defenitions. I believe
that a new header file having only this definition is preferable
than the current shape.

# By the way, I think that multiple occurance of function
# prototypes for the same function don't get error as long as
# they are identical. Or does for CL?

> Your guess is correct.  I wanted to avoid the duplication, but
> there was no good place to put this without requiring existing
> applications to change their #include directives.

I suppose that it is enough to let the pgtype headers
(pgtypes_(timestamp|numeric|interfval).h) include the new common
header. *I* think that it is better than multiple definitions for
the same function are placed here and there. Applications don't
need to be changed with this.

# Anyway existing applications need to replace free() to
# PGTYPES_free()..

I think this doesn't need more profound review so I'll mark this
Ready For Commit after confirming the amendment.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center