Hello updated patch - now plugin_info is per plpgsq_estate/plugin again.
Regards Pavel 2014-01-17 20:26 GMT+01:00 Pavel Stehule <pavel.steh...@gmail.com>: > > > > 2014/1/16 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? >> > > First use case - I would to protect repeated call of > plpgsql_check_function in passive mode. Where I have to store information > about successful first start? It is related to the function instance, so > function oid can be ambiguous (for function with polymorphic parameters). > When function instance is destroyed, then this information should be > destroyed. It is impossible do this check from plugin. Second use case - > attach session life cycle plugin data with some function - for example for > coverage calculation. Inside plugin without function specific data you have > to hold a hash of all used function, and you have to search again and > again. When plpgsql hold this info in internal plpgsql function structures, > then you don't need search anything. > > > > > Regards > > Pavel > > > >> >> >> 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. >> >> 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. >> >> 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. >> >> 4) The comment /* reserved for use by optional plugin */ seems a bit >> weird in its new context. >> >> >> Regards, >> Marko Tiikkaja >> > >
commit eecd714579b3683b02a814b685b1d559ba0e0da8 Author: Pavel Stehule <pavel.steh...@gmail.com> Date: Sun Feb 9 22:08:18 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..ed540a9 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--1.0--1.1.sql b/src/pl/plpgsql/src/plpgsql--1.0--1.1.sql new file mode 100644 index 0000000..c7e9a62 --- /dev/null +++ b/src/pl/plpgsql/src/plpgsql--1.0--1.1.sql @@ -0,0 +1,6 @@ +/* src/pl/plpgsql/src/plpgsql--1.0--1.1.sql */ + +CREATE FUNCTION plpgsql_register_plugin(internal) +RETURNS void +AS 'MODULE_PATHNAME' +LANGUAGE C STRICT; diff --git a/src/pl/plpgsql/src/plpgsql--1.1.sql b/src/pl/plpgsql/src/plpgsql--1.1.sql new file mode 100644 index 0000000..74aa4b9 --- /dev/null +++ b/src/pl/plpgsql/src/plpgsql--1.1.sql @@ -0,0 +1,16 @@ +/* src/pl/plpgsql/src/plpgsql--1.1.sql */ + +/* + * Currently, all the interesting stuff is done by CREATE LANGUAGE. + * Later we will probably "dumb down" that command and put more of the + * knowledge into this script. + */ + +CREATE PROCEDURAL LANGUAGE plpgsql; + +COMMENT ON PROCEDURAL LANGUAGE plpgsql IS 'PL/pgSQL procedural language'; + +CREATE FUNCTION plpgsql_register_plugin(internal) +RETURNS void +AS 'MODULE_PATHNAME' +LANGUAGE C STRICT; 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..6494fb4 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -792,7 +792,8 @@ 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; @@ -830,11 +831,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 +900,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 +945,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 +966,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