Re: proposal: PL/Pythonu - function ereport

On Fri, Feb 19, 2016 at 9:41 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> It looks like good idea. Last version are not breaking compatibility - and
> I think so it can works.
>
> I wrote the code, that works on Python2 and Python3

Hi,

I've attached a patch on top of yours with some documentation
improvements, feel free to fold it in your next version.

I think the concept is good. We're down to code level changes. Most of
them are cosmetical, misleading comments and so on but there are some
bugs in there as far as I see.

In object_to_string you don't need to test for Py_None. PyObject_Str
will return NULL on failure and whatever str() returns on the
underlying object. No need to special case None.

In object_to_string, when you're already in a (so != NULL) block, you
can use Py_DECREF instead of XDECREF.

object_to_string seems buggy to me: it returns the pointer returned by
PyString_AsString which points to the internal buffer of the Python
object but it also XDECREFs that object. You seem to be returning a
pointer to freed space.

get_string_attr seems to have the same issue as object_to_string.

In PLy_get_error_data query and position will never be set for
plpy.Error since you don't offer them to Python and therefore don't
set them in PLy_output_kw. So I think you should remove the code
related to them, including the char **query, int *position parameters
for PLy_get_error_data. Removing position also allows removing
get_int_attr and the need to exercise this function in the tests.

You're using PLy_get_spi_sqlerrcode in PLy_get_error_data which makes
the spi in the name unsuitable. I would rename it to just
PLy_get_sqlerrcode. At least the comment at the top of
PLy_get_spi_sqlerrcode needs to change since it now also extracts info
from Error not just SPIError.

/* set value of string pointer on object field */ comments are weird
for a function that gets a value. But those functions need to change
anyway since they're buggy (see above).

The only change in plpy_plpymodule.h is the removal of an empty line
unrelated to this patch, probably from previous versions. You should
undo it to leave plpy_plpymodule.h untouched.

Why rename PLy_output to PLy_output_kw? It only makes the patch bigger
without being an improvement. Maybe you also have this from previous
versions.

In PLy_output_kw you don't need to check message for NULL, just as sv
wasn't checked before.

In PLy_output_kw you added a FreeErrorData(edata) which didn't exist
before. I'm not familiar with that API but I'm wondering if it's
needed or not, good to have it or not etc.

In PLy_output_kw you didn't update the "Note: If sv came from
PyString_AsString(), it points into storage..." comment which still
refers to sv which is now deleted.

In PLy_output_kw you removed the "return a legal object so the
interpreter will continue on its merry way" comment which might not be
the most valuable comment in the world but removing it is still
unrelated to this patch.

In PLy_output_kw you test for kw != NULL && kw != Py_None. The kw !=
NULL is definitely needed but I'm quite sure Python will never pass
Py_None into it so the != Py_None isn't needed. Can't find a reference
to prove this at the moment.

Some more tests could be added to exercise more parts of the patch:
- check that SPIError is enhanced with all the new things:
schema_name, table_name etc.
- in the plpy.error call use every keyword argument not just detail
and hint: it proves you save and restore every field correctly from
the Error fields since that's not exercised by the info call above
which does use every argument
- use a wrong keyword argument to see it gets rejected with you error message
- try some other types than str (like detail=42) for error as well
since error goes on another code path than info
- a test exercising the "invalid sqlstate" code