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