Sorry to poke - but I'd like to get a patch submitted next week. Any
more comments? Thanks.|
for the quick feedback.
Agreed, I'm not crazy about using a custom_variable_class variable
1) I think the most straightforward way to load an instrumentation
plugin is to define a new custom GUC variable (using the
This seems a bit messy and special-purpose.
Hmmm... but the plugins themselves would 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 such a GUC variable look like)?
I see no good reason to tie
it to plpgsql; we'll just need another one for every other language.
IMHO what 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.)
But there's a timing issue there. If you ask the plugin to call a
call-handler function, then you can't load the plugin at backend
startup because the PL/pgSQL call-handler isn't loaded until it's
required. Since both the plugin and the call-handler are dynamically
loaded, I think one of them has to load the other. We already have a
mechanism for loading call-handlers on demand - it seems kind of messy
to introduce another mechanism for loading plugins (that in turn load
When the plugin's shared library gets loaded, one way or the 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).
The PL/pgSQL call-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).
What I'm thinking is that the plpgsql_init() function would look
something like this (my changes in red);
typedef void (*plugin_loader_func)(PLpgSQL_plugin *hooks);
static char * pluginName;
/* Do initialization only once */
plpgsql_firstcall = false;
/* Load any instrumentation plugins */
"Name of instrumentation plugin to use
when PL/pgSQL function is invoked",
if (pluginName )
plugin_loader = (plugin_loader_func
*)load_external_function(pluginName, "plugin_loader", false, NULL );
(Ignore the custom variable stuff for now)
Each plugin would export a plugin_loader() function - that function,
given a pointer to a PLpgSQL_plugin structure, would fill in that
structure with the required function pointers.
You're right, privileges are an issue. Is it safe enough if we force
all plugins to reside in $libdir? Each plugin could enforce additional
security as needed that way, but you'd have to hold enough privileges
to get your plugin into $libdir to begin with so you can't write your
own nasty plugin to gain more privileges than you ought to have.
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.
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();" ...
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 to the end of the plugin structure - if the pointers are
non-NULL, you gain more functionality.
3) Any comments on the PLpgSQL_plugin structure? Should it include (as
it's first member) a structure version number so we can add to/change
the structure as needed?
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.
4) Do we need to support multiple active plugins?
Probably, but let's fix the API to hide this, so we don't have to commit