Multirepos Databse Upgrade from 0.11 Fails w/ PostgreSQL

[jhampton]

Attempting to upgrade from 0.11-stable (r8161) to MultiRepos trunk (r8380) fails during trac-admin env upgrade

Issue is located at upgrades/db23.py#L6​. If the repository table doesn't exist (which it doesn't due to upgrading from 0.11), then the select fails. However, on PostgreSQL, this leaves the cursor in a state requiring a rollback. Since there is no rollback in the execept block, then it fails with:

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

The real kicker here, however, is that if you simply put a rollback in the except block, then you knock out the upgrades done in upgrades/db22.py​ which means the cache table never gets created. Thus leaving trac in a broken state.

It seems as though the purpose of the try/except block in do_upgrade​ is simply meant to replay upgrades/db22.py​. This doesn't make sense. Why would the database base upgrade need to be re-run? Anyway, I don't have a good idea on how to fix this, other than eliminating the try/except block such that it's unnecessary.

[cboos]

The reason why the code is like it is was clearly explained by the comments.

But you're right, I didn't expect the failed "SELECT" to leave the connection in an unusable state, so it's broken for PostgreSQL at least.

When moving on trunk, all this "compatibility" code due to the db22.py to db23.py rename that happened at one point during the development will be removed and this issue will go away.

Btw. Remy, what about removing this code now?

[rblank]

Replying to cboos:

Btw. Remy, what about removing this code now?

Sure, go ahead.

[jhampton]

Replying to cboos:

The reason why the code is like it is was clearly explained by the comments.

I did read the comment. What I meant to say is that I don't understand why one would ever replay a db upgrade. If something was missed in the first db upgrade, then simply create another one, etc.

But you're right, I didn't expect the failed "SELECT" to leave the connection in an unusable state, so it's broken for PostgreSQL at least.

With PostgreSQL, if ever a query fails, ie, you have to use a try/except then the cursor is in a bad state and must be rolled back. (Un?)fortunately, SQLite does not have this issue. I also don't know about MySQL.

Btw. Remy, what about removing this code now?

FYI, I have tested the removing of the code and it does work. So removal is a +1 from me.

[rblank]

Replying to jhampton:

I did read the comment. What I meant to say is that I don't understand why one would ever replay a db upgrade. If something was missed in the first db upgrade, then simply create another one, etc.

This was due to having two DB upgrades in parallel, one on multirepos and one on trunk. At some point, db23 was called db22 on multirepos, and it had to be renamed to db23 after db22 was created on trunk. So there were some multirepos installations that already had db23 applied (as db22), but needed the new db22. The check was supposed to catch that case.

[cboos]

"Buggy" compatibility code removed in r8395.

[cboos]

I think we can close the completed MultiRepos tickets. As soon as we have the separate 0.12-multirepos milestone, as discussed yesterday in #1507, we'll be able to distinguish what's closed on trunk and what's closed on the branch.