Re: Convert pltcl from strings to objects

On Mon, 22 Feb 2016 17:57:36 -0600
Jim Nasby <Jim.Nasby@BlueTreble.com> wrote:

> On 2/18/16 6:26 AM, Victor Wagner wrote:

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

If it has been there, before, it is OK to leave it this way.
It was rather wild guess that use of referential counting which we now
have here, can solve this old problem.
> (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).

As far as I understand ResourceOwner is completely different garbage
collection scheme, than reference counting. ResourceOwner mechanism lets
free everything associated with transaction when transaction is over.

Here we have another case. prodesc is a global thing. And it is shared
between different operations. Problem was that there is no partcular
owner, and we have to wait when last operation which deals with it
would finish. It looks like perfect job for reference counting.

Actually, it doesn't matter much, because it wastes some memory only on
procedure change, and typically in the heavy loaded production
environments server side code doesn't change too frequently.

> 
> > 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 
[skip]
> 
> Done.

Glad that my suggestion was useful to you.

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

I don't think so. ErrorCode and ErrorInfo mechanisms present in the Tcl
for ages. As far as I remember, it predates Tcl_Obj API. Unfortunately
I've lost my copy of 2-nd edition of "Practical Programming on Tcl/Tk"
and have only 4-th edition handy, so it's quite hard to dig out Tcl 7.6
docs. 

Moreover. errorcode and errorinfo information travel behind the scenes
and cannot break any code which doesn't try to explicitely access it.


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

I've mean, that there is no need to be to aggressive in the type
conversion. You'll never know if Tcl code would actually access some
tuple elements or not. Let's leave conversion to the time of actual use
of values.
> 
> > Thanks for your effort.  
> 
> Thanks for the review!

Please, send updated patch to the list in this thread, so it would
appear in the commitfest and I can mark your patch as "ready for
committer".







--                                   Victor Wagner <vitus@wagner.pp.ru>