Re: pl/python refactoring

On 18/01/11 23:22, Peter Eisentraut wrote:
> On mån, 2011-01-17 at 21:49 +0200, Peter Eisentraut wrote:
>> On sön, 2011-01-02 at 12:41 +0100, Jan Urbański wrote:
>>> Here they are. There are 16 patches in total. They amount to what's
>>> currently in my refactor branch (and almost to what I've sent as the
>>> complete refactor patch, while doing the splitting I discovered a few
>>> useless hunks that I've discarded).
>>
>> Committed 0001, rest later.
>
> Today committed: 3, 5, 10, 11, 12, 13

Thanks,

> #2: It looks like this loses some information/formatting in the error
> message. Should we keep the pointing arrow there?
>
> --- a/src/pl/plpython/expected/plpython_error.out
> +++ b/src/pl/plpython/expected/plpython_error.out
> @@ -10,10 +10,7 @@ CREATE FUNCTION sql_syntax_error() RETURNS text
> SELECT sql_syntax_error();
> WARNING: PL/Python: plpy.SPIError: unrecognized error in PLy_spi_execute_query
> CONTEXT: PL/Python function "sql_syntax_error"
> -ERROR: syntax error at or near "syntax"
> -LINE 1: syntax error
> - ^
> -QUERY: syntax error
> +ERROR: PL/Python: plpy.SPIError: syntax error at or near "syntax"
> CONTEXT: PL/Python function "sql_syntax_error"
> /* check the handling of uncaught python exceptions
> */

Yes, the message is less informative, because the error is reported by
PL/Python, by wrapping the SPI message. I guess I could try to extract
more info from the caught ErrorData and put it in the new error that
PL/Python throws. But I'd like to do that as a refinement of the
spi-in-subxacts patch, because the current behaviour is broken anyway -
to catch the SPI error safely you need to wrap the whole thing in a
subtransaction. This patch only gets rid of the global error state, and
to do that I needed to raise some exception from the try/catch block
around SPI calls.

> #7: This is unnecessary because the remaining fields are automatically
> initialized with NULL. The Python documentation uses initializations
> like that. The way I understand it, newer Python versions might add
> more fields at the end, and they will rely on the fact that they get
> automatically initialized even if the source code was for an older
> version. So I would rather not touch this, as it doesn't buy anything.

OK.

> #16: This is probably pointless because pgindent will reformat this.

If it does, than it's a shame. Maybe the comment should be moved above
the if() then.

Cheers,
Jan