Re: Loading the PL/pgSQL debugger (and other plugins)

Thanks for the quick feedback.
>> 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).
>>
>
> This seems a bit messy and special-purpose.
Agreed, I'm not crazy about using a custom_variable_class variable either.
> I see no good reason to tie
> it to plpgsql; we'll just need another one for every other language.
>
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)?

> 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.)
>
Agreed.
> 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).
>
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 the
call-handlers).

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

PLpgSQL_plugin pluginHooks;
typedef void (*plugin_loader_func)(PLpgSQL_plugin *hooks);

void
plpgsql_init(void)
{
static char * pluginName;
plugin_load_func plugin_loader();

/* Do initialization only once */
if (!plpgsql_firstcall)
return;

plpgsql_HashTableInit();
RegisterXactCallback(plpgsql_xact_cb, NULL);
plpgsql_firstcall = false;

/* Load any instrumentation plugins */
DefineCustomStringVariable( "plpgsql.plugin",
"Name of instrumentation plugin to use
when PL/pgSQL function is invoked",
NULL,
&pluginName,
PGC_USERSET,
NULL,
NULL );

EmitWarningsOnPlaceholders("plpgsql");

if (pluginName )
{
plugin_loader = (plugin_loader_func
*)load_external_function(pluginName, "plugin_loader", false, NULL );

if (plugin_loader)
(*plugin_loader)(&pluginHooks);
}
}

(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.
> 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.
>
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.
> 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();" ...
>
Ok.
>> 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.
>
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.
>> 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
> now.
>
Cool.

-- Korry