Re: [plpython] Add missing volatile qualifier.

Поиск
Список
Период
Сортировка
Искать

Re: [plpython] Add missing volatile qualifier.

От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
Xing Guo  writes:
> I'm playing a toy static analysis checker with PostgreSQL and found a
> variable is missing volatile qualifier.

Good catch!  It looks like the consequences of a failure would be
pretty minimal --- AFAICS, no worse than a possible failure to remove
a refcount on Py_None --- but that's still a bug.

I don't care for your proposed fix though.  I think the real problem
here is schizophrenia about when to set up pltargs, and we could
fix it more nicely as attached.  (Perhaps the Asserts are overkill
though.)

			regards, tom lane

diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index e06fde1dd9..3145c69699 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -689,7 +689,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			   *pltrelid,
 			   *plttablename,
 			   *plttableschema,
-			   *pltargs = NULL,
+			   *pltargs,
 			   *pytnew,
 			   *pytold,
 			   *pltdata;
@@ -713,6 +713,11 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			return NULL;
 		}
 	}
+	else
+	{
+		Py_INCREF(Py_None);
+		pltargs = Py_None;
+	}
 
 	PG_TRY();
 	{
@@ -856,7 +861,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			PyObject   *pltarg;
 
 			/* pltargs should have been allocated before the PG_TRY block. */
-			Assert(pltargs);
+			Assert(pltargs && pltargs != Py_None);
 
 			for (i = 0; i < tdata->tg_trigger->tgnargs; i++)
 			{
@@ -870,8 +875,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 		}
 		else
 		{
-			Py_INCREF(Py_None);
-			pltargs = Py_None;
+			Assert(pltargs == Py_None);
 		}
 		PyDict_SetItemString(pltdata, "args", pltargs);
 		Py_DECREF(pltargs);

Re: [plpython] Add missing volatile qualifier.

От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
Nathan Bossart  writes:
> On Mon, Apr 01, 2024 at 11:57:07AM -0400, Tom Lane wrote:
>> Good catch!  It looks like the consequences of a failure would be
>> pretty minimal --- AFAICS, no worse than a possible failure to remove
>> a refcount on Py_None --- but that's still a bug.

> Huh.  I seem to have dropped that "volatile" shortly before committing for
> some reason [0].

Oh, I'd forgotten that discussion.  Given that we were both confused
about the need for it, all the more reason to try to avoid using a
within-PG_TRY assignment.

> Your fix seems fine to me.

Thanks for looking, I'll push it shortly.

			regards, tom lane

Re: [plpython] Add missing volatile qualifier.

От:
Nathan Bossart <nathandbossart@gmail.com>
Дата:
On Mon, Apr 01, 2024 at 11:57:07AM -0400, Tom Lane wrote:
> Xing Guo  writes:
>> I'm playing a toy static analysis checker with PostgreSQL and found a
>> variable is missing volatile qualifier.
> 
> Good catch!  It looks like the consequences of a failure would be
> pretty minimal --- AFAICS, no worse than a possible failure to remove
> a refcount on Py_None --- but that's still a bug.

Huh.  I seem to have dropped that "volatile" shortly before committing for
some reason [0].

> I don't care for your proposed fix though.  I think the real problem
> here is schizophrenia about when to set up pltargs, and we could
> fix it more nicely as attached.  (Perhaps the Asserts are overkill
> though.)

Your fix seems fine to me.

[0] https://postgr.es/m/20230504234235.GA2419591%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

[MASSMAIL][plpython] Add missing volatile qualifier.

От:
Xing Guo <higuoxing@gmail.com>
Дата:
Hi hackers,

I'm playing a toy static analysis checker with PostgreSQL and found a
variable is missing volatile qualifier.

Best Regards,
Xing
FAQ