> On 12/01/26 09:24, Peter Eisentraut wrote:
> >> 0001, 0003 and 0004 looks good to me, just a small comment on 0002:
> >>
> >> - /*
> >> - * PyModule_AddObject does not add a refcount to the object, for
> >> some odd
> >> - * reason; we must do that.
> >> - */
> >> - Py_INCREF(exc);
> >> - PyModule_AddObject(mod, modname, exc);
> >> -
> >> /*
> >> * The caller will also store a pointer to the exception object
> >> in some
> >> - * permanent variable, so add another ref to account for that.
> >> This is
> >> - * probably excessively paranoid, but let's be sure.
> >> + * permanent variable, so add another ref to account for that.
> >> */
> >> Py_INCREF(exc);
> >>
> >> The later code comment say "so add another ref to account for that",
> >> but you've removed the previous Py_INCREF() call. The returned object
> >> PyErr_NewException() already have a refcount increased for usage? If
> >> that's not the case I think that the "add another ref..." don't seems
> >> correct because IIUC we are increasing the ref count for the first
> >> time, so there is no "another" refcount being increased.
> >
> > The reference created by PyErr_NewException() is "stolen" by
> > PyModule_AddObject(), so we need to create another one for returning
> > the object from the function and storing it in the permanent variable.
> > I have updated the comment in this new patch version. But I think the
> > actual code is correct.
>
> Thanks, it looks more clear IMHO now. Agree that the code is correct.
>
>
> --
> Matheus Alcantara
> EDB: https://www.enterprisedb.com
Hi Peter,
I have applied and reviewed the v2 patch set. The cleanup of the initialization
flow is a very good improvement and makes the logic much easier to follow.
In particular, the updated comments in PLy_create_exception regarding the
reference counting logic are very helpful and clarify the previous ambiguity.
I have one minor nitpick in plpy_main.c regarding consistency. In _PG_init(),
the import and dictionary insertion of the "plpy" module is currently split
into two checks with the same error message. To make it more consistent with
how the __main__ module is handled earlier in the same function, we could
potentially streamline it like this:
/*
* Import plpy.
*/
plpy_mod = PyImport_ImportModule("plpy");
if (plpy_mod == NULL || PyDict_SetItemString(main_dict, "plpy",
plpy_mod) < 0)
PLy_elog(ERROR, "could not import \"plpy\" module");
This is just a small suggestion for style consistency; the existing logic in v2
is perfectly correct.
Regards,
Yuan Li(carol)
