Date: Mon, 3 Jun 2024 23:48:47 +0200
From: Erik Wienhold <ewie@ewie.name>
To: Tom Lane <tgl@sss.pgh.pa.us>
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
Message-ID: <dc4d4bc0-6283-41dd-8e5d-5b9f4cf4fa4b@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>
 <73249.1717433869@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <73249.1717433869@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/dc4d4bc0-6283-41dd-8e5d-5b9f4cf4fa4b%40ewie.name>
Precedence: bulk

On 2024-06-03 18:57 +0200, Tom Lane wrote:
> 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.

LGTM.

> 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.

Agree.

> 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.

Yeah, I'd fix that trigger error text as well to bring both in line.

-- 
Erik