Thread: plpython issue with Win64 (PG 9.2)

CREATE PROCEDURAL LANGUAGE 'plpython3u';
CREATE OR REPLACE FUNCTION pymax (a integer, b integer)
  RETURNS integer
AS $$
  if a > b:
    return a
  return b
$$ LANGUAGE plpython3u;
SELECT pymax(1, 2);

On 27/06/12 11:51, Asif Naeem wrote:
> Hi,
>
> On Windows 7 64bit, plpython is causing server crash with the following
> test case i.e.
>
> CREATE PROCEDURAL LANGUAGE 'plpython3u';
>> CREATE OR REPLACE FUNCTION pymax (a integer, b integer)
>>    RETURNS integer
>> AS $$
>>    if a>  b:
>>      return a
>>    return b
>> $$ LANGUAGE plpython3u;
>> SELECT pymax(1, 2);

>
> I think primary reason that trigger this issue is when Function
> PLyUnicode_Bytes() calls "PyUnicode_AsEncodedString( ,WIN1252 /*Server
> encoding*/, ) " it fails with null. I built latest pg 9.2 source code with
> python 3.2.2.3 by using Visual Studio 2010. Thanks.

I'll try to reproduce this on Linux, which should be possible given the 
results of your investigation.

Cheers,
Jan

On 27/06/12 13:57, Jan Urbański wrote:
> On 27/06/12 11:51, Asif Naeem wrote:
>> Hi,
>>
>> On Windows 7 64bit, plpython is causing server crash with the following
>> test case i.e.
>>
>> CREATE PROCEDURAL LANGUAGE 'plpython3u';
>>> CREATE OR REPLACE FUNCTION pymax (a integer, b integer)
>>> RETURNS integer
>>> AS $$
>>> if a> b:
>>> return a
>>> return b
>>> $$ LANGUAGE plpython3u;
>>> SELECT pymax(1, 2);
>
>>
>> I think primary reason that trigger this issue is when Function
>> PLyUnicode_Bytes() calls "PyUnicode_AsEncodedString( ,WIN1252 /*Server
>> encoding*/, ) " it fails with null. I built latest pg 9.2 source code
>> with
>> python 3.2.2.3 by using Visual Studio 2010. Thanks.
>
> I'll try to reproduce this on Linux, which should be possible given the
> results of your investigation.

Your analysis is correct, I managed to reproduce this by injecting

serverenc = "win1252";

into PLyUnicode_Bytes. The comment in that function says that Python 
understands all PostgreSQL encoding names except for SQL_ASCII, but 
that's not really true. In your case GetDatabaseEncodingName() returns 
"WIN1252" and Python accepts "CP125".

I'm wondering how this should be fixed. Just by adding more special 
cases in PLyUnicode_Bytes?

Even if we add a switch statement that would convert PG_WIN1250 into 
"CP1250", Python can still raise an exception when encoding (for various 
reasons). How about replacing the PLy_elog there with just an elog? This 
loses traceback support and the Python exception message, which could be 
helpful for debugging (something like "invalid character <foo> for 
encoding cp1250"). OTOH, I'm uneasy about invoking the entire PLy_elog 
machinery from a function that's as low-level as PLyUnicode_Bytes.

Lastly, we map SQL_ASCII to "ascii" which is arguably wrong. The 
function is supposed to return bytes in the server encoding, and under 
SQL_ASCII that probably means we can return anything (ie. use any 
encoding we deem useful). Using "ascii" as the Python codec name will 
raise an error on anything that has the high bit set.

So: I'd add code to translate WINxxx into CPxxx when choosing the Python 
to use, change PLy_elog to elog in PLyUnicode_Bytes and leave the 
SQL_ASCII case alone, as there were no complaints and people using 
SQL_ASCII are asking for it anyway.

Cheers,
Jan

On 29/06/12 00:36, Jan Urbański wrote:
> On 27/06/12 13:57, Jan Urbański wrote:
>> On 27/06/12 11:51, Asif Naeem wrote:
>>> Hi,
>>>
>>> On Windows 7 64bit, plpython is causing server crash with the following
>>> test case i.e.

> So: I'd add code to translate WINxxx into CPxxx when choosing the Python
> to use, change PLy_elog to elog in PLyUnicode_Bytes and leave the
> SQL_ASCII case alone, as there were no complaints and people using
> SQL_ASCII are asking for it anyway.

Since no one commented, I'll produce a patch to that effect. I believe 
this should go into 9.2 given that otherwise PL/Python will basically 
crash any database using the CP12xx encoding.

Cheers,
Jan

On 03/07/12 17:45, Jan Urbański wrote:
> On 29/06/12 00:36, Jan Urbański wrote:
>> On 27/06/12 13:57, Jan Urbański wrote:
>>> On 27/06/12 11:51, Asif Naeem wrote:
>>>> Hi,
>>>>
>>>> On Windows 7 64bit, plpython is causing server crash with the following
>>>> test case i.e.
>
>> So: I'd add code to translate WINxxx into CPxxx when choosing the Python
>> to use, change PLy_elog to elog in PLyUnicode_Bytes and leave the
>> SQL_ASCII case alone, as there were no complaints and people using
>> SQL_ASCII are asking for it anyway.
>
> Since no one commented, I'll produce a patch to that effect. I believe
> this should go into 9.2 given that otherwise PL/Python will basically
> crash any database using the CP12xx encoding.

Patch attached. Asif, could you try a few things on a CP1252 database?

First verify if your original test case now works and then try this:

create function enctest() returns text as $$
   return b'tr\xc3\xb3spido'.decode('utf-8')
$$ language plpython3u;

select enctest(), encode(convert_to(enctest(), 'utf-8'), 'hex');

Thanks,
Jan

On 04/07/12 13:58, Asif Naeem wrote:
>> Patch attached. Asif, could you try a few things on a CP1252 database?
>
> First verify if your original test case now works and then try this:
>>
>>
> I have test the patch on Win64. postgres server is working fine now for
> WIN1252. Thanks.
>
>
>> create function enctest() returns text as $$
>>    return b'tr\xc3\xb3spido'.decode('**utf-8')
>> $$ language plpython3u;
>>
>> select enctest(), encode(convert_to(enctest(), 'utf-8'), 'hex');
>>
>
> create function enctest() returns text as $$
>    return b'tr\xc3\xb3spido'.decode('utf-8')
> $$ language plpython3u;
> select enctest(), encode(convert_to(enctest(), 'utf-8'), 'hex');
>   enctest  |       encode
> ----------+--------------------
>   tróspido | 7472c3b3737069646f
> (1 row)
>
> Please do let me know If you have any other query. Thanks.

Great, this looks correct.

Can we apply this to 9.2?

Cheers,
Jan

On 04.07.2012 15:11, Jan Urbański wrote:
> On 04/07/12 13:58, Asif Naeem wrote:
>> I have test the patch on Win64. postgres server is working fine now for
>> WIN1252. Thanks.
>>
>>> create function enctest() returns text as $$
>>> return b'tr\xc3\xb3spido'.decode('**utf-8')
>>> $$ language plpython3u;
>>>
>>> select enctest(), encode(convert_to(enctest(), 'utf-8'), 'hex');
>>>
>>
>> create function enctest() returns text as $$
>> return b'tr\xc3\xb3spido'.decode('utf-8')
>> $$ language plpython3u;
>> select enctest(), encode(convert_to(enctest(), 'utf-8'), 'hex');
>> enctest | encode
>> ----------+--------------------
>> tróspido | 7472c3b3737069646f
>> (1 row)
>>
>> Please do let me know If you have any other query. Thanks.
>
> Great, this looks correct.
>
> Can we apply this to 9.2?

Committed. This bug was present in versions >= 9.0, so backpatched.

I used ereport() rather than elog() in the error message. Correct me if 
that was wrong, but the point was to avoid PLy_elog(), because that 
might cause recursion, and ereport() should be ok. I believe the message 
should be translated, as it's quite possible to get that error, at least 
if you use SQL_ASCII, so ereport() is more approriate than elog().

Thanks!

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

On 05/07/12 21:37, Heikki Linnakangas wrote:
> Committed. This bug was present in versions >= 9.0, so backpatched.

Thanks!

> I used ereport() rather than elog() in the error message. Correct me if
> that was wrong, but the point was to avoid PLy_elog(), because that
> might cause recursion, and ereport() should be ok. I believe the message
> should be translated, as it's quite possible to get that error, at least
> if you use SQL_ASCII, so ereport() is more approriate than elog().

Yes, you're absolutely right.

Cheers,
Jan