Regression in handling of db access failures

[Steffen Hoffmann]

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.

[Christian Boos]

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.

[Remy Blank]

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.

[Christian Boos]

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.

[Remy Blank]

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

[Steffen Hoffmann]

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.

[Steffen Hoffmann]

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.

[osimons]

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.

[osimons]

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

[Steffen Hoffmann]

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.

[Remy Blank]

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

[Remy Blank]

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?

[Christian Boos]

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

[Remy Blank]

Thanks for reviewing. Patch applied in [11072].

[Steffen Hoffmann]

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

Since​r12069 ​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.