Thread: 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-committrigger 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@dunslane.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@gmail.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@gmail.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@gmail.com> writes:
> On Mon, Nov 18, 2013 at 9:39 AM, Ian Lawrence Barwick <barwick@gmail.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@sss.pgh.pa.us>: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Nov 18, 2013 at 9:39 AM, Ian Lawrence Barwick <barwick@gmail.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