Re: [HACKERS] Completing PL support for Event Triggers
On 12/11/13, 5:06 AM, Dimitri Fontaine wrote: > Peter Eisentraut 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
Peter Eisentraut 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 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 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 Date: Tue, 26 Nov 2013 06:45:57 -0500 Subject: [PATCH] PL/Perl: Add event trigger support From: Dimitri Fontaine --- 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 @@ PL/Perl Triggers + + PL/Perl Event Triggers + + + PL/Perl can be used to write event trigger functions. In an event trigger + function, the hash reference $_TD contains information + about the current trigger event. $_TD is a global variable, + which gets a separate local value for each invocation of the trigger. The + fields of the $_TD hash reference are: + + + + $_TD->{event} + + + The name of the event the trigger is fired for. + + + + + + $_TD->{tag} + + + The command tag for which the trigger is fired. + + + + + + + + The return value of the trigger procedure is ignored. + + + + Here is an example of an event trigger function, illustrating some of the + above: + +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(); + + + + PL/Perl Under the Hood 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_func
Re: [HACKERS] Completing PL support for Event Triggers
Peter Eisentraut 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 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