Edgewall Software
Modify

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#10451 closed defect (fixed)

Regression in handling of db access failures

Reported by: Steffen Hoffmann Owned by: Remy Blank
Priority: normal Milestone: 1.0
Component: database backend Version: 0.13dev
Severity: normal Keywords: database read-only rollback
Cc: Branch:
Release Notes:

More robust environment upgrades by using a new db connection for each upgrade participant.

API Changes:
Internal Changes:

Description

In some situations a .rollback() method is required for sane db handling even on read-only connections, but it has been removed in r10179.

This applies i.e. to the IEnvironmentSetupParticipant method environment_needs_upgrade() and blocks current th:wiki:TagsPlugin from initial db table creation. As pointed out by Remy only the removal of .commit() is clearly satisfied for a read-only db connection, and .rollback() should better be restored.

Attachments (1)

10451-upgrade-connections-r11054.patch (1.2 KB ) - added by Remy Blank 8 years ago.
Use a separate DB connections for each setup participant when checking if an upgrade is required.

Download all attachments as: .zip

Change History (15)

comment:1 by Christian Boos, 9 years ago

There shouldn't be a need for calling .rollback() on error explicitly. This should be taken care by the backend itself if needed, see PyFormatCursor._rollback_on_error for the SQLite backend.

If it's missing for other backends, we should also take care about this in trac.db, in order to keep the semantic of our DB API clean:

  • with env.db_transaction ⇒ read-write connection; there's an automatic commit() or rollback() at the end of a transaction, depending on its success
  • with env.db_query ⇒ read-only connection; there's an automatic rollback() in case of failure if the backend requires it

We should indeed check that it's done correctly for MySQL and PostgreSQL. If all backend need this, then we could do that rollback at the level of QueryContextManager.__exit__.

IIRC, keeping commit() and rollback() methods at the level of the rw connection was essentially for backward compatibility reasons. We might add rollback() for the same reason, but its use should be discouraged.

comment:2 by Remy Blank, 9 years ago

The issue is the following:

  • In environment_needs_upgrade(), we have a single with block to get a read-only transaction.
  • In that block, we iterate over all components and pass the same connection.
  • In one plugin (th:TagsPlugin), the check to find out if an upgrade is necessary is to "SELECT COUNT(*) FROM tags", and to catch the exception. If the exception is thrown, IIRC on some backends (PostgreSQL?) you have to roll back before you can issue any new statements. So this breaks all the components coming after th:TagsPlugin.

On second thought, I'm not sure anymore that re-adding rollback() is the right solution either. Maybe we should just include the with block in the loop. Also, SELECTing for a table that doesn't exist is probably bad practice.

in reply to:  2 ; comment:3 by Christian Boos, 9 years ago

Replying to rblank:

… IIRC on some backends (PostgreSQL?) you have to roll back before you can issue any new statements. So this breaks all the components coming after th:TagsPlugin.

Right, I just verified and a query in PostgreSQL executed right after a failed select * from nonexistingtable (and probably any other kind of error) fails as well with:

InternalError: current transaction is aborted, commands ignored until end of transaction block

Sounds familiar indeed ;-)

On second thought, I'm not sure anymore that re-adding rollback() is the right solution either. Maybe we should just include the with block in the loop.

Now that I remember this a bit better, I think we used to add a .rollback() in such places always, as it wouldn't harm even for backends which don't require this (MySQL?).

So I think we should really add it this in QueryContextManager.__exit__, and well, for the sake of backward compatibility, allow an explicit rollback() at least for 0.13.

Also, SELECTing for a table that doesn't exist is probably bad practice.

Both MySQL and PostgreSQL support:

SELECT count(*) FROM information_schema.tables WHERE table_schema=%s AND table_name=%s

(see the reset methods in tags/trac-0.12.2/trac/test.py).

SQLite has:

SELECT count(*) FROM sqlite_master WHERE type='table' AND tbl_name=%s

We could wrap these in a Connection table_exists method.

in reply to:  3 ; comment:4 by Remy Blank, 9 years ago

Milestone: 0.13

Replying to cboos:

So I think we should really add it this in QueryContextManager.__exit__, and well, for the sake of backward compatibility, allow an explicit rollback() at least for 0.13.

Yes, let's do that. Unfortunately, intentional database errors and rollbacks interfere badly with nested transaction blocks, and there's not much we can do about it.

We could wrap these in a Connection table_exists method.

I'm not sure we should encourage this practice. I would rather encourage plugin authors to have a database version number for their schema, in the same way as we do for core (#8172).

in reply to:  4 comment:5 by Steffen Hoffmann, 9 years ago

Replying to rblank:

Replying to cboos:

So I think we should really add it this in QueryContextManager.__exit__, and well, for the sake of backward compatibility, allow an explicit rollback() at least for 0.13.

Yes, let's do that. Unfortunately, intentional database errors and rollbacks interfere badly with nested transaction blocks, and there's not much we can do about it.

So this should be explicitly discouraged then. At least don't do that inside such interface methodes that involve iteration over a number of existing implementation.

Related question: Would it be safe to do such intrusive checks on plugin load i.e. in the __init__() method of the class that implements IEnvironmentSetupParticipant?

We could wrap these in a Connection table_exists method.

I'm not sure we should encourage this practice. I would rather encourage plugin authors to have a database version number for their schema, in the same way as we do for core (#8172).

Sure. This is common, but astonishingly not followed by this rather prominent plugin. I'll aim at implementing this, but will need to retain the "ugly" checks somewhere for late upgrades from older versions.

comment:6 by Steffen Hoffmann, 9 years ago

With th:changeset:11041 I've added a workaround.

Comments welcome. Nevertheless I support an explicit policy to explicitly discouraged intentional db connection errors in favor of storing (plugin) schema versions inside the Trac db (table system). I'll change th:TagsPlugin accordingly too.

comment:7 by osimons, 9 years ago

Looking at the trac-hacks changeset, would it not be sufficient to remove the line and not call db.rollback() at all? Just log the error, and then just call raise to propagate the error up the chain and let Trac upgrade routine deal with the error? It should do the necessary handling according to whatever logic exists at the version in use.

Last edited 9 years ago by osimons (previous) (diff)

comment:8 by osimons, 9 years ago

(Note: My comment was not intended as a general comment about the need for db.rollback(), it was just specific to the environment upgrade handling where Trac has always handled the transaction commit or rollback as needed depending on success of participants).

comment:9 by Steffen Hoffmann, 9 years ago

I got the impression, that the specific case of an intended db connection error to indicate no need for an upgrade wasn't/isn't handled that well now, but I've not tested it either.

by Remy Blank, 8 years ago

Use a separate DB connections for each setup participant when checking if an upgrade is required.

in reply to:  4 comment:10 by Remy Blank, 8 years ago

Replying to rblank:

Replying to cboos:

So I think we should really add it this in QueryContextManager.__exit__, and well, for the sake of backward compatibility, allow an explicit rollback() at least for 0.13.

Yes, let's do that. Unfortunately, intentional database errors and rollbacks interfere badly with nested transaction blocks, and there's not much we can do about it.

In the meantime, I'm convinced that we shouldn't do that. There's only one known affected plugin, which now has a workaround. So I would prefer discouraging the use of DB exceptions to "check stuff".

To isolate the plugins against each other somewhat, I suggest something like 10451-upgrade-connections-r11054.patch, which uses a separate DB connection for each call to environment_needs_upgrade(). So a plugin generating a DB exception won't affect all the others. I believe this would also have solved the issue with the TagsPlugin.

Ok to apply?

comment:11 by Christian Boos, 8 years ago

Yep, looks good! Such connections should be immediately reused (if pooled), so there should be no performance penalty in the general case.

comment:12 by Remy Blank, 8 years ago

Resolution: fixed
Status: newclosed

Thanks for reviewing. Patch applied in [11072].

comment:13 by Christian Boos, 8 years ago

Release Notes: modified (diff)

in reply to:  4 comment:14 by Steffen Hoffmann, 8 years ago

Replying to rblank:

Replying to cboos:

We could wrap these in a Connection table_exists method.

I'm not sure we should encourage this practice. I would rather encourage plugin authors to have a database version number for their schema, in the same way as we do for core (#8172).

Sincer12069 th:TagsPlugin finally uses version numbers for it's schema. Db table existence checks are done now only a few times during initial install/upgrade to current schema version, and possibly remaining side-effects of db errors have been minimized by using separate connections for these ugly tests.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.