Thread: 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(-) 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
Attachment
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.
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
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.
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.
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
Attachment
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.
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
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.
Attachment
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?
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
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.
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
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?)