Re: proposal: PL/Pythonu - function ereport

2016-02-25 7:06 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

> 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.
>

merged

>
> 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.
>

fixed

>
> 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.
>

fixed

>
> get_string_attr seems to have the same issue as object_to_string.
>

use pstrdup, but the test on Py_None is necessary

PyObject_GetAttrString can returns Py_None, and 3.4's PyString_AsString
produce a error "ERROR: could not convert Python Unicode object to bytes"
when object is None.

So minimally in "get_string_attr" the test on Py_None is necessary

>
> 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.
>

has sense, removed

>
> 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.
>

renamed

>
> /* 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).
>

fixed

>
> 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.
>

fixed

>
> 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.
>

renamed to PLy_output only

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

It is not necessary, but I am thinking so it better - it is maybe too
defensive - but the message can be taken from more sources than in old
code, and one check on NULL is correct

> 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.
>

The previous code used Python memory for message. Now, I used PostgreSQL
memory (via pstrdup), so now I have to call FreeErrorData.

>
> 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.
>

I moved Py_XDECREF(so) up, and removed comment

>
> 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.
>

returned

>
> 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.
>

cleaned

>
>
> 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
>

done

Sending updated version

Regards

Pavel