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

Sorry to poke - but I'd like to get a patch submitted next week. Any
more comments? Thanks.

-- Korry

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