Re: Convert pltcl from strings to objects

On 2/18/16 6:26 AM, Victor Wagner wrote:
> On Tue, 9 Feb 2016 16:23:21 -0600
> 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.

Are you referring to this comment?

>         /************************************************************
>          * Add the proc description block to the hashtable.  Note we do not
>          * attempt to free any previously existing prodesc block.  This is
>          * annoying, but necessary since there could be active calls using
>          * the old prodesc.
>          ************************************************************/

That is not changed by the patch, and I don't think it's in the scope of 
this patch. I agree it would be nice to get rid of that and the related 
malloc() call, as well as what perm_fmgr_info() is doing, but those are 
unrelated to this work.

(BTW, I also disagree about using a Tcl list for prodesc. That's an 
object in a *Postgres* hash table; as such it needs to be managed by 
Postgres, not Tcl. AFAIK the Postgres ResourceOwner stuff would be the 
correct way to do that (but I'm not exactly an expert on that area).

> 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];

Done.

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

Is there any backwards compatibility risk to these changes? Could having 
that new info break someone's existing code?

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

Switched everything to using the new API.

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

Yeah, that's on the list. But IMHO it's outside the scope of this patch.

> Thanks for your effort.

Thanks for the review!
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com