Re: proposal: PL/Pythonu - function ereport

2015-11-02 17:01 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

> Hello,
>
> Here's a detailed review:
>
> 1. in PLy_spi_error__init__ you need to check kw for NULL before doing
> PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
> call because PyDict_Size expects a real dictionary not NULL
>

PyDict_Size returns -1 when the dictionary is NULL

http://grokbase.com/t/python/python-dev/042ft6qaqq/pydict-size-error-return

done

>
> 2. a test with just plpy.SPIError() is still missing, this would have
> caught 1.
>

please, can you write some example - I am not able raise described error

>
> 3. the tests have "possibility to set all accessable fields in custom
> exception" above a test that doesn't set all fields, it's confusing
> (and accessible is spelled wrong)
>
>
done

> 4. in the docs, "The constructor of SPIError exception (class)
> supports keyword parameters." sounds better as "An SPIError instance
> can be constructed using keyword parameters."
>

done

>
> 5. there is conceptual code duplication between PLy_spi_exception_set
> in plpy_spi.c, since that code also constructs an SPIError but from C
> and with more info available (edata->internalquery and
> edata->internalpos). But making a tuple and assigning it to spidata is
> in both. Not sure how this can be improved.
>

I see it, but I don't think, so current code should be changed.
PLy_spi_exception_set is controlled directly by fully filled ErrorData
structure, __init__ is based only on keyword parameters with possibility
use inherited data. If I'll build ErrorData in __init__ function and call
PLy_spi_exception_set, then the code will be longer and more complex.

>
> 6. __init__ can use METH_VARARGS | METH_KEYWORDS in both Python2 and
> 3, no need for the #if
>
>
done

> 7. "could not create dictionary for SPI exception" would be more
> precise as "could not create dictionary for SPIError" right?
>

done

>
> 8. in PLy_add_methods_to_dictionary I would avoid import since it
> makes one think of importing modules; maybe "could not create
> functionwrapper for \"%s\"", "could not create method wrapper for \"%s\""

done

>
> 9. also in PLy_add_methods_to_dictionary "could public method \"%s\"
> in dictionary" is not proper English and is missing not, maybe "could
> not add method \"%s\" to class dictionary"?
>

done

>
> 10. in PLy_add_methods_to_dictionary if PyCFunction_New succeeds but
> PyMethod_New fails, func will leak
>

done

>
> 11. it would be nice to have a test for the invalid SQLSTATE code part
>

done

>
> 12. similar to 10, in PLy_spi_error__init__ taking the "invalid
> SQLSTATE" branch leaks exc_args, in general early returns via PLy_elog
> will leave the intermediate Python objects leaking
>

dome

>
>
> Will mark the patch as Waiting for author.
>

attached new update

Regards

Pavel