Excerpts from Kyotaro HORIGUCHI's message of jue jun 21 07:22:43 -0400 2012:
> Hello,
>
> > > Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
> > > and SvPVUTF8() when turning a perl string into a cstring.
> >
> > Right.
>
> I spent a bit longer time catching on pl/perl and now understand
> what is the problem...
Hi,
Thanks for the review.
> > So I played a bit with this patch, and touched it a bit mainly just to
> > add some more comments; and while at it I noticed that some of the
> > functions in Util.xs might leak some memory, so I made an attempt to
> > plug them, as in the attached patch (which supersedes yours).
>
> Ok, Is it ok to look into the newer patch including fix of leaks
> at first?
Yeah, I'm going to have to backpatch the whole of it so having full
review is good. Thanks.
> -- The others
>
> Variable naming in util_quote_*() seems a bit confusing,
>
> > text *arg = sv2text(sv);
> > text *ret = DatumGetTextP(..., PointerGetDatum(arg)));
> > char *str;
> > pfree(arg);
> > str = text_to_cstring(ret);
> > RETVAL = cstr2sv(str);
> > pfree(str);
>
> Renaming ret to quoted and str to ret as the patch attached might
> make it easily readable.
I think I'm going to refrain from this because it will be more painful
to backpatch.
> > Now, with my version of the patch applied and using a SQL_ASCII database
> > to test the problem in the original report, I notice that we now have a
> > regression failure:
> snip.
> > I'm not really sure what to do here -- maybe have a second expected file
> > for that test is a good enough answer? Or should I just take the test
> > out? Opinions please.
>
>
> The attached ugly patch does it. We seem should put NO_LOCALE=1
> on the 'make check' command line for the encodings not compatible
> with the environmental locale, although it looks work.
The idea of separating the test into its own file has its merit; but
instead of having two different tests, I'm going to have a single test
and two expected files. That seems simpler than messing around in the
makefile.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
