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

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

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> 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.

Currently Event Triggers are disabled in single user mode, in parts
because operating them require accessing to a catalog index, which might
be corrupted and the reason why you're in single user mode in the first
place.

So please keep your new event trigger disabled in single user mode.

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=cd3413ec3683918c9cb9cfb39ae5b2c32f231e8b

Regards, and thanks for this new Event Trigger!
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Mon, Nov 18, 2013 at 9:39 AM, Ian Lawrence Barwick <barwick(at)gmail(dot)com> wrote:
> 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.

Ending up in a failed transaction block would be wrong. If the user
does a BEGIN, a bunch of stuff, and a COMMIT, they're entitled to
assume without checking that they are no longer in a transaction
block. The COMMIT may have actually performed a ROLLBACK, but one way
or the other the transaction block will have ended. This is important
for things like psql <
my-dumb-script-with-several-begin-commit-blocks.

It is a little less clear whether it's best for the COMMIT to return
an ERROR message or something else, but I think the ERROR is probably
the best solution. There is already commit-time code that can fail
today, so there should be precedent here. And I suspect anything
other than ERROR will be really messy to implement.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/19/2013 10:58 AM, Robert Haas wrote:
> On Mon, Nov 18, 2013 at 9:39 AM, Ian Lawrence Barwick <barwick(at)gmail(dot)com> wrote:
>> 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.
> Ending up in a failed transaction block would be wrong. If the user
> does a BEGIN, a bunch of stuff, and a COMMIT, they're entitled to
> assume without checking that they are no longer in a transaction
> block. The COMMIT may have actually performed a ROLLBACK, but one way
> or the other the transaction block will have ended. This is important
> for things like psql <
> my-dumb-script-with-several-begin-commit-blocks.
>
> It is a little less clear whether it's best for the COMMIT to return
> an ERROR message or something else, but I think the ERROR is probably
> the best solution. There is already commit-time code that can fail
> today, so there should be precedent here. And I suspect anything
> other than ERROR will be really messy to implement.
>

OK, you've convinced me.

cheers

andrew

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Nov 18, 2013 at 9:39 AM, Ian Lawrence Barwick <barwick(at)gmail(dot)com> wrote:
>> 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.

> Ending up in a failed transaction block would be wrong. If the user
> does a BEGIN, a bunch of stuff, and a COMMIT, they're entitled to
> assume without checking that they are no longer in a transaction
> block.

Absolutely. There are plenty of ways to fail at COMMIT already,
eg deferred foreign key constraints. This shouldn't act any
different.

regards, tom lane

2013/11/20 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Nov 18, 2013 at 9:39 AM, Ian Lawrence Barwick <barwick(at)gmail(dot)com> wrote:
>>> 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.
>
>> Ending up in a failed transaction block would be wrong. If the user
>> does a BEGIN, a bunch of stuff, and a COMMIT, they're entitled to
>> assume without checking that they are no longer in a transaction
>> block.
>
> Absolutely. There are plenty of ways to fail at COMMIT already,
> eg deferred foreign key constraints. This shouldn't act any
> different.

Ah OK, I see how that works. Thanks for the explanation.

Ian Barwick