From: Tom Lane <tgl@sss.pgh.pa.us>
To: Erik Wienhold <ewie@ewie.name>
cc: Pierre Forstmann <pierre.forstmann@gmail.com>,
        "a.kozhemyakin" <a.kozhemyakin@postgrespro.ru>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: pltcl crashes due to a syntax error
In-reply-to: <309214c5-d4da-4e00-938d-0e35990e564c@ewie.name>
References: <6a2a1c40-2b2c-4a33-8b72-243c0766fcda@postgrespro.ru> <CAM-sOH9zC5VeycS0BFCV4CAEfDL4OLEh_41oWtVLfbOZCtxNFw@mail.gmail.com> <5af13487-7722-4bdd-bad3-939598c8dd4e@ewie.name> <2365308.1717366548@sss.pgh.pa.us> <309214c5-d4da-4e00-938d-0e35990e564c@ewie.name>
Comments: In-reply-to Erik Wienhold <ewie@ewie.name>
	message dated "Mon, 03 Jun 2024 18:16:48 +0200"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <73222.1717433844.0@sss.pgh.pa.us>
Date: Mon, 03 Jun 2024 12:57:49 -0400
Message-ID: <73249.1717433869@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/73249.1717433869%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <73222.1717433844.1@sss.pgh.pa.us>

Erik Wienhold <ewie@ewie.name> writes:
> On 2024-06-03 00:15 +0200, Tom Lane wrote:
>> The new bit of information that this bug report provides is that it's
>> possible to get a TCL_ERROR result without Tcl having set errorInfo.
>> That seems a tad odd, and it must happen only in weird corner cases,
>> else we'd have heard of this decades ago.  Not sure if it's worth
>> trying to characterize those cases further, however.

> ISTM that errorInfo is set automatically only during script evaluation.

Yeah, I've just come to the same conclusion.  Changing throw_tcl_error
to ignore errorInfo if it's unset would be wrong, because that implies
that the function we called doesn't fill errorInfo.  I found by
testing that it's actually possible that errorInfo contains leftover
text from a previous error (that might not even have been in the same
function), resulting in completely confusing/misleading output.

So your thought that we should just not use throw_tcl_error here
was correct, and a minimal fix could look like the attached.

I thought about going further and creating a different function that
could be used in such cases, but we seem to have only the two
Tcl_ListObjGetElements() calls that could use it, so for now I think
it's not worth the trouble.

Also, compile_pltcl_function contains a Tcl_EvalEx() call that
presumably could use throw_tcl_error, except it wants to add "could
not create internal procedure" which would require some refactoring.
As far as I can tell that error case is not reproducibly reachable,
as it'd require OOM or some other problem inside Tcl, so (a) it's
probably not worth troubling over and (b) changing it is a bit scary
for lack of ability to test.  I'm inclined to leave that alone too.

The other thing I noticed while looking at this is that the error text
for the other Tcl_ListObjGetElements() call seems a bit confusingly
worded: "could not split return value from trigger: %s".  You could
easily read that as suggesting that the return value is somehow
attached to the trigger and has to be separated from it.  I'm
tempted to suggest rephrasing it to be parallel to the new error
I added: "could not parse trigger return value: %s".  But I didn't
do that below.

Thoughts?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-pltcl-error-handling-v1.patch";
	charset="us-ascii"
Content-ID: <73222.1717433844.2@sss.pgh.pa.us>
Content-Description: fix-pltcl-error-handling-v1.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/pl/tcl/expected/pltcl_call.out b/src/pl/tcl/expected/pltc=
l_call.out
index e4498375ec..9307236944 100644
--- a/src/pl/tcl/expected/pltcl_call.out
+++ b/src/pl/tcl/expected/pltcl_call.out
@@ -66,6 +66,14 @@ END
 $$;
 NOTICE:  a: 10
 NOTICE:  _a: 10, _b: 20
+-- syntax errors
+CREATE PROCEDURE test_proc10(INOUT a text)
+LANGUAGE pltcl
+AS $$
+return [list a {$a + $a}])
+$$;
+CALL test_proc10('abc');
+ERROR:  could not parse function return value: list element in braces fol=
lowed by ")" instead of space
 DROP PROCEDURE test_proc1;
 DROP PROCEDURE test_proc2;
 DROP PROCEDURE test_proc3;
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index 18d14a2b98..104175aa8c 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -1026,7 +1026,10 @@ pltcl_func_handler(PG_FUNCTION_ARGS, pltcl_call_sta=
te *call_state,
 		/* Convert function result to tuple */
 		resultObj =3D Tcl_GetObjResult(interp);
 		if (Tcl_ListObjGetElements(interp, resultObj, &resultObjc, &resultObjv)=
 =3D=3D TCL_ERROR)
-			throw_tcl_error(interp, prodesc->user_proname);
+			ereport(ERROR,
+					(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
+					 errmsg("could not parse function return value: %s",
+							utf_u2e(Tcl_GetStringResult(interp)))));
 =

 		tup =3D pltcl_build_tuple_result(interp, resultObjv, resultObjc,
 									   call_state);
@@ -1355,6 +1358,10 @@ pltcl_event_trigger_handler(PG_FUNCTION_ARGS, pltcl=
_call_state *call_state,
 =

 /**********************************************************************
  * throw_tcl_error	- ereport an error returned from the Tcl interpreter
+ *
+ * Caution: use this only to report errors returned by Tcl_EvalObjEx() or
+ * other variants of Tcl_Eval().  Other functions may not fill "errorInfo=
",
+ * so it could be unset or even contain details from some previous error.
  **********************************************************************/
 static void
 throw_tcl_error(Tcl_Interp *interp, const char *proname)
diff --git a/src/pl/tcl/sql/pltcl_call.sql b/src/pl/tcl/sql/pltcl_call.sql
index 37efbdefc2..eba5793f94 100644
--- a/src/pl/tcl/sql/pltcl_call.sql
+++ b/src/pl/tcl/sql/pltcl_call.sql
@@ -71,6 +71,17 @@ END
 $$;
 =

 =

+-- syntax errors
+
+CREATE PROCEDURE test_proc10(INOUT a text)
+LANGUAGE pltcl
+AS $$
+return [list a {$a + $a}])
+$$;
+
+CALL test_proc10('abc');
+
+
 DROP PROCEDURE test_proc1;
 DROP PROCEDURE test_proc2;
 DROP PROCEDURE test_proc3;

------- =_aaaaaaaaaa0--