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(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