Re: pltcl crashes due to a syntax error

Erik Wienhold <ewie(at)ewie(dot)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