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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
