Re: [PATCH] Fix conversion for Decimal arguments in plpython functions

It seems like you confused me with steve :)

The patch applies cleanly, and the regression tests pass on python2 when
cdecimal is not installed. When it is, the type info returned for the
converted numeric value is cdecimal.Decimal instead of decimal.Decimal.

The regression tests expected output have not been modified for python3,
and as such they fail on the type conversions.

I am a bit confused with the use of PyModule_GetDict: shouldn't
PyObj_GetAttrString be used directly instead ? Moreover, the reference
count in the current implementation might be off: the reference count for
the decimal module is never decreased, while the reference count to the
module dict is, when the docs say it returns a borrowed reference.

Please find a patch that fixes both issues.

2013/6/26 Szymon Guz <mabewlun(at)gmail(dot)com>

> Thanks Steve, that's exactly what I wanted to send when you sent your
> patches :)
>
> I need to figure out why that patch v2 worked for me, I think I made mess
> somewhere in my git repo and didn't create the patch properly. Sorry for
> that.
>
> Patch is attached, I've also added information about cdecimal to plpython
> documentation.
>
> I'm just wondering how to make integration tests to check when cdecimal is
> installed and when it is not.
>
> thanks,
> Szymon
>
>
> On 26 June 2013 10:12, Ronan Dunklau <rdunklau(at)gmail(dot)com> wrote:
>
>> The v2 patch does not work for me: regression tests for plpython fail on
>> the plpython_types test: every numeric is converted to None.
>>
>> It seems the global decimal ctor is not initialized.
>>
>> Please find two patches, to be applied on top of the v2 patch: one
>> initializes the decimal ctor, the other uses cdecimal when possible.
>>
>> Using the performance test from steve, on my machine:
>>
>> - with cdecimal installed: ~84ms
>> - without cdecimal installed (standard decimal module): ~511ms
>>
>>
>> 2013/6/26 Szymon Guz <mabewlun(at)gmail(dot)com>
>>
>>> On 26 June 2013 01:40, Steve Singer <steve(at)ssinger(dot)info> wrote:
>>>
>>>> On 06/25/2013 06:42 AM, Szymon Guz wrote:
>>>>
>>>>
>>>>
>>>>> Hi,
>>>>>
>>>>> I've attached a new patch. I've fixed all the problems you've found,
>>>>> except for the efficiency problem, which has been described in previous
>>>>> email.
>>>>>
>>>>> thanks,
>>>>> Szymon
>>>>>
>>>>>
>>>> This version of the patch addresses the issues I mentioned. Thanks for
>>>> looking into seeing if the performance issue is with our conversions to
>>>> strings or inherit with the python decimal type. I guess we (Postgresql)
>>>> can't do much about it. A runtime switch to use cdecimal if it is
>>>> available is a good idea, but I agree with you that could be a different
>>>> patch.
>>>>
>>>> One minor thing I noticed in this round,
>>>>
>>>> PLy_elog(ERROR, "could not import module 'decimal'");
>>>>
>>>> I think should have "decimal" in double-quotes.
>>>>
>>>> I think this patch is ready for a committer to look at it.
>>>>
>>>> Steve
>>>>
>>>>
>>>>>
>>>>
>>> Hi Steve,
>>> thanks for the review.
>>>
>>> I was thinking about speeding up the Decimal conversion using the module
>>> you wrote about. What about trying to import it, if it fails, than trying
>>> to load decimal.Decimal? There will be no warning in logs, just additional
>>> information in documentation that it uses this module if it is available?
>>>
>>> thanks,
>>> Szymon
>>>
>>
>>
>