Re: [HACKERS] Completing PL support for Event Triggers
Peter Eisentraut pete...@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
Re: [HACKERS] Completing PL support for Event Triggers
On 12/11/13, 5:06 AM, Dimitri Fontaine wrote: Peter Eisentraut pete...@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
Re: [HACKERS] 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
Re: [HACKERS] Completing PL support for Event Triggers
Peter Eisentraut pete...@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
Re: [HACKERS] 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
Re: [HACKERS] 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.
From 92f87f18db712697a273acd9443f77c4b2a83021 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut pete...@gmx.net
Date: Tue, 26 Nov 2013 06:45:57 -0500
Subject: [PATCH] PL/Perl: Add event trigger support
From: Dimitri Fontaine dimi...@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 @@ titlePL/Perl Triggers/title
/para
/sect1
+ sect1 id=plperl-event-triggers
+ titlePL/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
+ termliteral$_TD-gt;{event}/literal/term
+ listitem
+ para
+ The name of the event the trigger is fired for.
+ /para
+ /listitem
+/varlistentry
+
+varlistentry
+ termliteral$_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
titlePL/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
Re: [HACKERS] Completing PL support for Event Triggers
Peter Eisentraut pete...@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
Re: [HACKERS] 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
Re: [HACKERS] 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 pete...@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 event_trigger_plperl.v2.patch.gz Description: Binary data event_trigger_pltcl.v2.patch.gz Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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
Re: [HACKERS] 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
Re: [HACKERS] 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 Herrerahttp://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
Re: [HACKERS] 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
[HACKERS] Completing PL support for Event Triggers
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(-) perl4 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 event_trigger_plperl.v1.patch.gz Description: Binary data event_trigger_plpython.v1.patch.gz Description: Binary data event_trigger_pltcl.v1.patch.gz Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
