Completing PL support for Event Triggers

Started by Dimitri Fontaineover 12 years ago14 messages
#1Dimitri Fontaine
dimitri@2ndQuadrant.fr
3 attachment(s)

Hi,

Please find attached to this email three patches covering the missing PL
support for Event Triggers: pltcl, plperl and plpython.

Due to “platform” problems here tonight and the CF deadline, the
plpython patch is known not to pass regression tests on my machine. The
code is fully rebased and compiles without warning, though, so I'm still
entering it into this CF: hopefully I will figure out what's wrong with
my local plpython platform support here early next week.

I'm going to add the 3 attached patches to the CF as a single item, but
maybe we'll then split if we have separate subsystem maintainers for
those 3 PLs?

Given the size of the changes (docs included in the following stats) I
don't think that's going to be necessary, but well, let me know.

tcl 4 files changed, 204 insertions(+), 24 deletions(-)
perl 4 files changed, 240 insertions(+), 12 deletions(-)
python 8 files changed, 165 insertions(+), 14 deletions(-)
total 16 files changed, 609 insertions(+), 50 deletions(-)

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Attachments:

event_trigger_plperl.v1.patch.gzapplication/octet-streamDownload
event_trigger_plpython.v1.patch.gzapplication/octet-streamDownload
event_trigger_pltcl.v1.patch.gzapplication/octet-streamDownload
#2Peter Eisentraut
peter_e@gmx.net
In reply to: Dimitri Fontaine (#1)
Re: Completing PL support for Event Triggers

On Fri, 2013-09-13 at 22:40 +0200, Dimitri Fontaine wrote:

Please find attached to this email three patches covering the missing PL
support for Event Triggers: pltcl, plperl and plpython.

You introduced some new compiler warnings, please fix those.

http://pgci.eisentraut.org/jenkins/view/PostgreSQL/job/postgresql_commitfest_world/80/warnings19Result/new/?

In the source code, I'm dubious about the use of is_dml_trigger. I can
see where you are coming from, but in most of the code, a trigger is a
trigger and an event trigger is an event trigger. Let's not introduce
more terminology.

Other opinions on that?

Due to “platform” problems here tonight and the CF deadline, the
plpython patch is known not to pass regression tests on my machine. The
code is fully rebased and compiles without warning, though, so I'm still
entering it into this CF: hopefully I will figure out what's wrong with
my local plpython platform support here early next week.

For me, the plpython patch causes an (well, many) assertion failures in
the regression tests, because this change is wrong:

if (!found)
{
! /* Haven't found it, create a new cache entry */
! entry->proc = PLy_procedure_create(procTup, fn_oid,
! is_dml_trigger, is_evt_trigger);
if (use_cache)
entry->proc = proc;
}

When that is fixed, I get more failures and segfaults later.

Please give this another look. I'll review the Perl and Tcl things more
closely in the meantime.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Peter Eisentraut (#2)
Re: Completing PL support for Event Triggers

Peter Eisentraut wrote:

In the source code, I'm dubious about the use of is_dml_trigger. I can
see where you are coming from, but in most of the code, a trigger is a
trigger and an event trigger is an event trigger. Let's not introduce
more terminology.

Other opinions on that?

I'm with you.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Peter Eisentraut
peter_e@gmx.net
In reply to: Dimitri Fontaine (#1)
Re: Completing PL support for Event Triggers

Review of the PL/Tcl part: The functionality looks OK. There are some
cosmetic issues. If those are addressed, I think this can be committed.

In the documentation, "Event Triggers" -> "Event triggers".

For the example in the documentation, please show the output, that is,
what the trigger outputs.

Remove the extra space before " tclsnitch".

Document the return value (it is ignored). Will we need the return
value in a future expansion? Should we leave room for that?

Change "command trigger" to "event trigger" in several places.

compile_pltcl_function() does not catch trigger function being called as
event trigger or vice versa. Not sure if that should be covered.

The first PG_TRY block in pltcl_event_trigger_handler() is unnecessary,
because there is nothing in there that can throw an exception.

I'd remove some comments from pltcl_event_trigger_handler(). They were
obviously copied from pltcl_trigger_handler(), but that function is much
longer, so more comments might have been appropriate there.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Peter Eisentraut
peter_e@gmx.net
In reply to: Dimitri Fontaine (#1)
Re: Completing PL support for Event Triggers

On Fri, 2013-09-13 at 22:40 +0200, Dimitri Fontaine wrote:

Please find attached to this email three patches covering the missing PL
support for Event Triggers: pltcl, plperl and plpython.

For plperl, the previous reviews mostly apply analogously. In addition,
I have these specific points:

- In plperl_event_trigger_build_args(), the hv_ksplit() call is probably
unnecessary.

- plperl_call_perl_event_trigger_func and plperl_call_perl_trigger_func
as well as plperl_event_trigger_handler and plperl_trigger_handler have
a lot of overlap could perhaps be combined or refactored differently.

- In plperl_event_trigger_handler(), a point is being made about setting
up current_call_data before SPI_connect. Other handler functions don't
do this, though. It's not quite clear to me on first readong why it's
done differently here.

- Like I pointed out for the pltcl patch, calling trigger functions as
event triggers and vice versa is not detected in
compile_plperl_function, but I guess this can only happen if you
manually butcher the function after you have set up the trigger.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Peter Eisentraut (#4)
2 attachment(s)
Re: Completing PL support for Event Triggers

Hi,

Thanks for your review, sorry about the delays in answering, I've been
quite busy with other matters recently, it seems like things are going
to be smoother this week.

Peter Eisentraut <peter_e@gmx.net> writes:

Review of the PL/Tcl part: The functionality looks OK. There are some
cosmetic issues. If those are addressed, I think this can be committed.

In the documentation, "Event Triggers" -> "Event triggers".

Done in the attached v2 of the patch.

For the example in the documentation, please show the output, that is,
what the trigger outputs.

I checked and we don't do that for plpgsql… if you insist, I'll be happy
to prepare something though.

Remove the extra space before " tclsnitch".

Done in the attached.

Document the return value (it is ignored). Will we need the return
value in a future expansion? Should we leave room for that?

That's documented already in the main "Event Triggers" chapter of the
documentation, please see the following URL. Should we repeat the docs
in the per-PL chapters?

http://www.postgresql.org/docs/9.3/interactive/event-trigger-definition.html

Change "command trigger" to "event trigger" in several places.

Done.

compile_pltcl_function() does not catch trigger function being called as
event trigger or vice versa. Not sure if that should be covered.

It's not covered in the PLpgSQL parts, at least.

The first PG_TRY block in pltcl_event_trigger_handler() is unnecessary,
because there is nothing in there that can throw an exception.

Cleaned.

I'd remove some comments from pltcl_event_trigger_handler(). They were
obviously copied from pltcl_trigger_handler(), but that function is much
longer, so more comments might have been appropriate there.

Done

For plperl, the previous reviews mostly apply analogously. In addition,
I have these specific points:

- In plperl_event_trigger_build_args(), the hv_ksplit() call is probably
unnecessary.

Yeah, removed.

- plperl_call_perl_event_trigger_func and plperl_call_perl_trigger_func
as well as plperl_event_trigger_handler and plperl_trigger_handler have
a lot of overlap could perhaps be combined or refactored differently.

I didn't do that in the attached.

- In plperl_event_trigger_handler(), a point is being made about setting
up current_call_data before SPI_connect. Other handler functions don't
do this, though. It's not quite clear to me on first readong why it's
done differently here.

I don't know where that comes from. I know that without it (just tried)
the regression tests are all failing.

You introduced some new compiler warnings, please fix those.

http://pgci.eisentraut.org/jenkins/view/PostgreSQL/job/postgresql_commitfest_world/80/warnings19Result/new/?

I did that in the attached, thanks.

Is there a simple enough way to recompile changed files in -O0? To cover
the whole compiler warnings we want to catch, my experience is that the
only way is to reconfigure then recompile the whole tree with different
compiler optimisation targets, as the compiler then see different
things.

I didn't find a way to be able to run the compiler once and be done, and
it's really easy to forget redoing the whole tree just in case.

In the source code, I'm dubious about the use of is_dml_trigger. I can
see where you are coming from, but in most of the code, a trigger is a
trigger and an event trigger is an event trigger. Let's not introduce
more terminology.

Well, that's how it's been committer in PLpgSQL, and I kept that in the
others. In the attached, I used the terminology you seem to be proposing
here, that is is_trigger and is_event_trigger.

For me, the plpython patch causes an (well, many) assertion failures in
the regression tests, because this change is wrong:

I still need to review my python setup here to be able to build
something, I had problems with master even in a debian VM IIRC.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Attachments:

event_trigger_plperl.v2.patch.gzapplication/octet-streamDownload
event_trigger_pltcl.v2.patch.gzapplication/octet-streamDownload
#7Peter Eisentraut
peter_e@gmx.net
In reply to: Dimitri Fontaine (#6)
Re: Completing PL support for Event Triggers

I have committed the PL/Tcl part.

I'll work on the PL/Perl part next.

I believe we're still waiting on something from you for PL/Python.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Peter Eisentraut (#7)
Re: Completing PL support for Event Triggers

Peter Eisentraut <peter_e@gmx.net> writes:

I have committed the PL/Tcl part.
I'll work on the PL/Perl part next.

Thanks!

I believe we're still waiting on something from you for PL/Python.

Yes I still need to figure that one out.
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Peter Eisentraut
peter_e@gmx.net
In reply to: Dimitri Fontaine (#8)
1 attachment(s)
Re: Completing PL support for Event Triggers

I made one significant change in the PL/Perl patch. You had this in
plperl_event_trigger_handler():

+       /*
+        * Create the call_data before connecting to SPI, so that it is not
+        * allocated in the SPI memory context
+        */
+       current_call_data = (plperl_call_data *) palloc0(sizeof(plperl_call_data));
+       current_call_data->fcinfo = fcinfo;

I think this is wrong, and the reason it crashes if you remove it is
that you need to call increment_prodesc_refcount(prodesc), like in the
other handlers.

Attached is my "final" patch. Let me know if it's OK for you.

Attachments:

0001-PL-Perl-Add-event-trigger-support.patchtext/x-patch; charset=UTF-8; name=0001-PL-Perl-Add-event-trigger-support.patchDownload
>From 92f87f18db712697a273acd9443f77c4b2a83021 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Tue, 26 Nov 2013 06:45:57 -0500
Subject: [PATCH] PL/Perl: Add event trigger support

From: Dimitri Fontaine <dimitri@2ndQuadrant.fr>
---
 doc/src/sgml/plperl.sgml                  |  50 ++++++++++
 src/pl/plperl/expected/plperl_trigger.out |  35 +++++++
 src/pl/plperl/plperl.c                    | 148 +++++++++++++++++++++++++++---
 src/pl/plperl/sql/plperl_trigger.sql      |  20 ++++
 4 files changed, 242 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/plperl.sgml b/doc/src/sgml/plperl.sgml
index 10eac0e..34663e4 100644
--- a/doc/src/sgml/plperl.sgml
+++ b/doc/src/sgml/plperl.sgml
@@ -1211,6 +1211,56 @@ <title>PL/Perl Triggers</title>
   </para>
  </sect1>
 
+ <sect1 id="plperl-event-triggers">
+  <title>PL/Perl Event Triggers</title>
+
+  <para>
+   PL/Perl can be used to write event trigger functions.  In an event trigger
+   function, the hash reference <varname>$_TD</varname> contains information
+   about the current trigger event.  <varname>$_TD</> is a global variable,
+   which gets a separate local value for each invocation of the trigger.  The
+   fields of the <varname>$_TD</varname> hash reference are:
+
+   <variablelist>
+    <varlistentry>
+     <term><literal>$_TD-&gt;{event}</literal></term>
+     <listitem>
+      <para>
+       The name of the event the trigger is fired for.
+      </para>
+     </listitem>
+    </varlistentry>
+
+    <varlistentry>
+     <term><literal>$_TD-&gt;{tag}</literal></term>
+     <listitem>
+      <para>
+       The command tag for which the trigger is fired.
+      </para>
+     </listitem>
+    </varlistentry>
+   </variablelist>
+  </para>
+
+  <para>
+   The return value of the trigger procedure is ignored.
+  </para>
+
+  <para>
+   Here is an example of an event trigger function, illustrating some of the
+   above:
+<programlisting>
+CREATE OR REPLACE FUNCTION perlsnitch() RETURNS event_trigger AS $$
+  elog(NOTICE, "perlsnitch: " . $_TD->{event} . " " . $_TD->{tag} . " ");
+$$ LANGUAGE plperl;
+
+CREATE EVENT TRIGGER perl_a_snitch
+    ON ddl_command_start
+    EXECUTE PROCEDURE perlsnitch();
+</programlisting>
+  </para>
+ </sect1>
+
  <sect1 id="plperl-under-the-hood">
   <title>PL/Perl Under the Hood</title>
 
diff --git a/src/pl/plperl/expected/plperl_trigger.out b/src/pl/plperl/expected/plperl_trigger.out
index 181dcfa..36ecb92 100644
--- a/src/pl/plperl/expected/plperl_trigger.out
+++ b/src/pl/plperl/expected/plperl_trigger.out
@@ -309,3 +309,38 @@ $$ LANGUAGE plperl;
 SELECT direct_trigger();
 ERROR:  trigger functions can only be called as triggers
 CONTEXT:  compilation of PL/Perl function "direct_trigger"
+-- test plperl command triggers
+create or replace function perlsnitch() returns event_trigger language plperl as $$
+  elog(NOTICE, "perlsnitch: " . $_TD->{event} . " " . $_TD->{tag} . " ");
+$$;
+create event trigger perl_a_snitch on ddl_command_start
+   execute procedure perlsnitch();
+create event trigger perl_b_snitch on ddl_command_end
+   execute procedure perlsnitch();
+create or replace function foobar() returns int language sql as $$select 1;$$;
+NOTICE:  perlsnitch: ddl_command_start CREATE FUNCTION 
+CONTEXT:  PL/Perl function "perlsnitch"
+NOTICE:  perlsnitch: ddl_command_end CREATE FUNCTION 
+CONTEXT:  PL/Perl function "perlsnitch"
+alter function foobar() cost 77;
+NOTICE:  perlsnitch: ddl_command_start ALTER FUNCTION 
+CONTEXT:  PL/Perl function "perlsnitch"
+NOTICE:  perlsnitch: ddl_command_end ALTER FUNCTION 
+CONTEXT:  PL/Perl function "perlsnitch"
+drop function foobar();
+NOTICE:  perlsnitch: ddl_command_start DROP FUNCTION 
+CONTEXT:  PL/Perl function "perlsnitch"
+NOTICE:  perlsnitch: ddl_command_end DROP FUNCTION 
+CONTEXT:  PL/Perl function "perlsnitch"
+create table foo();
+NOTICE:  perlsnitch: ddl_command_start CREATE TABLE 
+CONTEXT:  PL/Perl function "perlsnitch"
+NOTICE:  perlsnitch: ddl_command_end CREATE TABLE 
+CONTEXT:  PL/Perl function "perlsnitch"
+drop table foo;
+NOTICE:  perlsnitch: ddl_command_start DROP TABLE 
+CONTEXT:  PL/Perl function "perlsnitch"
+NOTICE:  perlsnitch: ddl_command_end DROP TABLE 
+CONTEXT:  PL/Perl function "perlsnitch"
+drop event trigger perl_a_snitch;
+drop event trigger perl_b_snitch;
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index de8cb0e..4f5b92f 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -21,6 +21,7 @@
 #include "catalog/pg_language.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
+#include "commands/event_trigger.h"
 #include "commands/trigger.h"
 #include "executor/spi.h"
 #include "funcapi.h"
@@ -254,10 +255,13 @@
 
 static Datum plperl_func_handler(PG_FUNCTION_ARGS);
 static Datum plperl_trigger_handler(PG_FUNCTION_ARGS);
+static void plperl_event_trigger_handler(PG_FUNCTION_ARGS);
 
 static void free_plperl_function(plperl_proc_desc *prodesc);
 
-static plperl_proc_desc *compile_plperl_function(Oid fn_oid, bool is_trigger);
+static plperl_proc_desc *compile_plperl_function(Oid fn_oid,
+												 bool is_trigger,
+												 bool is_event_trigger);
 
 static SV  *plperl_hash_from_tuple(HeapTuple tuple, TupleDesc tupdesc);
 static SV  *plperl_hash_from_datum(Datum attr);
@@ -1610,6 +1614,23 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 }
 
 
+/* Set up the arguments for an event trigger call. */
+static SV  *
+plperl_event_trigger_build_args(FunctionCallInfo fcinfo)
+{
+	EventTriggerData *tdata;
+	HV		   *hv;
+
+	hv = newHV();
+
+	tdata = (EventTriggerData *) fcinfo->context;
+
+	hv_store_string(hv, "event", cstr2sv(tdata->event));
+	hv_store_string(hv, "tag", cstr2sv(tdata->tag));
+
+	return newRV_noinc((SV *) hv);
+}
+
 /* Set up the new tuple returned from a trigger. */
 
 static HeapTuple
@@ -1717,6 +1738,11 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 		current_call_data = &this_call_data;
 		if (CALLED_AS_TRIGGER(fcinfo))
 			retval = PointerGetDatum(plperl_trigger_handler(fcinfo));
+		else if (CALLED_AS_EVENT_TRIGGER(fcinfo))
+		{
+			plperl_event_trigger_handler(fcinfo);
+			retval = (Datum) 0;
+		}
 		else
 			retval = plperl_func_handler(fcinfo);
 	}
@@ -1853,7 +1879,8 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 	Oid		   *argtypes;
 	char	  **argnames;
 	char	   *argmodes;
-	bool		istrigger = false;
+	bool		is_trigger = false;
+	bool		is_event_trigger = false;
 	int			i;
 
 	/* Get the new function's pg_proc entry */
@@ -1865,13 +1892,15 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 	functyptype = get_typtype(proc->prorettype);
 
 	/* Disallow pseudotype result */
-	/* except for TRIGGER, RECORD, or VOID */
+	/* except for TRIGGER, EVTTRIGGER, RECORD, or VOID */
 	if (functyptype == TYPTYPE_PSEUDO)
 	{
 		/* we assume OPAQUE with no arguments means a trigger */
 		if (proc->prorettype == TRIGGEROID ||
 			(proc->prorettype == OPAQUEOID && proc->pronargs == 0))
-			istrigger = true;
+			is_trigger = true;
+		else if (proc->prorettype == EVTTRIGGEROID)
+			is_event_trigger = true;
 		else if (proc->prorettype != RECORDOID &&
 				 proc->prorettype != VOIDOID)
 			ereport(ERROR,
@@ -1898,7 +1927,7 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 	/* Postpone body checks if !check_function_bodies */
 	if (check_function_bodies)
 	{
-		(void) compile_plperl_function(funcoid, istrigger);
+		(void) compile_plperl_function(funcoid, is_trigger, is_event_trigger);
 	}
 
 	/* the result of a validator is ignored */
@@ -2169,6 +2198,63 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 }
 
 
+static void
+plperl_call_perl_event_trigger_func(plperl_proc_desc *desc,
+									FunctionCallInfo fcinfo,
+									SV *td)
+{
+	dSP;
+	SV		   *retval,
+			   *TDsv;
+	int			count;
+
+	ENTER;
+	SAVETMPS;
+
+	TDsv = get_sv("main::_TD", 0);
+	if (!TDsv)
+		elog(ERROR, "couldn't fetch $_TD");
+
+	save_item(TDsv);			/* local $_TD */
+	sv_setsv(TDsv, td);
+
+	PUSHMARK(sp);
+	PUTBACK;
+
+	/* Do NOT use G_KEEPERR here */
+	count = perl_call_sv(desc->reference, G_SCALAR | G_EVAL);
+
+	SPAGAIN;
+
+	if (count != 1)
+	{
+		PUTBACK;
+		FREETMPS;
+		LEAVE;
+		elog(ERROR, "didn't get a return item from trigger function");
+	}
+
+	if (SvTRUE(ERRSV))
+	{
+		(void) POPs;
+		PUTBACK;
+		FREETMPS;
+		LEAVE;
+		/* XXX need to find a way to assign an errcode here */
+		ereport(ERROR,
+				(errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV)))));
+	}
+
+	retval = newSVsv(POPs);
+	(void) retval;				/* silence compiler warning */
+
+	PUTBACK;
+	FREETMPS;
+	LEAVE;
+
+	return;
+}
+
 static Datum
 plperl_func_handler(PG_FUNCTION_ARGS)
 {
@@ -2181,7 +2267,7 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 	if (SPI_connect() != SPI_OK_CONNECT)
 		elog(ERROR, "could not connect to SPI manager");
 
-	prodesc = compile_plperl_function(fcinfo->flinfo->fn_oid, false);
+	prodesc = compile_plperl_function(fcinfo->flinfo->fn_oid, false, false);
 	current_call_data->prodesc = prodesc;
 	increment_prodesc_refcount(prodesc);
 
@@ -2295,7 +2381,7 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 		elog(ERROR, "could not connect to SPI manager");
 
 	/* Find or compile the function */
-	prodesc = compile_plperl_function(fcinfo->flinfo->fn_oid, true);
+	prodesc = compile_plperl_function(fcinfo->flinfo->fn_oid, true, false);
 	current_call_data->prodesc = prodesc;
 	increment_prodesc_refcount(prodesc);
 
@@ -2386,6 +2472,45 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 }
 
 
+static void
+plperl_event_trigger_handler(PG_FUNCTION_ARGS)
+{
+	plperl_proc_desc *prodesc;
+	SV		   *svTD;
+	ErrorContextCallback pl_error_context;
+
+	/* Connect to SPI manager */
+	if (SPI_connect() != SPI_OK_CONNECT)
+		elog(ERROR, "could not connect to SPI manager");
+
+	/* Find or compile the function */
+	prodesc = compile_plperl_function(fcinfo->flinfo->fn_oid, false, true);
+	current_call_data->prodesc = prodesc;
+	increment_prodesc_refcount(prodesc);
+
+	/* Set a callback for error reporting */
+	pl_error_context.callback = plperl_exec_callback;
+	pl_error_context.previous = error_context_stack;
+	pl_error_context.arg = prodesc->proname;
+	error_context_stack = &pl_error_context;
+
+	activate_interpreter(prodesc->interp);
+
+	svTD = plperl_event_trigger_build_args(fcinfo);
+	plperl_call_perl_event_trigger_func(prodesc, fcinfo, svTD);
+
+	if (SPI_finish() != SPI_OK_FINISH)
+		elog(ERROR, "SPI_finish() failed");
+
+	/* Restore the previous error callback */
+	error_context_stack = pl_error_context.previous;
+
+	SvREFCNT_dec(svTD);
+
+	return;
+}
+
+
 static bool
 validate_plperl_function(plperl_proc_ptr *proc_ptr, HeapTuple procTup)
 {
@@ -2437,7 +2562,7 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 
 
 static plperl_proc_desc *
-compile_plperl_function(Oid fn_oid, bool is_trigger)
+compile_plperl_function(Oid fn_oid, bool is_trigger, bool is_event_trigger)
 {
 	HeapTuple	procTup;
 	Form_pg_proc procStruct;
@@ -2543,7 +2668,7 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 		 * Get the required information for input conversion of the
 		 * return value.
 		 ************************************************************/
-		if (!is_trigger)
+		if (!is_trigger && !is_event_trigger)
 		{
 			typeTup =
 				SearchSysCache1(TYPEOID,
@@ -2562,7 +2687,8 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 				if (procStruct->prorettype == VOIDOID ||
 					procStruct->prorettype == RECORDOID)
 					 /* okay */ ;
-				else if (procStruct->prorettype == TRIGGEROID)
+				else if (procStruct->prorettype == TRIGGEROID ||
+						 procStruct->prorettype == EVTTRIGGEROID)
 				{
 					free_plperl_function(prodesc);
 					ereport(ERROR,
@@ -2598,7 +2724,7 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
 		 * Get the required information for output conversion
 		 * of all procedure arguments
 		 ************************************************************/
-		if (!is_trigger)
+		if (!is_trigger && !is_event_trigger)
 		{
 			prodesc->nargs = procStruct->pronargs;
 			for (i = 0; i < prodesc->nargs; i++)
diff --git a/src/pl/plperl/sql/plperl_trigger.sql b/src/pl/plperl/sql/plperl_trigger.sql
index c43b31e..a375b40 100644
--- a/src/pl/plperl/sql/plperl_trigger.sql
+++ b/src/pl/plperl/sql/plperl_trigger.sql
@@ -169,3 +169,23 @@ CREATE FUNCTION direct_trigger() RETURNS trigger AS $$
 $$ LANGUAGE plperl;
 
 SELECT direct_trigger();
+
+-- test plperl command triggers
+create or replace function perlsnitch() returns event_trigger language plperl as $$
+  elog(NOTICE, "perlsnitch: " . $_TD->{event} . " " . $_TD->{tag} . " ");
+$$;
+
+create event trigger perl_a_snitch on ddl_command_start
+   execute procedure perlsnitch();
+create event trigger perl_b_snitch on ddl_command_end
+   execute procedure perlsnitch();
+
+create or replace function foobar() returns int language sql as $$select 1;$$;
+alter function foobar() cost 77;
+drop function foobar();
+
+create table foo();
+drop table foo;
+
+drop event trigger perl_a_snitch;
+drop event trigger perl_b_snitch;
-- 
1.8.4.3

#10Peter Eisentraut
peter_e@gmx.net
In reply to: Peter Eisentraut (#9)
Re: Completing PL support for Event Triggers

On Tue, 2013-11-26 at 06:59 -0500, Peter Eisentraut wrote:

Attached is my "final" patch. Let me know if it's OK for you.

Dimitri, will you have a change to review this?

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Peter Eisentraut (#9)
Re: Completing PL support for Event Triggers

Peter Eisentraut <peter_e@gmx.net> writes:

I think this is wrong, and the reason it crashes if you remove it is
that you need to call increment_prodesc_refcount(prodesc), like in the
other handlers.

Thanks, looks like this indeed.

Attached is my "final" patch. Let me know if it's OK for you.

It looks like you started with the v1 of the plperl patch rather than
the v2, where the only difference is in only using is_trigger or using
both is_trigger and is_event_trigger. Your version currently uses both
where I though we chose using is_trigger only.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Peter Eisentraut
peter_e@gmx.net
In reply to: Dimitri Fontaine (#11)
Re: Completing PL support for Event Triggers

On Mon, 2013-12-09 at 09:45 +0100, Dimitri Fontaine wrote:

It looks like you started with the v1 of the plperl patch rather than
the v2, where the only difference is in only using is_trigger or using
both is_trigger and is_event_trigger. Your version currently uses both
where I though we chose using is_trigger only.

I think you are mistaken. My patch includes all changes between your v1
and v2 patch.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Peter Eisentraut (#12)
Re: Completing PL support for Event Triggers

Peter Eisentraut <peter_e@gmx.net> writes:

I think you are mistaken. My patch includes all changes between your v1
and v2 patch.

I mistakenly remembered that we did remove all the is_event_trigger
business from the plperl patch too, when it's not the case. Sorry about
this confusion.

My vote is for “ready for commit” then.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Peter Eisentraut
peter_e@gmx.net
In reply to: Dimitri Fontaine (#13)
Re: Completing PL support for Event Triggers

On 12/11/13, 5:06 AM, Dimitri Fontaine wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

I think you are mistaken. My patch includes all changes between your v1
and v2 patch.

I mistakenly remembered that we did remove all the is_event_trigger
business from the plperl patch too, when it's not the case. Sorry about
this confusion.

My vote is for “ready for commit” then.

PL/Perl was committed.

Please update the commit fest entry with your plans about PL/Python.
(Returned with Feedback or move to next CF or close and create a
separate entry?)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers