Review: pre-commit triggers

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.

Submission review
-----------------
* Is the patch in a patch format which has context?
Yes

* Does it apply cleanly to the current git master?
Yes. No new files are introduced by this patch.

* 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).

Usability review
----------------
* Does the patch actually implement that?
Yes.

* Do we want that?
There is an item in the todo-list "Add database and transaction-level triggers";
the linked thread:

http://archives.postgresql.org/pgsql-hackers/2008-05/msg00620.php

from 2008 doesn't seem too receptive to the idea, but this time round
there don't seem to be any particular objections. Personally I don't
have a use-case but it certainly looks like a useful compatibility
feature when porting from other databases. Andrew mentions
porting from Firebird; for reference this is the relevant Firebird
documentation:

http://www.firebirdsql.org/refdocs/langrefupd21-ddl-trigger.html

* Do we already have it?
No.

* Does it follow SQL spec, or the community-agreed behavior?
Not sure about the SQL spec. The "Compatibility" section of the
CREATE TRIGGER doc page doesn't mention anything along these lines.

http://www.postgresql.org/docs/9.3/static/sql-createtrigger.html#SQL-CREATETRIGGER-COMPATIBILITY

* Does it include pg_dump support (if applicable)?
Yes

Feature test
------------

* Does the feature work as advertised?
Yes.

* 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.

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.

* Are there any assertion failures or crashes?
No.

Performance review
------------------

* Does the patch slow down simple tests?
No.

* If it claims to improve performance, does it?
n/a

* Does it slow down other things?
No.

Coding review
-------------

* Does it follow the project coding guidelines?
Yes.

* Are there portability issues?
I don't see any.

* Will it work on Windows/BSD etc?
Tested on OS X and Linux. I don't see anything, e.g. system calls, which
might prevent it working on Windows.

* Does it do what it says, correctly?
As far as I can tell, yes.

* Does it produce compiler warnings?
No.

* Can you make it crash?
So far, no.

Architecture review
-------------------

* Is everything done in a way that fits together coherently with other
features/modules?
The changes are not all that complex and I don't see any issues, however
I'm not very familiar with that area of the code (but hey, that's why I'm
taking a look) so I might be missing something.

* Are there interdependencies that can cause problems?
I can't see any.

Additional notes
----------------

A sample transaction commit trigger:

CREATE OR REPLACE FUNCTION pre_commit_trigger()
RETURNS EVENT_TRIGGER
LANGUAGE 'plpgsql'
AS $$
BEGIN
RAISE NOTICE 'Pre-commit trigger called';
END;
$$;

CREATE EVENT TRIGGER trg_pre_commit ON transaction_commit
EXECUTE PROCEDURE pre_commit_trigger();

Some relevant links:

http://www.postgresql.org/docs/9.3/interactive/event-triggers.html
http://www.postgresql.org/docs/9.3/interactive/sql-createeventtrigger.html
http://www.postgresql.org/docs/9.3/interactive/sql-prepare-transaction.html
http://en.wikipedia.org/wiki/Database_trigger

Regards

Ian Barwick