Thread: Re: [HACKERS] statement level trigger causes pltcl, plpython SIGSEGV
Joe Conway wrote:
> I'll try to submit a patch later tonight or tomorrow morning if no one
> beats me to it.
>
Here's a patch for pltcl. It works for my test case:
CREATE OR REPLACE FUNCTION footrigfunc() RETURNS trigger AS 'return OK'
LANGUAGE pltcl;
CREATE TRIGGER footrig BEFORE INSERT OR UPDATE OR DELETE ON foo FOR EACH
STATEMENT EXECUTE PROCEDURE footrigfunc();
insert into foo values(11,'cat99',1.89);
delete from foo where f0 = 11;
I also ran the "runtest" script in pl/tcl/test -- it said:
**** Running test queries ****
Tests passed O.K.
Note that I also changed behavior in that when trigdata->tg_event
doesn't match anything known -- instead of pressing on with a value of
"UNKNOWN" it now throws an "elog(ERROR...". The previous behavior made
no sense to me, but you may not want to change existing behavior in this
way (even though it seems to me that it is a "should not happen" kind of
error).
If this patch is acceptable, I'll make similar changes to plpython. If
not, let me know what to change.
Thanks,
Joe
Index: src/pl/tcl/pltcl.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/pl/tcl/pltcl.c,v
retrieving revision 1.74
diff -c -r1.74 pltcl.c
*** src/pl/tcl/pltcl.c 4 Aug 2003 00:43:33 -0000 1.74
--- src/pl/tcl/pltcl.c 4 Aug 2003 05:36:12 -0000
***************
*** 708,770 ****
else if (TRIGGER_FIRED_AFTER(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "AFTER");
else
! Tcl_DStringAppendElement(&tcl_cmd, "UNKNOWN");
/* The level part of the event for TG_level */
if (TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "ROW");
else if (TRIGGER_FIRED_FOR_STATEMENT(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "STATEMENT");
- else
- Tcl_DStringAppendElement(&tcl_cmd, "UNKNOWN");
! /* Build the data list for the trigtuple */
! pltcl_build_tuple_argument(trigdata->tg_trigtuple,
! tupdesc, &tcl_trigtup);
!
! /*
! * Now the command part of the event for TG_op and data for NEW and
! * OLD
! */
! if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
! {
! Tcl_DStringAppendElement(&tcl_cmd, "INSERT");
- Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
Tcl_DStringAppendElement(&tcl_cmd, "");
-
- rettup = trigdata->tg_trigtuple;
- }
- else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
- {
- Tcl_DStringAppendElement(&tcl_cmd, "DELETE");
-
Tcl_DStringAppendElement(&tcl_cmd, "");
- Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
! rettup = trigdata->tg_trigtuple;
! }
! else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
! {
! Tcl_DStringAppendElement(&tcl_cmd, "UPDATE");
!
! pltcl_build_tuple_argument(trigdata->tg_newtuple,
! tupdesc, &tcl_newtup);
!
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_newtup));
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
!
! rettup = trigdata->tg_newtuple;
}
else
! {
! Tcl_DStringAppendElement(&tcl_cmd, "UNKNOWN");
!
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
!
! rettup = trigdata->tg_trigtuple;
! }
memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart));
Tcl_DStringFree(&tcl_trigtup);
--- 708,785 ----
else if (TRIGGER_FIRED_AFTER(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "AFTER");
else
! /* internal error */
! elog(ERROR, "unrecognized WHEN tg_event");
/* The level part of the event for TG_level */
if (TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
+ {
Tcl_DStringAppendElement(&tcl_cmd, "ROW");
+
+ /* Build the data list for the trigtuple */
+ pltcl_build_tuple_argument(trigdata->tg_trigtuple,
+ tupdesc, &tcl_trigtup);
+
+ /*
+ * Now the command part of the event for TG_op and data for NEW and
+ * OLD
+ */
+ if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
+ {
+ Tcl_DStringAppendElement(&tcl_cmd, "INSERT");
+
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
+ Tcl_DStringAppendElement(&tcl_cmd, "");
+
+ rettup = trigdata->tg_trigtuple;
+ }
+ else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
+ {
+ Tcl_DStringAppendElement(&tcl_cmd, "DELETE");
+
+ Tcl_DStringAppendElement(&tcl_cmd, "");
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
+
+ rettup = trigdata->tg_trigtuple;
+ }
+ else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
+ {
+ Tcl_DStringAppendElement(&tcl_cmd, "UPDATE");
+
+ pltcl_build_tuple_argument(trigdata->tg_newtuple,
+ tupdesc, &tcl_newtup);
+
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_newtup));
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
+
+ rettup = trigdata->tg_newtuple;
+ }
+ else
+ /* internal error */
+ elog(ERROR, "unrecognized OP tg_event");
+ }
else if (TRIGGER_FIRED_FOR_STATEMENT(trigdata->tg_event))
+ {
Tcl_DStringAppendElement(&tcl_cmd, "STATEMENT");
! if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
! Tcl_DStringAppendElement(&tcl_cmd, "INSERT");
! else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
! Tcl_DStringAppendElement(&tcl_cmd, "DELETE");
! else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
! Tcl_DStringAppendElement(&tcl_cmd, "UPDATE");
! else
! /* internal error */
! elog(ERROR, "unrecognized OP tg_event");
Tcl_DStringAppendElement(&tcl_cmd, "");
Tcl_DStringAppendElement(&tcl_cmd, "");
! rettup = (HeapTuple) NULL;
}
else
! /* internal error */
! elog(ERROR, "unrecognized LEVEL tg_event");
memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart));
Tcl_DStringFree(&tcl_trigtup);
Joe Conway <mail@joeconway.com> writes:
> Note that I also changed behavior in that when trigdata->tg_event
> doesn't match anything known -- instead of pressing on with a value of
> "UNKNOWN" it now throws an "elog(ERROR...". The previous behavior made
> no sense to me, but you may not want to change existing behavior in this
> way (even though it seems to me that it is a "should not happen" kind of
> error).
Seems reasonable to me. As a matter of style, the "unrecognized" elogs
should print the offending tg_event value; otherwise the patch looks fine.
> If this patch is acceptable, I'll make similar changes to plpython.
Please do.
regards, tom lane
Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>
>>Note that I also changed behavior in that when trigdata->tg_event
>>doesn't match anything known -- instead of pressing on with a value of
>>"UNKNOWN" it now throws an "elog(ERROR...". The previous behavior made
>>no sense to me, but you may not want to change existing behavior in this
>>way (even though it seems to me that it is a "should not happen" kind of
>>error).
>
> Seems reasonable to me. As a matter of style, the "unrecognized" elogs
> should print the offending tg_event value; otherwise the patch looks fine.
>
>>If this patch is acceptable, I'll make similar changes to plpython.
>
> Please do.
>
This patch includes pltcl and plpython, with the mentioned style issue
fixed. Both PLs pass their scripted tests and my simple statement level
trigger test.
Joe
Index: src/pl/plpython/plpython.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/pl/plpython/plpython.c,v
retrieving revision 1.38
diff -c -r1.38 plpython.c
*** src/pl/plpython/plpython.c 4 Aug 2003 01:57:58 -0000 1.38
--- src/pl/plpython/plpython.c 4 Aug 2003 17:32:55 -0000
***************
*** 612,618 ****
DECLARE_EXC();
TriggerData *tdata;
PyObject *pltname,
! *pltevent,
*pltwhen,
*pltlevel,
*pltrelid;
--- 612,618 ----
DECLARE_EXC();
TriggerData *tdata;
PyObject *pltname,
! *pltevent = NULL,
*pltwhen,
*pltlevel,
*pltrelid;
***************
*** 651,718 ****
Py_DECREF(pltrelid);
pfree(stroid);
-
-
if (TRIGGER_FIRED_BEFORE(tdata->tg_event))
pltwhen = PyString_FromString("BEFORE");
else if (TRIGGER_FIRED_AFTER(tdata->tg_event))
pltwhen = PyString_FromString("AFTER");
else
! pltwhen = PyString_FromString("UNKNOWN");
PyDict_SetItemString(pltdata, "when", pltwhen);
Py_DECREF(pltwhen);
if (TRIGGER_FIRED_FOR_ROW(tdata->tg_event))
pltlevel = PyString_FromString("ROW");
else if (TRIGGER_FIRED_FOR_STATEMENT(tdata->tg_event))
pltlevel = PyString_FromString("STATEMENT");
! else
! pltlevel = PyString_FromString("UNKNOWN");
! PyDict_SetItemString(pltdata, "level", pltlevel);
! Py_DECREF(pltlevel);
- if (TRIGGER_FIRED_BY_INSERT(tdata->tg_event))
- {
- pltevent = PyString_FromString("INSERT");
PyDict_SetItemString(pltdata, "old", Py_None);
- pytnew = PLyDict_FromTuple(&(proc->result), tdata->tg_trigtuple,
- tdata->tg_relation->rd_att);
- PyDict_SetItemString(pltdata, "new", pytnew);
- Py_DECREF(pytnew);
- *rv = tdata->tg_trigtuple;
- }
- else if (TRIGGER_FIRED_BY_DELETE(tdata->tg_event))
- {
- pltevent = PyString_FromString("DELETE");
PyDict_SetItemString(pltdata, "new", Py_None);
! pytold = PLyDict_FromTuple(&(proc->result), tdata->tg_trigtuple,
! tdata->tg_relation->rd_att);
! PyDict_SetItemString(pltdata, "old", pytold);
! Py_DECREF(pytold);
! *rv = tdata->tg_trigtuple;
! }
! else if (TRIGGER_FIRED_BY_UPDATE(tdata->tg_event))
! {
! pltevent = PyString_FromString("UPDATE");
! pytnew = PLyDict_FromTuple(&(proc->result), tdata->tg_newtuple,
! tdata->tg_relation->rd_att);
! PyDict_SetItemString(pltdata, "new", pytnew);
! Py_DECREF(pytnew);
! pytold = PLyDict_FromTuple(&(proc->result), tdata->tg_trigtuple,
! tdata->tg_relation->rd_att);
! PyDict_SetItemString(pltdata, "old", pytold);
! Py_DECREF(pytold);
! *rv = tdata->tg_newtuple;
}
else
! {
! pltevent = PyString_FromString("UNKNOWN");
! PyDict_SetItemString(pltdata, "old", Py_None);
! PyDict_SetItemString(pltdata, "new", Py_None);
! *rv = tdata->tg_trigtuple;
! }
! PyDict_SetItemString(pltdata, "event", pltevent);
! Py_DECREF(pltevent);
if (tdata->tg_trigger->tgnargs)
{
--- 651,741 ----
Py_DECREF(pltrelid);
pfree(stroid);
if (TRIGGER_FIRED_BEFORE(tdata->tg_event))
pltwhen = PyString_FromString("BEFORE");
else if (TRIGGER_FIRED_AFTER(tdata->tg_event))
pltwhen = PyString_FromString("AFTER");
else
! /* internal error */
! elog(ERROR, "unrecognized WHEN tg_event: %u", tdata->tg_event);
PyDict_SetItemString(pltdata, "when", pltwhen);
Py_DECREF(pltwhen);
if (TRIGGER_FIRED_FOR_ROW(tdata->tg_event))
+ {
pltlevel = PyString_FromString("ROW");
+ PyDict_SetItemString(pltdata, "level", pltlevel);
+ Py_DECREF(pltlevel);
+
+ if (TRIGGER_FIRED_BY_INSERT(tdata->tg_event))
+ {
+ pltevent = PyString_FromString("INSERT");
+
+ PyDict_SetItemString(pltdata, "old", Py_None);
+ pytnew = PLyDict_FromTuple(&(proc->result), tdata->tg_trigtuple,
+ tdata->tg_relation->rd_att);
+ PyDict_SetItemString(pltdata, "new", pytnew);
+ Py_DECREF(pytnew);
+ *rv = tdata->tg_trigtuple;
+ }
+ else if (TRIGGER_FIRED_BY_DELETE(tdata->tg_event))
+ {
+ pltevent = PyString_FromString("DELETE");
+
+ PyDict_SetItemString(pltdata, "new", Py_None);
+ pytold = PLyDict_FromTuple(&(proc->result), tdata->tg_trigtuple,
+ tdata->tg_relation->rd_att);
+ PyDict_SetItemString(pltdata, "old", pytold);
+ Py_DECREF(pytold);
+ *rv = tdata->tg_trigtuple;
+ }
+ else if (TRIGGER_FIRED_BY_UPDATE(tdata->tg_event))
+ {
+ pltevent = PyString_FromString("UPDATE");
+
+ pytnew = PLyDict_FromTuple(&(proc->result), tdata->tg_newtuple,
+ tdata->tg_relation->rd_att);
+ PyDict_SetItemString(pltdata, "new", pytnew);
+ Py_DECREF(pytnew);
+ pytold = PLyDict_FromTuple(&(proc->result), tdata->tg_trigtuple,
+ tdata->tg_relation->rd_att);
+ PyDict_SetItemString(pltdata, "old", pytold);
+ Py_DECREF(pytold);
+ *rv = tdata->tg_newtuple;
+ }
+ else
+ /* internal error */
+ elog(ERROR, "unrecognized OP tg_event: %u", tdata->tg_event);
+
+ PyDict_SetItemString(pltdata, "event", pltevent);
+ Py_DECREF(pltevent);
+ }
else if (TRIGGER_FIRED_FOR_STATEMENT(tdata->tg_event))
+ {
pltlevel = PyString_FromString("STATEMENT");
! PyDict_SetItemString(pltdata, "level", pltlevel);
! Py_DECREF(pltlevel);
PyDict_SetItemString(pltdata, "old", Py_None);
PyDict_SetItemString(pltdata, "new", Py_None);
! *rv = (HeapTuple) NULL;
!
! if (TRIGGER_FIRED_BY_INSERT(tdata->tg_event))
! pltevent = PyString_FromString("INSERT");
! else if (TRIGGER_FIRED_BY_DELETE(tdata->tg_event))
! pltevent = PyString_FromString("DELETE");
! else if (TRIGGER_FIRED_BY_UPDATE(tdata->tg_event))
! pltevent = PyString_FromString("UPDATE");
! else
! /* internal error */
! elog(ERROR, "unrecognized OP tg_event: %u", tdata->tg_event);
!
! PyDict_SetItemString(pltdata, "event", pltevent);
! Py_DECREF(pltevent);
}
else
! /* internal error */
! elog(ERROR, "unrecognized LEVEL tg_event: %u", tdata->tg_event);
if (tdata->tg_trigger->tgnargs)
{
Index: src/pl/tcl/pltcl.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/pl/tcl/pltcl.c,v
retrieving revision 1.74
diff -c -r1.74 pltcl.c
*** src/pl/tcl/pltcl.c 4 Aug 2003 00:43:33 -0000 1.74
--- src/pl/tcl/pltcl.c 4 Aug 2003 17:07:24 -0000
***************
*** 708,770 ****
else if (TRIGGER_FIRED_AFTER(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "AFTER");
else
! Tcl_DStringAppendElement(&tcl_cmd, "UNKNOWN");
/* The level part of the event for TG_level */
if (TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "ROW");
else if (TRIGGER_FIRED_FOR_STATEMENT(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "STATEMENT");
- else
- Tcl_DStringAppendElement(&tcl_cmd, "UNKNOWN");
! /* Build the data list for the trigtuple */
! pltcl_build_tuple_argument(trigdata->tg_trigtuple,
! tupdesc, &tcl_trigtup);
!
! /*
! * Now the command part of the event for TG_op and data for NEW and
! * OLD
! */
! if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
! {
! Tcl_DStringAppendElement(&tcl_cmd, "INSERT");
- Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
Tcl_DStringAppendElement(&tcl_cmd, "");
-
- rettup = trigdata->tg_trigtuple;
- }
- else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
- {
- Tcl_DStringAppendElement(&tcl_cmd, "DELETE");
-
Tcl_DStringAppendElement(&tcl_cmd, "");
- Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
! rettup = trigdata->tg_trigtuple;
! }
! else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
! {
! Tcl_DStringAppendElement(&tcl_cmd, "UPDATE");
!
! pltcl_build_tuple_argument(trigdata->tg_newtuple,
! tupdesc, &tcl_newtup);
!
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_newtup));
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
!
! rettup = trigdata->tg_newtuple;
}
else
! {
! Tcl_DStringAppendElement(&tcl_cmd, "UNKNOWN");
!
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
! Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
!
! rettup = trigdata->tg_trigtuple;
! }
memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart));
Tcl_DStringFree(&tcl_trigtup);
--- 708,785 ----
else if (TRIGGER_FIRED_AFTER(trigdata->tg_event))
Tcl_DStringAppendElement(&tcl_cmd, "AFTER");
else
! /* internal error */
! elog(ERROR, "unrecognized WHEN tg_event: %u", trigdata->tg_event);
/* The level part of the event for TG_level */
if (TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
+ {
Tcl_DStringAppendElement(&tcl_cmd, "ROW");
+
+ /* Build the data list for the trigtuple */
+ pltcl_build_tuple_argument(trigdata->tg_trigtuple,
+ tupdesc, &tcl_trigtup);
+
+ /*
+ * Now the command part of the event for TG_op and data for NEW and
+ * OLD
+ */
+ if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
+ {
+ Tcl_DStringAppendElement(&tcl_cmd, "INSERT");
+
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
+ Tcl_DStringAppendElement(&tcl_cmd, "");
+
+ rettup = trigdata->tg_trigtuple;
+ }
+ else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
+ {
+ Tcl_DStringAppendElement(&tcl_cmd, "DELETE");
+
+ Tcl_DStringAppendElement(&tcl_cmd, "");
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
+
+ rettup = trigdata->tg_trigtuple;
+ }
+ else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
+ {
+ Tcl_DStringAppendElement(&tcl_cmd, "UPDATE");
+
+ pltcl_build_tuple_argument(trigdata->tg_newtuple,
+ tupdesc, &tcl_newtup);
+
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_newtup));
+ Tcl_DStringAppendElement(&tcl_cmd, Tcl_DStringValue(&tcl_trigtup));
+
+ rettup = trigdata->tg_newtuple;
+ }
+ else
+ /* internal error */
+ elog(ERROR, "unrecognized OP tg_event: %u", trigdata->tg_event);
+ }
else if (TRIGGER_FIRED_FOR_STATEMENT(trigdata->tg_event))
+ {
Tcl_DStringAppendElement(&tcl_cmd, "STATEMENT");
! if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
! Tcl_DStringAppendElement(&tcl_cmd, "INSERT");
! else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
! Tcl_DStringAppendElement(&tcl_cmd, "DELETE");
! else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
! Tcl_DStringAppendElement(&tcl_cmd, "UPDATE");
! else
! /* internal error */
! elog(ERROR, "unrecognized OP tg_event: %u", trigdata->tg_event);
Tcl_DStringAppendElement(&tcl_cmd, "");
Tcl_DStringAppendElement(&tcl_cmd, "");
! rettup = (HeapTuple) NULL;
}
else
! /* internal error */
! elog(ERROR, "unrecognized LEVEL tg_event: %u", trigdata->tg_event);
memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart));
Tcl_DStringFree(&tcl_trigtup);
Joe Conway <mail@joeconway.com> writes:
> This patch includes pltcl and plpython, with the mentioned style issue
> fixed. Both PLs pass their scripted tests and my simple statement level
> trigger test.
Applied, thanks.
BTW, one other stylistic nit: I don't think comments like
/* internal error */
elog(ERROR, "unrecognized OP tg_event: %u", tdata->tg_event);
are really necessary. In the brave new ereport world, any elog(ERROR)
call is an internal error by definition --- if it isn't, you should be
using ereport. So the use of elog is sufficient documentation. IMHO
anyway.
regards, tom lane
Tom Lane wrote: > BTW, one other stylistic nit: I don't think comments like > > /* internal error */ > elog(ERROR, "unrecognized OP tg_event: %u", tdata->tg_event); > > are really necessary. In the brave new ereport world, any elog(ERROR) > call is an internal error by definition --- if it isn't, you should be > using ereport. So the use of elog is sufficient documentation. IMHO > anyway. Yeah, I agree. During the conversion, I did that mostly to show that it was a conscious decision. Going forward, every new elog (versus ereport) *is* a conscious decision, or at least should be. Joe