Re: pl/perl and utf-8 in sql_ascii databases

Hi,

we have a database that is storing strings in various encodings (and
non-encodings, namely the arbitrary byte soup that you might see in
email headers from the internet). For this reason, the database uses
sql_ascii encoding. The columns are text, as most characters are
ascii, so bytea didn't seem the right way to go.

Currently we are on 8.3 and try to upgrade to 9.1, but the plperlu
functions we have are acting up.

Old behavior on 8.3 .. 9.0:

sql_ascii =# create or replace function whitespace(text) returns text
language plperlu as $$ $a = shift; $a =~ s/[\t ]+/ /g; return $a; $$;
CREATE FUNCTION

sql_ascii =# select whitespace (E'\200'); -- 0x80 is not valid utf-8
whitespace
------------

sql_ascii =# select whitespace (E'\200')::bytea;
whitespace
------------
\x80

New behavior on 9.1.2:

sql_ascii =# select whitespace (E'\200');
ERROR: XX000: Malformed UTF-8 character (fatal) at line 1.
KONTEXT: PL/Perl function "whitespace"
ORT: plperl_call_perl_func, plperl.c:2037

A crude workaround is:

sql_ascii =# create or replace function whitespace_utf8_off(text)
returns text language plperlu as $$ use Encode; $a = shift;
Encode::_utf8_off($a); $a =~ s/[\t ]+/ /g; return $a; $$;
CREATE FUNCTION

sql_ascii =# select whitespace_utf8_off (E'\200');
whitespace_utf8_off
---------------------
\u0080

sql_ascii =# select whitespace_utf8_off (E'\200')::bytea;
whitespace_utf8_off
---------------------
\xc280

(Note that the workaround is not perfect as the resulting 0x80..0xff
bytes are still tagged to be utf8.)

I think the bug is in plperl_helpers.h:

/*
* Create a new SV from a string assumed to be in the current database's
* encoding.
*/

static inline SV *
cstr2sv(const char *str)
{
SV *sv;
char *utf8_str = utf_e2u(str);

sv = newSVpv(utf8_str, 0);
SvUTF8_on(sv);

pfree(utf8_str);

return sv;
}

In sql_ascii databases, utf_e2u does not do any recoding, but then
SvUTF8_on still marks the string as utf-8, while it isn't.

(Returned values might also need fixing.)

In my view, this is clearly a bug in pl/perl on sql_ascii databases.

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

On Thu, Feb 9, 2012 at 03:21, Christoph Berg <cb(at)df7cb(dot)de> wrote:
> Hi,
>
> we have a database that is storing strings in various encodings (and
> non-encodings, namely the arbitrary byte soup [ ... ]
> For this reason, the database uses
> sql_ascii encoding

> ...snip...

> In sql_ascii databases, utf_e2u does not do any recoding, but then
> SvUTF8_on still marks the string as utf-8, while it isn't.
>
> (Returned values might also need fixing.)
>
> In my view, this is clearly a bug in pl/perl on sql_ascii databases.

Yeah, there was some musing about this over in:
http://archives.postgresql.org/pgsql-hackers/2011-02/msg01142.php

Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
and SvPVUTF8() when turning a perl string into a cstring.

With the attached I get:
=> create or replace function perl_white(a text) returns text as $$
return shift; $$ language plperlu;
=> select perl_white(E'\200'), perl_white(E'\200')::bytea,
coalesce(perl_white(E'\200'), 'null');
perl_white | perl_white | coalesce
------------+------------+----------
| \x80 |

=> select perl_white(E'\401');
perl_white
------------
\x01
(1 row)

Does the attached fix the issue for you?

Ill note that all the pls seem to behave a bit differently:

=> create or replace function py_white(a text) returns text as $$
return a; $$ language plpython3u;
=> select py_white(E'\200'), py_white(E'\200')::bytea,
coalesce(py_white(E'\200'), 'null');
py_white | py_white | coalesce
----------+----------+----------
| | null
(1 row)

=>select py_white(E'\401');
py_white
----------
\x01
(1 row)

=> create or replace function tcl_white(text) returns text as $$
return $1; $$ language pltcl;
=> select tcl_white(E'\200'), tcl_white(E'\200')::bytea,
coalesce(tcl_white(E'\200'), 'null');
tcl_white | tcl_white | coalesce
-----------+-----------+----------
| \x80 |

=> select tcl_white(E'\402');
tcl_white
-----------
\x02
(1 row)

Re: Alex Hunsaker 2012-02-10 <CAFaPBrR9y1fu6gpVu+8TA8vTY6QVCm3DfarKT8JG_EhGeTXnDA(at)mail(dot)gmail(dot)com>
> Does the attached fix the issue for you?

Yes. :)

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:

> Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
> and SvPVUTF8() when turning a perl string into a cstring.

Hmm, this patch belongs into back branches too, right? Not just the
current development tree?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Jun 18, 2012 at 3:30 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:
>
>> Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
>> and SvPVUTF8() when turning a perl string into a cstring.
>
> Hmm, this patch belongs into back branches too, right?  Not just the
> current development tree?

It seems like a bug fix to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jun 18, 2012 at 1:30 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
>
> Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:
>
>> Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
>> and SvPVUTF8() when turning a perl string into a cstring.
>
> Hmm, this patch belongs into back branches too, right?  Not just the
> current development tree?

(Have been out of town, sorry for the late reply)

Yeah, back to 9.1.

Excerpts from Robert Haas's message of mar jun 19 11:36:41 -0400 2012:
>
> On Mon, Jun 18, 2012 at 3:30 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
> > Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:
> >
> >> Seems like we missed the fact that we still did SvUTF8_on() in sv2cstr
> >> and SvPVUTF8() when turning a perl string into a cstring.
> >
> > Hmm, this patch belongs into back branches too, right?  Not just the
> > current development tree?
>
> It seems like a bug fix to me.

That's what I thought. I will commit to both branches soon, then.
Mind you, this should have been an "open item", not a commitfest item.
(Actually not even an open item. We should have committed it right
away.)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Jun 19, 2012 at 3:03 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> That's what I thought.  I will commit to both branches soon, then.

I think there might be three branches involved.

> Mind you, this should have been an "open item", not a commitfest item.
> (Actually not even an open item.  We should have committed it right
> away.)

True, but it's not a perfect world, and I didn't feel qualified to
commit it myself. Adding it the CommitFest at least prevented it from
getting lost forever.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:

> 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.

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).

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:

*** /pgsql/source/HEAD/src/pl/plperl/expected/plperl.out 2012-05-16 13:38:02.495647259 -0400
--- /home/alvherre/Code/pgsql/build/HEAD/src/pl/plperl/results/plperl.out 2012-06-20 15:09:19.869778824 -0400
***************
*** 658,664 ****
return "abcd\0efg";
$$ LANGUAGE plperl;
SELECT perl_zerob();
! ERROR: invalid byte sequence for encoding "UTF8": 0x00
CONTEXT: PL/Perl function "perl_zerob"
-- make sure functions marked as VOID without an explicit return work
CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$
--- 658,664 ----
return "abcd\0efg";
$$ LANGUAGE plperl;
SELECT perl_zerob();
! ERROR: invalid byte sequence for encoding "SQL_ASCII": 0x00
CONTEXT: PL/Perl function "perl_zerob"
-- make sure functions marked as VOID without an explicit return work
CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$

======================================================================

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.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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...

> 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?

-- Coding and styles.

This also seems to have polished the previous one on some codes,
styles and comments which generally look reasonable. And patch
style was corrected into unified.

-- Functions
I seems to work properly on the database the encodings of which
are SQL_ASCII and UTF8 (and EUC-JP) as below,

=================
=> create or replace function foo(text) returns text language plperlu as $$ $a = shift; return "BOO!" if ($a != "a\x80cあ"); return $a; $$;
SQL_ASCII=> select foo(E'a\200cあ') = E'a\200cあ';
?column?
----------
t
UTF8=> select foo(E'a\200cあ');
ERROR: invalid byte sequence for encoding "UTF8": 0x80
UTF8=> select foo(E'a\302\200cあ') = E'a\u0080cあ';
?column?
----------
t
=================

This looks quite valid according to the definition of the
encodings and perl's nature as far as I see.

-- 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.

> 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.

# UtfToLocal() seems to have a bug that always report illegal
# encoding was "UTF8" regardless of the real encoding. But
# plper_lc_*.(sql|out) increases if the bug is fixed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Ouch!

> # UtfToLocal() seems to have a bug that always report illegal
> # encoding was "UTF8" regardless of the real encoding. But
> # plper_lc_*.(sql|out) increases if the bug is fixed.

This is not a bug. The error message "invalid byte sequence for
encoding "UTF8"" meant "invalid INPUT byte sequence as encoding
"UTF8"" not encoding for output.. So the regtest patch covers all
of possible patterns - UTF8 and SQL_ASCII..

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

On Wed, Jun 20, 2012 at 1:15 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:
>
>> 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.
>
> 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).

I think most of these leaks go back to 9.0. Dunno if its worth
backpatching them...

> to test the problem in the original report, I notice that we now have a
> regression failure:

> 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.

I think we have broken that check twice so it seems like it would be
nice to keep. But I don't feel *to* strongly about it.

The comment and cleanups all look good to me.

Thanks!

On Thu, Jun 21, 2012 at 5:22 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

>> So I played a bit with this patch, and touched it a bit mainly
>> [...] functions in Util.xs might leak some memory, so I made an attempt to

> Ok, Is it ok to look into the newer patch including fix of leaks
> at first?

Yeah :-).

> Variable naming in util_quote_*() seems a bit confusing,
>
> Renaming ret to quoted and str to ret as the patch attached might
> make it easily readable.

Ok.

>> [ regression failure on a SQL_ASCII database ]
>> 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.

+REGRESS_LC0 = $(subst .sql,,$(shell cd sql; ls plperl_lc_$(shell echo
$(ENCODING) | tr "A-Z-" "a-z_").sql 2>/dev/null))
+REGRESS_LC = $(if $(REGRESS_LC0),$(REGRESS_LC0),plperl_lc)
+REGRESS = plperl $(REGRESS_LC) plperl_trigger plperl_shared
plperl_elog plperl_util plperl_init plperlu plperl_array

Hrm, that's quite cute. I dunno if there is a more cannon way of doing
the above-- but it seems to work. I'm not sure this regression test is
worth it. I'm thinking maybe we should just remove
theegressionegression test instead.

There is a minor issue with the patch where
sql/plperl_lc_sql_ascii.sql contains the text "plperl_lc.sql". After
copying sql/plperl_lc.sql to sql/plperl_lc_sql_ascii.sql everything
worked as described.

Thanks for the review!

Excerpts from Kyotaro HORIGUCHI's message of jue jun 21 08:02:58 -0400 2012:
>
> Ouch!
>
> > # UtfToLocal() seems to have a bug that always report illegal
> > # encoding was "UTF8" regardless of the real encoding. But
> > # plper_lc_*.(sql|out) increases if the bug is fixed.
>
> This is not a bug. The error message "invalid byte sequence for
> encoding "UTF8"" meant "invalid INPUT byte sequence as encoding
> "UTF8"" not encoding for output.. So the regtest patch covers all
> of possible patterns - UTF8 and SQL_ASCII..

Right, that confused me too for a while.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alex Hunsaker's message of jue jun 21 10:27:41 -0400 2012:
>
> On Wed, Jun 20, 2012 at 1:15 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
> > Excerpts from Alex Hunsaker's message of vie feb 10 16:53:05 -0300 2012:
> >
> >> 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.
> >
> > 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).
>
> I think most of these leaks go back to 9.0. Dunno if its worth
> backpatching them...

Well, nobody has ever complained about them so maybe they're just not
worth the effort.

> > to test the problem in the original report, I notice that we now have a
> > regression failure:
>
> > 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.
>
> I think we have broken that check twice so it seems like it would be
> nice to keep. But I don't feel *to* strongly about it.

Hmm, if we're broken it then it's probably best to keep it.

> The comment and cleanups all look good to me.

Thanks for the review.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello.

> > 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.
>
> +REGRESS_LC0 = $(subst .sql,,$(shell cd sql; ls plperl_lc_$(shell echo
snip.
> Hrm, that's quite cute. I dunno if there is a more cannon way of doing
> the above-- but it seems to work. I'm not sure this regression test is
> worth it. I'm thinking maybe we should just remove
> theegressionegression test instead.

I agree. That is the fundamental question. I've coded just for my
fun but I don't see not so much signicance to do that. We might
omit the test for this which is non-ciritical and corner cases.

I'll leave it to your decision whether to do that.

> There is a minor issue with the patch where
> sql/plperl_lc_sql_ascii.sql contains the text "plperl_lc.sql". After
> copying sql/plperl_lc.sql to sql/plperl_lc_sql_ascii.sql everything
> worked as described.

Ah. It is what was a simbolic link. I made the patch with doubt
whether symlink could be encoded into diff, and saw the doubious
result but left as it is :-p. I leaned that no meaningful
symbolic-link cannot be used in source tree managed by git.

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Hello.

> > 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.

I've felt hesitation to do so, too.

The new patch is indeed avoid leaks although which does not lasts
permanently. This will preserve more heap for future work but has
no necessity to do.

On the other hand, the return value of DatumGetTextP() is also
may (and 'should' in this case) palloc'ed but not pfree'd like
other part of PostgreSQL source (as far as I saw..) because of, I
suppose, the nature of these functions that it is
difficult/unable to be predicted/determined whether returning
memory block is palloc'ed ones or not. And the pain to maintain
such codes unrobust for future modification.

From such a point of view, we might be good to refrain to
backport this.

> > 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.

Yes, you're right. But it was easier to add pairs of .sql and
.out to do that. Plus, as I wrote in another message, I'm
unwilling to push it nevertheless I've wrote it:-(

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
>>> +REGRESS_LC0 = $(subst .sql,,$(shell cd sql; ls plperl_lc_$(shell echo

>> Hrm, that's quite cute. I dunno if there is a more cannon way of doing
>> the above-- but it seems to work. I'm not sure this regression test is
>> worth it. I'm thinking maybe we should just remove
>> theegressionegression test instead.

> I agree. That is the fundamental question. I've coded just for my
> fun but I don't see not so much signicance to do that. We might
> omit the test for this which is non-ciritical and corner cases.

We need these tests to work on Windows too, so fancy gmake tricks are
probably not the way to deal with varying results.

regards, tom lane

Hello, thank you for your sugestion.

> > I agree. That is the fundamental question. I've coded just for my
> > fun but I don't see not so much signicance to do that. We might
> > omit the test for this which is non-ciritical and corner cases.
>
> We need these tests to work on Windows too, so fancy gmake tricks are
> probably not the way to deal with varying results.

Hmm. I understand that you suggested that we should do this in
normal regression test.

Ok. Since there found to be only two patterns in the regression
test. The fancy thing is no more needed. I will unfold them and
make sure to work on mingw build environment.

And for one more environment, on the one with VC++.. I'll need a
bit longer time to make out what vcregress.pl does.

On the other things, I will decide as following and sent to
committer as soon as the above is finished.

- The main patch fixes the sql-ascii handling itself shoud ported
into 9.2 and 9.1. Someone shoud work for this. (me?)

- The remainder of the patch whic fixes the easy fixable leakes
of palloc'ed memory won't be ported into 9.1. This is only for
9.3dev.

- The patch for 9.3dev will be provided with the new regression
test. It will be easily ported into 9.1 and 9.2 and there seems
to be no problem technically, but a bit unsure from the other
points of view...

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Hello, Here is regression test runs on pg's also built with
cygwin-gcc and VC++.

The patches attached following,

- plperl_sql_ascii-4.patch : fix for pl/perl utf8 vs sql_ascii
- plperl_sql_ascii_regress-1.patch : regression test for this patch.
I added some tests on encoding to this.

I will mark this patch as 'ready for committer' after this.

For the continuity of the behavior for sql_ascii and the chars
like \x80, It might be better if the main patch is back ported
into 9.1 and 9.2. New regression tests seems to have less
necessity to do since it has not been there from the first..

This regression test runs for all of
Build with gcc3 / Linux(CentOS6.2-64)
Built with Cygwin gcc3 / Windows7-64
Built with VC++2008 / ActivePerl5.12 / Windows7-64

==========

I've been stuck in mud trying to plperl work on windows
environment. I saw many messages complaining that plperl wouldn't
be built to work. For the convenience of those and myself, I
describe the process of building postgresql with plperl on
Windows with cygwin and VC++ I've done below.

> Ok. Since there found to be only two patterns in the regression
> test. The fancy thing is no more needed. I will unfold them and
> make sure to work on mingw build environment.
>
> And for one more environment, on the one with VC++.. I'll need a
> bit longer time to make out what vcregress.pl does.

I could understand what you meant after I managed to build plperl
to run properly. vcregress.pl reads $REGRESS in GNUmakefile so
variable substitution of make does not work on Windows'
regression. I resolved this problem by copying plperl_lc_*.out
files into plperl_lc.out before it runs pg_regress.

> - The main patch fixes the sql-ascii handling itself shoud ported
> into 9.2 and 9.1. Someone shoud work for this. (me?)

done.

> - The remainder of the patch whic fixes the easy fixable leakes
> of palloc'ed memory won't be ported into 9.1. This is only for
> 9.3dev.
>
> - The patch for 9.3dev will be provided with the new regression
> test. It will be easily ported into 9.1 and 9.2 and there seems
> to be no problem technically, but a bit unsure from the other
> points of view...

What should I do for this?

regards,

========
Addition - Building Windows binary for plperl

NOTE: This is ONE example I tried and turned out a success.

A. Cygwin

Versions: Windows 7 64bit
Cygwin 1.7.15
gcc 3.4.4 (cygwin-server is running)

1. Build perl aside system-installed one
perl-5.16.0$ export PATH=/usr/local/bin:/bin:/usr/bin:/usr/sbin
perl-5.16.0$ ./Configure --
perl-5.16.0$ ./Configure -d
perl-5.16.0$ make
perl-5.16.0$ make install

- The first line needed to avoid the Makefile of perl stops by
parens in search path.

2. Build postgresql with --with-perl
pg93dev$ ./configure --with-perl
pg93dev$ make all

3. Run the regression test for plperl
pg93dev/src/pl/plperl$ make check
....
pg93dev/src/pl/plperl$ make check ENCODING=sql-ascii
....

B. VC++

Versions: Windows 7 64bit
Microsoft Visual C++ 2008 Express Edition
Active Perl v6.12.4 x86

1. Install Active Perl normally. Assuming the install location
is "c:\Perl" and I did all operation on cmd.exe after this.

2. Create config.pl in src/tools/msvc
pg> cd src\tools\msvc
msvc> copy config_default.pl config.pl
.... Edit as follows

- perl=>undef, # --with-perl
+ perl=>'c:\Perl', # --with-perl

3. Build it

msvc> C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\bin\vcvars32.bat
msvc> build
msvc> install c:\pgsql

4. Run the regression tests

4.1 Create test database and run postgres for UTF8 test

A> set PATH=c:\pgsql\bin;c:\pgsql\lib;%PATH%
A> initdb -D <pgdata dir> --no-locale --encoding=utf8
A> postgres -D <pgdata dir>

... on another cmd.exe

B> set PATH=c:\pgsql\bin;c:\pgsql\lib;%PATH%
pg> cd src\tools\msvc
msvc> vcregress plcheck

4.2 Run regression test for SQL-ASCII.
A> (delete <pgdata dir> and its contents.)
A> initdb -D <pgdata dir> --no-locale --encoding=sql_ascii
A> postgres -D <pgdata dir>
... same as 4.1 here after...

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

On Tue, Jul 3, 2012 at 2:59 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, Here is regression test runs on pg's also built with
> cygwin-gcc and VC++.

Thank you!

> The patches attached following,
>
> - plperl_sql_ascii-4.patch : fix for pl/perl utf8 vs sql_ascii
> - plperl_sql_ascii_regress-1.patch : regression test for this patch.
> I added some tests on encoding to this.
>
> I will mark this patch as 'ready for committer' after this.

Both patches look good to me..

> I've been stuck in mud trying to plperl work on windows
> environment. I saw many messages complaining that plperl wouldn't
> be built to work. For the convenience of those and myself, I
> describe the process of building postgresql with plperl on
> Windows with cygwin and VC++ I've done below.

Hrm, I don't develop on windows here, but out of curiosity, what were
the messages like?

>> - The remainder of the patch whic fixes the easy fixable leakes
>> of palloc'ed memory won't be ported into 9.1. This is only for
>> 9.3dev.

> What should I do for this?

Just let the commiter decide? :-)

Hello,

> > I've been stuck in mud trying to plperl work on windows
> > environment. I saw many messages complaining that plperl wouldn't
> > be built to work. For the convenience of those and myself, I
> > describe the process of building postgresql with plperl on
> > Windows with cygwin and VC++ I've done below.
>
> Hrm, I don't develop on windows here, but out of curiosity, what were
> the messages like?

My memory about that has already become faint.. As far as I
remember, I saw two patterns of crash.

One is caused by gcc-4's stack-protector in cygperl5_10.dll. It
caused crash on "create function", (or create language). Building
postgresql with gcc-4 did not help for me. Finally, I gave up to
use pre-installed dll and built all including perl with GCC-3 to
make it work.

The another is 0xC0000005 (Access Violation) on 'create language
plperl' for VC10(:-p) vs ActivePerl5.14. This happenend at ERRSV
in plperl_(un)trasted_init(). Replacing ERRSV with
get_sv("@",FALSE) had put down that (but also I don't know if it
works) but finally I had a error "didn't get a CODE reference
from compiling function" on "create function .. language plperl"
which was the sign of dead end for me. I decided to behave well
to use ActivePerl5.12 and VC8 at last. I suppose this is a kind
of so-called "DLL HELL" related to memory allocation. ActivePerl
5.12 links the system's msvcrt.dll but VC links its output with
msvcrxx.dll. MS says memory allocaltion/deallocation across DLL
bounary should cause crash. But I don't know why the pair of
AP5.12 and VC8 results in success.

http://msdn.microsoft.com/en-us/library/ms235460.aspx

badalex> >> - The remainder of the patch whic fixes the easy fixable leakes
badalex> >> of palloc'ed memory won't be ported into 9.1. This is only for
badalex> >> 9.3dev.
badalex>
badalex> > What should I do for this?
badalex>
badalex> Just let the commiter decide? :-)

Agreed.

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Excerpts from Kyotaro HORIGUCHI's message of mar jul 03 04:59:38 -0400 2012:
> Hello, Here is regression test runs on pg's also built with
> cygwin-gcc and VC++.
>
> The patches attached following,
>
> - plperl_sql_ascii-4.patch : fix for pl/perl utf8 vs sql_ascii
> - plperl_sql_ascii_regress-1.patch : regression test for this patch.
> I added some tests on encoding to this.
>
> I will mark this patch as 'ready for committer' after this.

I have pushed these changes to HEAD, 9.2 and 9.1. Instead of the games
with plperl_lc_*.out being copied around, I just used the ASCII version
as plperl_lc_1.out and the UTF8 one as plperl_lc.out.

I chose to backpatch the whole thing instead of cherry-picking parts of
it; that was turning into a tedious and pointless exercise. We'll see
how does the buildfarm like the whole thing -- including on MSVC, which
I did not test at all.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of mar jul 10 16:23:57 -0400 2012:
> Excerpts from Kyotaro HORIGUCHI's message of mar jul 03 04:59:38 -0400 2012:
> > Hello, Here is regression test runs on pg's also built with
> > cygwin-gcc and VC++.
> >
> > The patches attached following,
> >
> > - plperl_sql_ascii-4.patch : fix for pl/perl utf8 vs sql_ascii
> > - plperl_sql_ascii_regress-1.patch : regression test for this patch.
> > I added some tests on encoding to this.
> >
> > I will mark this patch as 'ready for committer' after this.
>
> I have pushed these changes to HEAD, 9.2 and 9.1. Instead of the games
> with plperl_lc_*.out being copied around, I just used the ASCII version
> as plperl_lc_1.out and the UTF8 one as plperl_lc.out.

... and this story hasn't ended yet, because one of the new tests is
failing. See here:

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=magpie&dt=2012-07-11%2010%3A00%3A04

The interesting part of the diff is:

***************
*** 34,41 ****
return ($str ne $match ? $code."DIFFER" : $code."ab\x{5ddd}cd");
$$ LANGUAGE plperl;
SELECT encode(perl_utf_inout(E'ab\xe5\xb1\xb1cd')::bytea, 'escape')
! encode
! --------------------------
! NotUTF8:ab\345\267\235cd
! (1 row)
!
--- 34,38 ----
return ($str ne $match ? $code."DIFFER" : $code."ab\x{5ddd}cd");
$$ LANGUAGE plperl;
SELECT encode(perl_utf_inout(E'ab\xe5\xb1\xb1cd')::bytea, 'escape')
! ERROR: character with byte sequence 0xe5 0xb7 0x9d in encoding "UTF8" has no equivalent in encoding "LATIN1"
! CONTEXT: PL/Perl function "perl_utf_inout"

I am not sure what can we do here other than remove this function and
query from the test.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Jul 11, 2012 at 1:42 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
>
>> I have pushed these changes to HEAD, 9.2 and 9.1. Instead of the games
>> with plperl_lc_*.out being copied around, I just used the ASCII version
>> as plperl_lc_1.out and the UTF8 one as plperl_lc.out.
>
> ... and this story hasn't ended yet, because one of the new tests is
> failing. See here:
>
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=magpie&dt=2012-07-11%2010%3A00%3A04

> [...]
> SELECT encode(perl_utf_inout(E'ab\xe5\xb1\xb1cd')::bytea, 'escape')
> ! ERROR: character with byte sequence 0xe5 0xb7 0x9d in encoding "UTF8" has no equivalent in encoding "LATIN1"
> ! CONTEXT: PL/Perl function "perl_utf_inout"
>
>
> I am not sure what can we do here other than remove this function and
> query from the test.

Hrm, me neither. I say drop em.

Hmm... Sorry for immature patch..

> ... and this story hasn't ended yet, because one of the new tests is
> failing. See here:
>
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=magpie&dt=2012-07-11%2010%3A00%3A04
>
> The interesting part of the diff is:
...
> SELECT encode(perl_utf_inout(E'ab\xe5\xb1\xb1cd')::bytea, 'escape')
> ! ERROR: character with byte sequence 0xe5 0xb7 0x9d in encoding "UTF8" has no equivalent in encoding "LATIN1"
> ! CONTEXT: PL/Perl function "perl_utf_inout"
>
>
> I am not sure what can we do here other than remove this function and
> query from the test.

I've run the regress only for the environment capable to handle
the character U+5ddd (Japanese character which means river)...

The byte sequences which can be decoded and the result byte
sequences of encoding from a unicode character vary among the
encodings.

The problem itself which is the aim of this thread could be
covered without the additional test. That confirms if
encoding/decoding is done as expected on calling the language
handler. I suppose that testing for the two cases and additional
one case which runs pg_do_encoding_conversion(), say latin1,
would be enough to confirm that encoding/decoding is properly
done, since the concrete conversion scheme is not significant
this case.

So I recommend that we should add the test for latin1 and omit
the test from other than sql_ascii, utf8 and latin1. This might
be archieved by create empty plperl_lc.sql and plperl_lc.out
files for those encodings.

What do you think about that?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Very sorry for rotten subject. I resent the message with correct subject.
# Our mail server insisted that the message is spam. sigh..
====
Hmm... Sorry for immature patch..

> ... and this story hasn't ended yet, because one of the new tests is
> failing. See here:
>
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=magpie&dt=2012-07-11%2010%3A00%3A04
>
> The interesting part of the diff is:
...
> SELECT encode(perl_utf_inout(E'ab\xe5\xb1\xb1cd')::bytea, 'escape')
> ! ERROR: character with byte sequence 0xe5 0xb7 0x9d in encoding "UTF8" has no equivalent in encoding "LATIN1"
> ! CONTEXT: PL/Perl function "perl_utf_inout"
>
>
> I am not sure what can we do here other than remove this function and
> query from the test.

I've run the regress only for the environment capable to handle
the character U+5ddd (Japanese character which means river)...

The byte sequences which can be decoded and the result byte
sequences of encoding from a unicode character vary among the
encodings.

The problem itself which is the aim of this thread could be
covered without the additional test. That confirms if
encoding/decoding is done as expected on calling the language
handler. I suppose that testing for the two cases and additional
one case which runs pg_do_encoding_conversion(), say latin1,
would be enough to confirm that encoding/decoding is properly
done, since the concrete conversion scheme is not significant
this case.

So I recommend that we should add the test for latin1 and omit
the test from other than sql_ascii, utf8 and latin1. This might
be archieved by create empty plperl_lc.sql and plperl_lc.out
files for those encodings.

What do you think about that?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Excerpts from Kyotaro HORIGUCHI's message of jue jul 12 00:09:19 -0400 2012:
>
> Hmm... Sorry for immature patch..

No need to apologize.

> > ... and this story hasn't ended yet, because one of the new tests is
> > failing. See here:
> >
> > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=magpie&dt=2012-07-11%2010%3A00%3A04
> >
> > The interesting part of the diff is:
> ...
> > SELECT encode(perl_utf_inout(E'ab\xe5\xb1\xb1cd')::bytea, 'escape')
> > ! ERROR: character with byte sequence 0xe5 0xb7 0x9d in encoding "UTF8" has no equivalent in encoding "LATIN1"
> > ! CONTEXT: PL/Perl function "perl_utf_inout"
> >
> >
> > I am not sure what can we do here other than remove this function and
> > query from the test.
>
> I've run the regress only for the environment capable to handle
> the character U+5ddd (Japanese character which means river)...
>
> The byte sequences which can be decoded and the result byte
> sequences of encoding from a unicode character vary among the
> encodings.

Right. I only ran the test in C and UTF8, not Latin1, so I didn't see
it fail either.

> The problem itself which is the aim of this thread could be
> covered without the additional test. That confirms if
> encoding/decoding is done as expected on calling the language
> handler.

Right.

> I suppose that testing for the two cases and additional
> one case which runs pg_do_encoding_conversion(), say latin1,
> would be enough to confirm that encoding/decoding is properly
> done, since the concrete conversion scheme is not significant
> this case.
>
> So I recommend that we should add the test for latin1 and omit
> the test from other than sql_ascii, utf8 and latin1. This might
> be archieved by create empty plperl_lc.sql and plperl_lc.out
> files for those encodings.
>
> What do you think about that?

I think that's probably too much engineering for something that doesn't
really warrant it. A real solution to this problem could be to create
yet another new test file containing just this function definition and
the query that calls it, and have one expected file for each encoding;
but that's too much work and too many files, I'm afraid.

I can see us supporting tests that require a small number of expected
files. No Make tricks with file copying, though. If we can't get
some easy way to test this without that, I submit we should just remove
the test.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello,

> > I suppose that testing for the two cases and additional
> > one case which runs pg_do_encoding_conversion(), say latin1,
> > would be enough to confirm that encoding/decoding is properly
> > done, since the concrete conversion scheme is not significant
> > this case.
> >
> > So I recommend that we should add the test for latin1 and omit
> > the test from other than sql_ascii, utf8 and latin1. This might
> > be archieved by create empty plperl_lc.sql and plperl_lc.out
> > files for those encodings.
> >
> > What do you think about that?
>
> I think that's probably too much engineering for something that doesn't
> really warrant it. A real solution to this problem could be to create
> yet another new test file containing just this function definition and
> the query that calls it, and have one expected file for each encoding;
> but that's too much work and too many files, I'm afraid.

I agree completely. The balance between the additional complexity
of regress and the what we would get from the complexity...

> I can see us supporting tests that require a small number of expected
> files. No Make tricks with file copying, though. If we can't get
> some easy way to test this without that, I submit we should just remove
> the test.

Ok I agree to do so.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

== My e-mail address has been changed since Apr. 1, 2012.

Excerpts from Kyotaro HORIGUCHI's message of mar jul 17 05:01:10 -0400 2012:

> > I think that's probably too much engineering for something that doesn't
> > really warrant it. A real solution to this problem could be to create
> > yet another new test file containing just this function definition and
> > the query that calls it, and have one expected file for each encoding;
> > but that's too much work and too many files, I'm afraid.
>
> I agree completely. The balance between the additional complexity
> of regress and the what we would get from the complexity...

I had to remove both that test and the one about the 0x80, because it
wasn't working for me in either SQL_ASCII or Latin1, I forget which.
I'm not sure I understand the reason for the failure -- I was getting a
false result instead of true, which was unexpected. Maybe there's a
trivial explanation for this .. or maybe it really is broken.

In any case, maybe it'd be a good idea to have more tests related to
encodings, if we can write them in some reasonable manner. But only in
HEAD, I guess, because having to backpatch stuff and test every branch
in at least three encodings is just too annoying.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support