Re: Convert pltcl from strings to objects

On Tue, 9 Feb 2016 16:23:21 -0600
Jim Nasby <Jim.Nasby@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.

--