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
