Re: Loading the PL/pgSQL debugger (and other plugins) - Mailing list pgsql-hackers

Thanks for the quick feedback.<br /><blockquote cite="mid14284.1153330516@sss.pgh.pa.us" type="cite"><blockquote
type="cite"><prewrap="">1) I think the most straightforward way to load an instrumentation 
 
plugin is to define a new custom GUC variable (using the 
custom_variable_classes mechanism).   </pre></blockquote><pre wrap="">
This seems a bit messy and special-purpose.  </pre></blockquote> Agreed, I'm not crazy about using a
custom_variable_classvariable either.<br /><blockquote cite="mid14284.1153330516@sss.pgh.pa.us" type="cite"><pre
wrap="">Isee no good reason to tie
 
it to plpgsql; we'll just need another one for every other language. </pre></blockquote> Hmmm... but the plugins
themselveswould be language-specific.  I can't imagine that a plugin (say a profiler) for PL/python would work for
PL/pgSQL. It seems to me that, even if we come up with a common mechanism, we'll still need a separate GUC variable
*name*for each PL.  Or am I not understanding something?  Can you post an example of what you are thinking (what would
sucha GUC variable look like)?<br /><br /><blockquote cite="mid14284.1153330516@sss.pgh.pa.us" type="cite"><pre
wrap="">IMHOwhat we want is something with similar properties to preload_libraries,
 
but processed on a per-backend basis instead of once at postmaster start.
(You could almost just tell people to select the plugin they want by
LOADing it, but that is hard to use if you're trying to debug a
non-interactive application.  A GUC variable can be set for an app
without much cooperation from the app.) </pre></blockquote> Agreed. <br /><blockquote
cite="mid14284.1153330516@sss.pgh.pa.us"type="cite"><pre wrap="">When the plugin's shared library gets loaded, one way
orthe other,
 
it should construct the function-pointer struct and then pass it to a
function defined by plpgsql (this lets us hide/postpone the decision
about whether there can be more than one active plugin). </pre></blockquote> But there's a timing issue there.  If you
askthe plugin to call a call-handler function, then you can't load the plugin at backend startup because the PL/pgSQL
call-handlerisn't loaded until it's required.  Since both the plugin and the call-handler are dynamically loaded, I
thinkone of them has to load the other.  We already have a mechanism for loading call-handlers on demand - it seems
kindof messy to introduce another mechanism for loading plugins (that in turn load the call-handlers).<br /><br /> The
PL/pgSQLcall-handler has a convenient initialization function that could read the GUC variable and load the referenced
plugin(that's what I'm doing right now).<br /><br /> What I'm thinking is that the plpgsql_init() function would look
somethinglike this (my changes in red);<br /><br /><tt><font color="#ff0000">PLpgSQL_plugin   pluginHooks;<br />
typedefvoid (*plugin_loader_func)(PLpgSQL_plugin *hooks);<br /></font><br /> void<br /> plpgsql_init(void)<br /> {<br
/><fontcolor="#ff0000">    static char * pluginName;<br />     plugin_load_func   plugin_loader();<br /></font><br />
   /* Do initialization only once */<br />     if (!plpgsql_firstcall)<br />         return;<br /><br />    
plpgsql_HashTableInit();<br/>     RegisterXactCallback(plpgsql_xact_cb, NULL);<br />     plpgsql_firstcall = false;<br
/><br/><font color="#ff0000">    /* Load any instrumentation plugins */<br />     DefineCustomStringVariable(
"plpgsql.plugin",<br />                                 "Name of instrumentation plugin to use when PL/pgSQL function
isinvoked",<br />                                 NULL,<br />                                 &pluginName,<br />
                               PGC_USERSET,<br />                                 NULL,<br />                        
       NULL );<br /><br />     EmitWarningsOnPlaceholders("plpgsql");<br /><br />     if (pluginName )<br />     {<br
/>        plugin_loader = (plugin_loader_func *)load_external_function(pluginName, "plugin_loader", false, NULL );<br
/><br/>         if (plugin_loader)<br />             (*plugin_loader)(&pluginHooks);<br />     }<br
/></font>}  </tt> <br /><br /> (Ignore the custom variable stuff for now)<br /><br /> Each plugin would export a
plugin_loader()function - that function, given a pointer to a PLpgSQL_plugin structure, would fill in that structure
withthe required function pointers. <br /><blockquote cite="mid14284.1153330516@sss.pgh.pa.us" type="cite"><pre
wrap="">
One issue that needs to be thought about with either this proposal or
your original is what permissions are needed to set the GUC variable.
I don't think we dare allow non-superusers to specify LOADing of
arbitrary shared libraries, so there has to be some filter function.

Perhaps a better way is that the GUC variable specifies a (list of)
initialization functions to call at backend start, and then the
superuserness is involved with installing the init functions into
pg_proc, and the GUC variable itself needs no special permissions.
Again, a plugin's init function would just register its function-pointer
struct with plpgsql. </pre></blockquote> You're right, privileges are an issue.  Is it safe enough if we force all
pluginsto reside in $libdir?  Each plugin could enforce additional security as needed that way, but you'd have to hold
enoughprivileges to get your plugin into $libdir to begin with so you can't write your own nasty plugin to gain more
privilegesthan you ought to have.<br /><blockquote cite="mid14284.1153330516@sss.pgh.pa.us" type="cite"><pre wrap="">
 
We should also think about a deregistration function.  This would allow
you to turn debugging on and off within an interactive session.  The
GUC variable is really only for coercing non-interactive applications
into being debuggable --- I don't see it as being important for
interactive debugging, as compared to just "select plugin_init();" ... </pre></blockquote> Ok.<br /><blockquote
cite="mid14284.1153330516@sss.pgh.pa.us"type="cite"><blockquote type="cite"><pre wrap="">3) Any comments on the
PLpgSQL_pluginstructure?  Should it include (as 
 
it's first member) a structure version number so we can add to/change 
the structure as needed?   </pre></blockquote><pre wrap="">
Given our current plans for enforcing recompiles at major version
changes (via magic-block checking), I'm not sure I see a need for this. </pre></blockquote> That makes a lot more sense
-I don't like the idea of each plugin managing its own version information.  We can always add more function pointers
tothe end of the plugin structure - if the pointers are non-NULL, you gain more functionality.<br /><blockquote
cite="mid14284.1153330516@sss.pgh.pa.us"type="cite"><pre wrap=""></pre><blockquote type="cite"><pre wrap="">4) Do we
needto support multiple active plugins?   </pre></blockquote><pre wrap="">
 
Probably, but let's fix the API to hide this, so we don't have to commit
now. </pre></blockquote> Cool.<br /><br />           -- Korry<br /><br />