Hello Marko
2014-01-16 23:54 GMT+01:00 Marko Tiikkaja <ma...@joh.to>: > Hi Pavel, > > First of all, thanks for working on this! > > > On 1/12/14, 8:58 PM, Pavel Stehule wrote: > >> I still not happy with plugin_info - it is only per plugin now and should >> be per plugin and per function. >> > > I'm not sure I understand the point of plugin_info in the first place, but > what would having a separate info per (plugin, function) achieve that can't > be done otherwise? > > > As for the current patch, I'd like to see improvements on a few things: > > 1) It doesn't currently compile because of extra semicolons in the > PLpgSQL_plugin struct. > fixed > > 2) The previous comment above the same struct still talk about the > rendezvous variable which is now gone. The comment should be > updated to reflect the new API. > removed > > 3) The same comment talks about how important it is to unregister a > plugin if its _PG_fini() is ever called, but the current API does > not support unregistering. That should probably be added? I'm not > sure when _PG_fini() would be called. > removed These plugins should not be removed - there is no any mechanism how to remove active plugin without close session Regards Pavel > > 4) The comment /* reserved for use by optional plugin */ seems a bit > weird in its new context. > > > Regards, > Marko Tiikkaja >
commit bf0820786f08b08812bba3d86233cbac30922054 Author: Pavel Stehule <pavel.steh...@gooddata.com> Date: Mon Feb 10 17:50:31 2014 +0100 initial diff --git a/src/pl/plpgsql/src/Makefile b/src/pl/plpgsql/src/Makefile index 852b0c7..37d17a8 100644 --- a/src/pl/plpgsql/src/Makefile +++ b/src/pl/plpgsql/src/Makefile @@ -19,7 +19,7 @@ rpath = OBJS = pl_gram.o pl_handler.o pl_comp.o pl_exec.o pl_funcs.o pl_scanner.o -DATA = plpgsql.control plpgsql--1.0.sql plpgsql--unpackaged--1.0.sql +DATA = plpgsql.control plpgsql--1.1.sql plpgsql--1.0--1.1.sql plpgsql--unpackaged--1.0.sql all: all-lib diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 3749fac..e6510f1 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -86,6 +86,22 @@ typedef struct SimpleEcontextStackEntry static EState *shared_simple_eval_estate = NULL; static SimpleEcontextStackEntry *simple_econtext_stack = NULL; +/* + * List of pointers and info of registered plugins. + */ +typedef struct PluginPtrEntry +{ + PLpgSQL_plugin *plugin_ptr; + struct PluginPtrEntry *next; +} PluginPtrEntry; + +/* + * Allocated in TopMemoryContext + */ +static PluginPtrEntry *plugins = NULL; +static int nplugins = 0; +static int used_plugin_hook_types = 0; + /************************************************************ * Local function forward declarations ************************************************************/ @@ -235,6 +251,18 @@ static char *format_expr_params(PLpgSQL_execstate *estate, const PLpgSQL_expr *expr); static char *format_preparedparamsdata(PLpgSQL_execstate *estate, const PreparedParamsData *ppd); +static void **get_plugin_info(PLpgSQL_execstate *estate, int plugin_number); + + +/* Bits for used plugin callback types */ +#define PLUGIN_HOOK_TYPE_FUNC_SETUP (1 << 0) +#define PLUGIN_HOOK_TYPE_FUNC_BEG (1 << 2) +#define PLUGIN_HOOK_TYPE_FUNC_END (1 << 3) +#define PLUGIN_HOOK_TYPE_STMT_BEG (1 << 4) +#define PLUGIN_HOOK_TYPE_STMT_END (1 << 5) + +#define EXEC_PLUGIN_HOOK_TYPE(type) \ + ((used_plugin_hook_types & (PLUGIN_HOOK_TYPE_ ## type)) == (PLUGIN_HOOK_TYPE_ ## type)) /* ---------- @@ -331,10 +359,28 @@ plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo, exec_set_found(&estate, false); /* - * Let the instrumentation plugin peek at this function + * Let the instrumentation plugins peek at this function */ - if (*plugin_ptr && (*plugin_ptr)->func_beg) - ((*plugin_ptr)->func_beg) (&estate, func); + if (EXEC_PLUGIN_HOOK_TYPE(FUNC_BEG)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; pe = pe->next, i++) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + + if (pl_ptr->func_beg) + { + void **plugin_info; + + plugin_info = get_plugin_info(&estate, i); + (pl_ptr->func_beg) (&estate, func, plugin_info); + } + } + } /* * Now call the toplevel block of statements @@ -479,10 +525,28 @@ plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo, estate.err_text = gettext_noop("during function exit"); /* - * Let the instrumentation plugin peek at this function + * Let the instrumentation plugins peek at this function */ - if (*plugin_ptr && (*plugin_ptr)->func_end) - ((*plugin_ptr)->func_end) (&estate, func); + if (EXEC_PLUGIN_HOOK_TYPE(FUNC_END)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; i++, pe = pe->next) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + + if (pl_ptr->func_end) + { + void **plugin_info; + + plugin_info = get_plugin_info(&estate, i); + (pl_ptr->func_end) (&estate, func, plugin_info); + } + } + } /* Clean up any leftover temporary memory */ plpgsql_destroy_econtext(&estate); @@ -699,10 +763,27 @@ plpgsql_exec_trigger(PLpgSQL_function *func, exec_set_found(&estate, false); /* - * Let the instrumentation plugin peek at this function + * Let the instrumentation plugins peek at this function */ - if (*plugin_ptr && (*plugin_ptr)->func_beg) - ((*plugin_ptr)->func_beg) (&estate, func); + if (EXEC_PLUGIN_HOOK_TYPE(FUNC_BEG)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; i++, pe = pe->next) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + void **plugin_info; + + if (pl_ptr->func_beg) + { + plugin_info = get_plugin_info(&estate, i); + (pl_ptr->func_beg) (&estate, func, plugin_info); + } + } + } /* * Now call the toplevel block of statements @@ -768,10 +849,27 @@ plpgsql_exec_trigger(PLpgSQL_function *func, } /* - * Let the instrumentation plugin peek at this function + * Let the instrumentation plugins peek at this function */ - if (*plugin_ptr && (*plugin_ptr)->func_end) - ((*plugin_ptr)->func_end) (&estate, func); + if (EXEC_PLUGIN_HOOK_TYPE(FUNC_END)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; i++, pe = pe->next) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + void **plugin_info; + + if (pl_ptr->func_end) + { + plugin_info = get_plugin_info(&estate, i); + (pl_ptr->func_end) (&estate, func, plugin_info); + } + } + } /* Clean up any leftover temporary memory */ plpgsql_destroy_econtext(&estate); @@ -831,10 +929,28 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata) var->freeval = true; /* - * Let the instrumentation plugin peek at this function + * Let the instrumentation plugins peek at this function */ - if (*plugin_ptr && (*plugin_ptr)->func_beg) - ((*plugin_ptr)->func_beg) (&estate, func); + if (EXEC_PLUGIN_HOOK_TYPE(FUNC_BEG)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; i++, pe = pe->next) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + + if (pl_ptr->func_beg) + { + void **plugin_info; + + plugin_info = get_plugin_info(&estate, i); + (pl_ptr->func_beg) (&estate, func, plugin_info); + } + } + } /* * Now call the toplevel block of statements @@ -865,10 +981,28 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata) estate.err_text = gettext_noop("during function exit"); /* - * Let the instrumentation plugin peek at this function + * Let the instrumentation plugins peek at this function */ - if (*plugin_ptr && (*plugin_ptr)->func_end) - ((*plugin_ptr)->func_end) (&estate, func); + if (EXEC_PLUGIN_HOOK_TYPE(FUNC_END)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; i++, pe = pe->next) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + + if (pl_ptr->func_end) + { + void **plugin_info; + + plugin_info = get_plugin_info(&estate, i); + (pl_ptr->func_end) (&estate, func, plugin_info); + } + } + } /* Clean up any leftover temporary memory */ plpgsql_destroy_econtext(&estate); @@ -1389,9 +1523,27 @@ exec_stmt(PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt) save_estmt = estate->err_stmt; estate->err_stmt = stmt; - /* Let the plugin know that we are about to execute this statement */ - if (*plugin_ptr && (*plugin_ptr)->stmt_beg) - ((*plugin_ptr)->stmt_beg) (estate, stmt); + /* Let the plugins know that we are about to execute this statement */ + if (EXEC_PLUGIN_HOOK_TYPE(STMT_BEG)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; i++, pe = pe->next) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + + if (pl_ptr->stmt_beg) + { + void **plugin_info; + + plugin_info = get_plugin_info(estate, i); + (pl_ptr->stmt_beg) (&estate, stmt, plugin_info); + } + } + } CHECK_FOR_INTERRUPTS(); @@ -1495,8 +1647,26 @@ exec_stmt(PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt) } /* Let the plugin know that we have finished executing this statement */ - if (*plugin_ptr && (*plugin_ptr)->stmt_end) - ((*plugin_ptr)->stmt_end) (estate, stmt); + if (EXEC_PLUGIN_HOOK_TYPE(STMT_END)) + { + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; i++, pe = pe->next) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + + if (pl_ptr->stmt_end) + { + void **plugin_info; + + plugin_info = get_plugin_info(estate, i); + (pl_ptr->stmt_end) (&estate, stmt, plugin_info); + } + } + } estate->err_stmt = save_estmt; @@ -3160,26 +3330,44 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate, estate->err_stmt = NULL; estate->err_text = NULL; - estate->plugin_info = NULL; - /* * Create an EState and ExprContext for evaluation of simple expressions. */ plpgsql_create_econtext(estate); + estate->plugin_info_vector = NULL; + estate->plugin_info_size = 0; + /* * Let the plugin see this function before we initialize any local * PL/pgSQL variables - note that we also give the plugin a few function * pointers so it can call back into PL/pgSQL for doing things like * variable assignments and stack traces */ - if (*plugin_ptr) + if (plugins) { - (*plugin_ptr)->error_callback = plpgsql_exec_error_callback; - (*plugin_ptr)->assign_expr = exec_assign_expr; + PluginPtrEntry *pe; + int i; + + Assert(plugins != NULL); + + for (i = 0, pe = plugins; pe != NULL; pe = pe->next, i++) + { + PLpgSQL_plugin *pl_ptr = pe->plugin_ptr; + + pl_ptr->error_callback = plpgsql_exec_error_callback; + pl_ptr->assign_expr = exec_assign_expr; + + if (EXEC_PLUGIN_HOOK_TYPE(FUNC_SETUP)) + { + void **plugin_info; + + Assert(pl_ptr->func_setup); - if ((*plugin_ptr)->func_setup) - ((*plugin_ptr)->func_setup) (estate, func); + plugin_info = get_plugin_info(estate, i); + (pl_ptr->func_setup) (estate, func, plugin_info); + } + } } } @@ -6632,3 +6820,62 @@ format_preparedparamsdata(PLpgSQL_execstate *estate, return paramstr.data; } + +/* + * Register a next plugin info + */ +void +plpgsql_register_plugin_internal(PLpgSQL_plugin *plugin_ptr) +{ + PluginPtrEntry *new_entry; + PluginPtrEntry *plugin; + MemoryContext old_cxt; + + old_cxt = MemoryContextSwitchTo(TopMemoryContext); + new_entry = palloc0(sizeof(PluginPtrEntry)); + MemoryContextSwitchTo(old_cxt); + + /* Order of plugins is important - new plugins must be on end */ + for (plugin = plugins; plugin->next != NULL; plugin = plugin->next); + plugin->next = new_entry; + nplugins++; + + used_plugin_hook_types |= (plugin_ptr->func_setup ? PLUGIN_HOOK_TYPE_FUNC_SETUP : 0); + used_plugin_hook_types |= (plugin_ptr->func_beg ? PLUGIN_HOOK_TYPE_FUNC_BEG : 0); + used_plugin_hook_types |= (plugin_ptr->func_end ? PLUGIN_HOOK_TYPE_FUNC_END : 0); + used_plugin_hook_types |= (plugin_ptr->stmt_beg ? PLUGIN_HOOK_TYPE_STMT_BEG : 0); + used_plugin_hook_types |= (plugin_ptr->stmt_end ? PLUGIN_HOOK_TYPE_STMT_END : 0); +} + +/* + * Ensure enough sized plugin_info_vector. This vector can be resized in runtime. + * It is unusual, but there are no reason, why we disallow it. + * + * Attention: plugins should not to cache plugin_info in private space. This pointer + * can be changed due possible repalloc. + */ +static void ** +get_plugin_info(PLpgSQL_execstate *estate, int plugin_number) +{ + int i; + + if (estate->plugin_info_size >= plugin_number) + return (void **) &estate->plugin_info_vector[plugin_number - 1]; + + /* + * plugin_info_vector holds a pointers to function execution level + * plugin private memory. Designed implementation expects relatively + * immutable and small number of plugins. plugin_vector is resized + * to number of registered plugins. So usually plugin_info_vector + * is allocated only once. + */ + estate->plugin_info_vector = repalloc(estate->plugin_info_vector, + nplugins * sizeof(void*)); + + for (i = estate->plugin_info_size; i < nplugins - 1; i++) + estate->plugin_info_vector[i] = NULL; + + estate->plugin_info_size = nplugins; + + return (void **) &estate->plugin_info_vector[plugin_number - 1]; +} diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index f02203a..db6851e 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -39,10 +39,6 @@ int plpgsql_variable_conflict = PLPGSQL_RESOLVE_ERROR; bool plpgsql_print_strict_params = false; -/* Hook for plugins */ -PLpgSQL_plugin **plugin_ptr = NULL; - - /* * _PG_init() - library load-time initialization * @@ -82,9 +78,6 @@ _PG_init(void) RegisterXactCallback(plpgsql_xact_cb, NULL); RegisterSubXactCallback(plpgsql_subxact_cb, NULL); - /* Set up a rendezvous point with optional instrumentation plugin */ - plugin_ptr = (PLpgSQL_plugin **) find_rendezvous_variable("PLpgSQL_plugin"); - inited = true; } @@ -384,3 +377,21 @@ plpgsql_validator(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } + +/* ---------- + * + * register any plpgsql plugin + * + * ---------- + */ +PG_FUNCTION_INFO_V1(plpgsql_register_plugin); + +Datum +plpgsql_register_plugin(PG_FUNCTION_ARGS) +{ + PLpgSQL_plugin *plugin_ptr = (PLpgSQL_plugin *) PG_GETARG_POINTER(0); + + plpgsql_register_plugin_internal(plugin_ptr); + + PG_RETURN_VOID(); +} diff --git a/src/pl/plpgsql/src/plpgsql.control b/src/pl/plpgsql/src/plpgsql.control index b320227..4c75c93 100644 --- a/src/pl/plpgsql/src/plpgsql.control +++ b/src/pl/plpgsql/src/plpgsql.control @@ -1,6 +1,6 @@ # plpgsql extension comment = 'PL/pgSQL procedural language' -default_version = '1.0' +default_version = '1.1' module_pathname = '$libdir/plpgsql' relocatable = false schema = pg_catalog diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index f3d1283..cac9b9a 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -792,18 +792,16 @@ typedef struct PLpgSQL_execstate PLpgSQL_stmt *err_stmt; /* current stmt */ const char *err_text; /* additional state info */ - void *plugin_info; /* reserved for use by optional plugin */ + void **plugin_info_vector; /* array of plugin info ptrs */ + int plugin_info_size; /* size of plugin_info_vector */ } PLpgSQL_execstate; /* * A PLpgSQL_plugin structure represents an instrumentation plugin. - * To instrument PL/pgSQL, a plugin library must access the rendezvous - * variable "PLpgSQL_plugin" and set it to point to a PLpgSQL_plugin struct. * Typically the struct could just be static data in the plugin library. - * We expect that a plugin would do this at library load time (_PG_init()). - * It must also be careful to set the rendezvous variable back to NULL - * if it is unloaded (_PG_fini()). + * We expect that a plugin would do this at library load time (_PG_init()) + * with call plpgsql_register_plugin. * * This structure is basically a collection of function pointers --- at * various interesting points in pl_exec.c, we call these functions @@ -830,11 +828,16 @@ typedef struct PLpgSQL_execstate typedef struct { /* Function pointers set up by the plugin */ - void (*func_setup) (PLpgSQL_execstate *estate, PLpgSQL_function *func); - void (*func_beg) (PLpgSQL_execstate *estate, PLpgSQL_function *func); - void (*func_end) (PLpgSQL_execstate *estate, PLpgSQL_function *func); - void (*stmt_beg) (PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt); - void (*stmt_end) (PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt); + void (*func_setup) (PLpgSQL_execstate *estate, PLpgSQL_function *func, + void **plugin_info;); + void (*func_beg) (PLpgSQL_execstate *estate, PLpgSQL_function *func, + void **plugin_info;); + void (*func_end) (PLpgSQL_execstate *estate, PLpgSQL_function *func, + void **plugin_info;); + void (*stmt_beg) (PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt, + void **plugin_info;); + void (*stmt_end) (PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt, + void **plugin_info;); /* Function pointers set by PL/pgSQL itself */ void (*error_callback) (void *arg); @@ -894,8 +897,6 @@ extern char *plpgsql_error_funcname; extern PLpgSQL_function *plpgsql_curr_compile; extern MemoryContext compile_tmp_cxt; -extern PLpgSQL_plugin **plugin_ptr; - /********************************************************************** * Function declarations **********************************************************************/ @@ -941,6 +942,7 @@ extern void _PG_init(void); extern Datum plpgsql_call_handler(PG_FUNCTION_ARGS); extern Datum plpgsql_inline_handler(PG_FUNCTION_ARGS); extern Datum plpgsql_validator(PG_FUNCTION_ARGS); +extern Datum plpgsql_register_plugin(PG_FUNCTION_ARGS); /* ---------- * Functions in pl_exec.c @@ -961,6 +963,7 @@ extern Oid exec_get_datum_type(PLpgSQL_execstate *estate, extern void exec_get_datum_type_info(PLpgSQL_execstate *estate, PLpgSQL_datum *datum, Oid *typeid, int32 *typmod, Oid *collation); +extern void plpgsql_register_plugin_internal(PLpgSQL_plugin *plugin_ptr); /* ---------- * Functions for namespace handling in pl_funcs.c
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers