Re: 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

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(at)gmx(dot)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

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(at)gmx(dot)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.

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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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?)