Re: Python 2.7 deprecated the PyCObject API?

According to a discussion over in Fedora-land, $subject is true:
http://lists.fedoraproject.org/pipermail/devel/2010-August/140995.html

I see several calls in plpython.c that seem to refer to PyCObject stuff.
Anybody have any idea if we need to do something about this?

regards, tom lane

On Aug 13, 2010, at 5:20 PM, Tom Lane wrote:
> According to a discussion over in Fedora-land, $subject is true:
> http://lists.fedoraproject.org/pipermail/devel/2010-August/140995.html
>
> I see several calls in plpython.c that seem to refer to PyCObject stuff.
> Anybody have any idea if we need to do something about this?

Well, we should at least be checking for an exception here anyways:

proc->me = PyCObject_FromVoidPtr(proc, NULL);
PyDict_SetItemString(PLy_procedure_cache, key, proc->me);

if (proc->me == NULL) complain();

That is, with those warnings adjustments, proc->me will be NULL and then explode in PyDict_SetItemString:

[David Malcolm]
However, if someone overrides the process-wide warnings settings, then
the API can fail altogether, raising a PendingDeprecationWarning
exception (which in CPython terms means setting a thread-specific error
state and returning NULL).
./

AFA a better fix is concerned, the shortest route would seem to be to use the new capsule stuff iff Python >= 2.7.

James William Pye <lists(at)jwp(dot)name> writes:
> On Aug 13, 2010, at 5:20 PM, Tom Lane wrote:
>> I see several calls in plpython.c that seem to refer to PyCObject stuff.
>> Anybody have any idea if we need to do something about this?

> Well, we should at least be checking for an exception here anyways:

> proc->me = PyCObject_FromVoidPtr(proc, NULL);
> PyDict_SetItemString(PLy_procedure_cache, key, proc->me);

> if (proc->me == NULL) complain();

Just to clarify, you're recommending something like

proc->me = PyCObject_FromVoidPtr(proc, NULL);
+ if (proc->me == NULL)
+ elog(ERROR, "could not create PyCObject for function");
PyDict_SetItemString(PLy_procedure_cache, key, proc->me);

correct? (Hm, and it looks like we'd better move the pfree just above that...)

> AFA a better fix is concerned, the shortest route would seem to be to
> use the new capsule stuff iff Python >= 2.7.

Yeah, and since we'll have to back-patch it, a fairly noninvasive patch
would be nice. Will you work on that?

regards, tom lane

On Aug 14, 2010, at 9:08 AM, Tom Lane wrote:
> Just to clarify, you're recommending something like
>
> proc->me = PyCObject_FromVoidPtr(proc, NULL);
> + if (proc->me == NULL)
> + elog(ERROR, "could not create PyCObject for function");
> PyDict_SetItemString(PLy_procedure_cache, key, proc->me);
>
> correct? (Hm, and it looks like we'd better move the pfree just above that...)

Almost, there's still a Python exception to report and/or clear.
I only glanced at this and didn't recall what the plpython mechanisms were for that, thus the ambiguous "complain()".

> Yeah, and since we'll have to back-patch it, a fairly noninvasive patch
> would be nice. Will you work on that?

I was hoping that Peter would pop in with a patch, but I think a few lines of CPP may suffice..
(warning: untested =)

#ifdef Py_CAPSULE_H
/*
* Python.h (2.7 and up) includes pycapsule.h, so rely on the header
* define to detect the API's existence.
*/
#define PyCObject_FromVoidPtr(POINTER, IGNORED) PyCapsule_New(POINTER, NULL, NULL)
#undef PyCObject_Check
#define PyCObject_Check(OBJ) PyCapsule_CheckExact(OBJ)
#define PyCObject_AsVoidPtr(OBJ) PyCapsule_GetPointer(OBJ, NULL)
#endif /* Py_CAPSULE_H */

http://svn.python.org/view/python/branches/release27-maint/Include/pycapsule.h?view=markup

yay? nay?

James William Pye <lists(at)jwp(dot)name> writes:
> On Aug 14, 2010, at 9:08 AM, Tom Lane wrote:
>> Just to clarify, you're recommending something like
>>
>> proc->me = PyCObject_FromVoidPtr(proc, NULL);
>> + if (proc->me == NULL)
>> + elog(ERROR, "could not create PyCObject for function");
>> PyDict_SetItemString(PLy_procedure_cache, key, proc->me);
>>
>> correct? (Hm, and it looks like we'd better move the pfree just above that...)

> Almost, there's still a Python exception to report and/or clear.

Ah, right, I guess that should be PLy_elog() not just elog().

>> Yeah, and since we'll have to back-patch it, a fairly noninvasive patch
>> would be nice. Will you work on that?

> I was hoping that Peter would pop in with a patch, but I think a few lines of CPP may suffice..
> ...
> yay? nay?

Damifino, I don't hack Python. Peter?

regards, tom lane

On fre, 2010-08-13 at 20:20 -0400, Tom Lane wrote:
> According to a discussion over in Fedora-land, $subject is true:
> http://lists.fedoraproject.org/pipermail/devel/2010-August/140995.html
>
> I see several calls in plpython.c that seem to refer to PyCObject
> stuff.
> Anybody have any idea if we need to do something about this?

Some exception handling might be good, but I think we don't need to
abandon the API yet. It's only "pending deprecation".

On tis, 2010-08-17 at 20:55 +0300, Peter Eisentraut wrote:
> On fre, 2010-08-13 at 20:20 -0400, Tom Lane wrote:
> > According to a discussion over in Fedora-land, $subject is true:
> > http://lists.fedoraproject.org/pipermail/devel/2010-August/140995.html
> >
> > I see several calls in plpython.c that seem to refer to PyCObject
> > stuff.
> > Anybody have any idea if we need to do something about this?
>
> Some exception handling might be good, but I think we don't need to
> abandon the API yet. It's only "pending deprecation".

Here is a patch. The crash is reproducible, if warnings are turned into
errors.

On tis, 2010-08-17 at 21:48 +0300, Peter Eisentraut wrote:
> On tis, 2010-08-17 at 20:55 +0300, Peter Eisentraut wrote:
> > On fre, 2010-08-13 at 20:20 -0400, Tom Lane wrote:
> > > According to a discussion over in Fedora-land, $subject is true:
> > > http://lists.fedoraproject.org/pipermail/devel/2010-August/140995.html
> > >
> > > I see several calls in plpython.c that seem to refer to PyCObject
> > > stuff.
> > > Anybody have any idea if we need to do something about this?
> >
> > Some exception handling might be good, but I think we don't need to
> > abandon the API yet. It's only "pending deprecation".
>
> Here is a patch. The crash is reproducible, if warnings are turned into
> errors.

I have applied this patch back to 8.0. 7.4's plpython crashes with
Python >= 2.5; it's probably not worth rescuing.