On Tue, 9 Feb 2016 16:23:21 -0600 Jim Nasby <jim.na...@bluetreble.com> wrote:
> Currently, pl/tcl is implemented through nothing but string > manipulation. In other words, the C code is effectively creating a > giant string that the tcl interpreter must re-parse every time the > function is executed. Additionally, all arguments are treated as raw > strings, instead of the far more efficient internal tcl object types. > > The biggest win comes with how pltcl interfaces with SPI result sets. > Currently, the entire chunk of tcl code that is executed for each > result row must be reparsed and recompiled from scratch. Now, the > code is compiled once and the bytecode is stored. > > This work is sponsored by Flight Aware (http://flightaware.com/). I've looked to your patch. As far as I know Tcl Object API it looks quite good. Of course, it breaks compatibility with pre-8.0 versions of Tcl, but it is almost twenty years since Tcl 8.0 come out. I think that checking for pre-8.0 Tcl and defining Tcl_StringResult in this case, should be removed from the code, because there is no object API in pre-8.0 Tcl anyway, and it doesn't worth effort to maintain compatibility. (line 51 of pltcl.c). There is suspicious place at the end of compile_pltcl_fuction function, where you've put comment that old prodesc cannot be deallocated, because it might be used by other call. It seems that reference counting mechanism which Tcl already has, can help here. Would there be serious performance impact if you'll use Tcl list instead of C structure to store procedure description? If not, you can rely on Tcl_IncrRefCount/Tcl_DecrRefCount to free a procedure description when last active call finishes. Function pltcl_elog have a big switch case to convert enum logpriority, local to this function to PostgreSql log levels. It seems not a most readable solution, because this enumeration is desined to be passed to Tcl_GetIndexFromObj, so it can be used and is used to index an array (logpriorities array of string representation). You can define simular array with PostgreSQL integer constant and replace page-sized switch with just two lines - this array definition and getting value from it by index static CONST84 int loglevels[]={DEBUG,LOG,INFO,NOTICE,WARNING,ERROR,FATAL}; .... Tcl_GetIndexFromObj(.... level=loglevels[priIndex]; It seems that you are losing some diagnostic information by extracting just message field from ErrorData, which you do in pltcl_elog and pltcl_subtrans_abort. Tcl has mechanisms for passing around additional error information. See Tcl_SetErrorCode and Tcl_AddErrorInfo functions pltcl_process_SPI_result might benefit from providing SPI result code in machine readable way via Tcl_SetErrorCode too. In some cases you use Tcl_SetObjResult(interp, Tcl_NewStringObj("error message",-1)) to report error with constant message, where in other places Tcl_SetResult(interp,"error message",TCL_STATIC) left in place. I see no harm in using old API with static messages, especially if Tcl_AppendResult is used, but mixing two patterns above seems to be a bit inconsistent. As far as I can understand, using Tcl_StringObj to represent all postgresql primitive (non-array) types and making no attempt to convert tuple data into integer, boolean or double objects is design decision. It really can spare few unnecessary type conversions. Thanks for your effort. -- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
