On 11/18/2013 09:39 AM, Ian Lawrence Barwick wrote:
> Review for Andrew Dunstan's patch in CF 2013-11:
>
> https://commitfest.postgresql.org/action/patch_view?id=1312
>
> This review is more from a usage point of view, I would like
> to spend more time looking at the code but only so many hours in a day,
> etcetera; I hope this is useful as-is.
Many thanks for the fast review.
>
> * Does it include reasonable tests, necessary doc patches, etc?
> Documentation patches included, but no tests. (I have a feeling it
> might be necessary to add a FAQ somewhere as to why there's
> no transaction rollback trigger).
I'll add some tests very shortly, and see about adding that to the docs.
> * Are there corner cases the author has failed to consider?
>
> I'm not sure whether it's intended behaviour, but if the
> commit trigger itself fails, there's an implicit rollback, e.g.:
>
> postgres=# BEGIN ;
> BEGIN
> postgres=*# INSERT INTO foo (id) VALUES (1);
> INSERT 0 1
> postgres=*# COMMIT ;
> NOTICE: Pre-commit trigger called
> ERROR: relation "bar" does not exist
> LINE 1: SELECT foo FROM bar
> ^
> QUERY: SELECT foo FROM bar
> CONTEXT: PL/pgSQL function pre_commit_trigger() line 4 at EXECUTE statement
> postgres=#
>
> I'd expect this to lead to a failed transaction block,
> or at least some sort of notice that the transaction itself
> has been rolled back.
I'll check on this.
>
> Note that 'DROP FUNCTION broken_trigger_function() CASCADE' will remove
> the broken function without having to resort to single user mode so
> it doesn't seem like an error in the commit trigger itself will
> necessarily lead to an intractable situation.
Given that, do we want to keep the bar on these operating in single user
mode? I can easily drop it and just document this way out of difficulty.
cheers
andrew
