refactor transaction handling

[Christian Boos]

Since the Trac 0.9 days, most of the data model methods are taking a db argument defaulting to None, with the convention that if None, the method should commit the transaction itself, otherwise it is just performing a part in the scope of a wider transaction.

Now, the logic above has been implemented explicitly in each method, with the help of flags (e.g. "handle_ta") and explicit calls to commit.

Not only this is verbose, but doing a rollback on failure was often missing.

The context managers and the with syntax introduced in Python 2.5 are ideally suited to handle such situations. As we still have to support Python 2.4, a slightly less optimal approach can nevertheless be work using a decorator.

The following mail has a proof of concept patch which is the result of some design discussion that happened on Trac-dev: ​googlegroups:trac-dev:c1f97c626f972010

This ticket will be used to track the progresses of a refactoring branch where we'll generalize this approach: sandbox/8751-with_transaction

[Christian Boos]

Patch above applied in r8671.

[shookie@…]

I started working on refactoring the handle_ta occurrences in the source, and first I turned to removing them from the wiki subtree completely. The only remaining handle_ta there was in the WikiAdmin import_page method. And this method raised some questions:

It makes checks reading the database, and uses return values to propagate the results of those checks. Now, the decorator wrapped transaction can't return without doing a commit prematurely. So we have 3 options how to deal with this:

1. pull the db checks out of the transaction, which avoids unnecessary commits in the case of db=None and a failed check, but also requires you to pull the db_cnx twice in this case.

2. leave the checks in the transaction, which requires you to propagate the check results with an array, does unneccessary commits in the case of db = none, but avoids pulling a new db_cnx twice in that case

        rvalue = [True]

        @with_transaction(self.env, db)
        def do_import(db):
            cursor = db.cursor()
            cursor.execute("SELECT text FROM wiki WHERE name=%s "
                           "ORDER BY version DESC LIMIT 1",
                           (title,))
            old = list(cursor)
            if old and title in create_only:
                printout('  %s already exists.' % title)
                rvalue[0] = False
                return
            if old and data == old[0][0]:
                printout('  %s already up to date.' % title)
                rvalue[0] = False
                return
            cursor.execute("INSERT INTO wiki"
                           "(version,name,time,author,ipnr,text) "
                           " SELECT 1+COALESCE(max(version),0),%s,%s,"
                           " 'trac','127.0.0.1',%s FROM wiki "
                           " WHERE name=%s",
                           (title, int(time.time()), data, title))
            if not old:
                WikiSystem(self.env).pages.invalidate(db)
                
        return rvalue[0]

3. Just leave it as it is and set a try block around the db updates, and do rollbacks if something goes wrong and handle_ta is true, which avoids all the problems above, but doesn't use our new transaction scheme

The question now is: is an unnecessary commit more costly than getting two db_cnx, and is any of these two options costly enough to warrant not using the new scheme, and keep handling such cases manually.

My suggestion would be option 2, which is the ugliest atm, but my guess is an unnecessary commit has virtually no cost, and the ugliness of the variable propagation via array could be removed in future with-statements by doing returns directly.

[Christian Boos]

Replying to shookie@…:

The question now is: is an unnecessary commit more costly than getting two db_cnx, and is any of these two options costly enough to warrant not using the new scheme, and keep handling such cases manually.

Getting another db is not actually what will happen. The API documentation on Environment.get_db_cnx() is a bit lacking on this topic, but it correctly states "Return a database connection from the connection pool." which is at least a hint of what actually happens … which is that a given thread is guaranteed to get back the same db connection if it already has a reference to a db connection.

That is, you have:

   db1 = env.get_db_cnx()
   db2 = env.get_db_cnx()
   assertTrue(db2 is db1)

Which means solution 1. would be OK.

Now solution 2. is interesting as well. Remy already wondered what would happen if we did a commit without having anything to commit. My take was that it doesn't matter. PySqlite even tracks the status of a connection, i.e. it knows if a transaction is in progress or not depending on whether there have been some DML commands (INSERT/UPDATE/DELETE) issued, and then only attempts a SQLite level COMMIT if a transaction is really in progress (see pysqlite:source:/src/connection.c@641bee#L448). It remains to be seen what happens for the other database backends.

At the command-line, there seem to be no problems:

mysql> begin;
Query OK, 0 rows affected (0.00 sec)

mysql> commit;
Query OK, 0 rows affected (0.00 sec)

mysql> commit;
Query OK, 0 rows affected (0.00 sec)

and:

trac0115postgres=> begin
trac0115postgres-> ;
BEGIN
trac0115postgres=> commit;
COMMIT
trac0115postgres=> commit;
WARNING:  there is no transaction in progress
COMMIT

Well, a warning, that's all.

Now we probably need to check with the bindings.

What has to be clear is that doing a with_transaction(env, None) will always result in a commit, and I don't think this will be a problem: if the caller of the method had issued a DML on the same connection and doesn't pass it to the method (but, as seen before, we will get that connection when we'll do a get_db_cnx()), we were already in trouble with the old style (handle_ta == True).

My suggestion would be option 2, which is the ugliest atm, but my guess is an unnecessary commit has virtually no cost, and the ugliness of the variable propagation via array could be removed in future with-statements by doing returns directly.

Right, that would be fine. And well, as we anyway see that it's a rvalue, let's pick a more explicit name for that variable, like imported.

But as explained earlier, solution 1. would be OK as well.

[shookie@…]

WikiAdmin Import_pages in the 1st variant, which is cleaner and has no penalty if get_db_cnx acts smart

[shookie@…]

Complete wiki subtree uses with_transaction now. No more db.commits to be found. Cumulative patch on top of the one before.

[Christian Boos]

The two patches above committed:

• in r8713, I removed the use of tmpdb and directly used get_db_cbx().cursor()
• in r8712, I fixed a nasty bug introduced by an indentation error, in … (well, exercise left to the reader, it should have been obvious when reviewing the patch but I managed to miss it and had to debug)

Feel free to continue with other modules ;-)

[shookie@…]

removes all handle_ta and commits from the web subtree

[shookie@…]

I think I need to say a few things on the web-subtree patch. Since it contains the session management, I tried some different approaches to deal with the concurrency checks, also did some more testing. But best approach still is just the more or less unaltered code put into a transaction wrapper, additional rollbacks and all. This patch can be applied independently.

[shookie@…]

Doing the versioncontrol subtree now, and have the problem, that env is not available in the CachedRepositry, so I have to either make the commit/rollback explicit, which we don't want, or I will have to change the getdb name to get_db_cnx, so I can pass self as env to the wrapper (yay, duck typing).

getdb is not marked as a private variable, but it is also not used outside the CachedRepository, according to grep. So, is that change approved, or has anyone an idea if and where that can cause compatibility issues?

[Remy Blank]

Replying to shookie@…:

Doing the versioncontrol subtree now, and have the problem, that env is not available in the CachedRepositry

The versioncontrol package was massively changed in the multirepos branch, and in particular, the env is available in CachedRepository there. So you should probably leave that package out until multirepos is merged to trunk.

[shookie@…]

The versioncontrol package was massively changed in the multirepos branch, and in particular, the env is available in CachedRepository there. So you should probably leave that package out until multirepos is merged to trunk.

Ok. Doing that. Thanks.

[shookie@…]

There was a tab bug in it too, sorry for that, lot of tabbing going on in that refactoring

[shookie@…]

ticket model updated for transactions

[shookie@…]

ticket/admin.py refactored, lots of transactions in there

[shookie@…]

completed ticket subtree

[shookie@…]

the transactions in trac/*.py

[shookie@…]

Those are the places that are not converted yet for now:

trac/versioncontrol/*

but this should be delayed anyway until multirepos merge

contrib/*

not sure if this should be done, sort of outside the main trac, maybe as an example

trac/test.py

good place for that, setting up and destroying the test databases, should probably do that here?

trac/db/tests/api.py

StringsTestCase, don't think this is necessary here, overkill with no benefit

[Remy Blank]

I see many locations in your patches where None is passed as db. We should set that as a default value in with_transaction():

def with_transaction(env, db=None):
    ...

[shookie@…]

Probably. Didn't expect that to happen so often. But I think it's better to make that change on top of the current patches with one patch, once they are committed, and possible problems/bugs ironed out.

[Christian Boos]

Great work Jan!

web_subtree.2.diff​ is fine. ticket_model.diff​ really shows the pay-off of the "with transaction" approach.

In ticket_admin.diff​:

• changed = [ False ] → changed = [False]
• I'm not sure I like the various xxx(db=db), but that was already there, so better not change it at first. It seems it comes from Milestone.delete which breaks the convention of having db as the first parameter (r1606). Wonder if we shouldn't fix this…

In ticket_rest.diff​ and report.py: the diff highlights the parts which should be refactored in a Report model object :-) For later.

In trac_root.diff​:

• trac/env.py, do_upgrade should probably be done for each upgrader (that would be a behavior change, see ​bitten:#462).
• trac/attachment.py, attachment_added is now called after targetfile.close; I think the change is OK

I'll test the changes later on.

I also think we should have with_transaction(env, db=None), I'll change that after applying the patches.

[anonymous]

Replying to cboos:

In ticket_rest.diff​ and report.py: the diff highlights the parts which should be refactored in a Report model object :-) For later.

Found error in it. Changed _do_delete prototype without changing call site.

[shookie@…]

Ok, trying to plan my next steps. There are a few issues listed here, but to start working on them, I need to know, whether I should work on the patches themselves, or if I should just wait until the patches are applied and work from the next revision.

I could also start adding the rename functionality to the wiki model in this branch. Any number of things I could do. Just need some coordination.

[Christian Boos]

Applied attachment:web_subtree.2.diff​ in r8756, with several adaptations:

• there was a return which was simply moved into the transaction function in the patch; that was not correct, used a flag instead (search for need_update)
• fixed line length to be under 80 chars
• additional changes: improve the logging statements (don't output the exception value, use arguments instead of % formatting)

An extra pair of eyes on that changeset would be welcomed ;-)

[Christian Boos]

Applied attachment:ticket_model.diff​ in r8758. Unfortunately, modify_comment was quite broken, so I had to fix it. The rest seemed to be fine.

[shookie@…]

Many thanks for looking into my patches. A few remarks/questions.

1. I saw a few cases where a line was exactly 80, so I thought it to be ok. Apparently not.
2. The return in question, was from the session save I suppose, and is the one that kept deleted sessions from trying to be kept alive. May I ask why exactly a return doesn't work in that place?
3. The modify_comment error is quite embarrassing, that's true. I'm gonna blame it on tiredness ;)

Now, how should I proceed? I gave a summary above of what files aren't converted yet. Anyone any opinion there?

[Remy Blank]

Replying to cboos:

An extra pair of eyes on that changeset would be welcomed ;-)

I'll review the whole branch, but I need a little time.

[Christian Boos]

I still have the following patches to apply and test:

• ticket_admin.diff​
• ticket_rest.diff​
• trac_root.diff​

[Christian Boos]

Replying to shookie@…:

Many thanks for looking into my patches. A few remarks/questions.

1. I saw a few cases where a line was exactly 80, so I thought it to be ok. Apparently not.

It's indeed 79 max, as ​PEP:0008 prescribes. But it's true that sometimes I let lines of exactly 80 chars through… those should be fixed eventually.

2. The return in question, was from the session save I suppose, and is the one that kept deleted sessions from trying to be kept alive. May I ask why exactly a return doesn't work in that place?

Those damn indentation based languages… fixed in r8772. Blame tiredness for that as well ;-)

[Christian Boos]

Applied ticket_admin.diff​ in r8773.

Clean-ups done:

• [ False ] ⇒ [False]
• if x == True: ⇒ if x:
• with_transaction(self.env, None) ⇒ with_transaction(self.env)

[shookie@…]

Now that 11.6 is out, are things going to move on this front again? I was sitting back the last few weeks to see what happens/get feedback.

So what are the priorities right now? The multirepo-merge, the notification system? Just wanna know what to expect. Or if my contributions are found lacking/more trouble than worth it.

[Remy Blank]

Replying to shookie@…:

So what are the priorities right now? The multirepo-merge, the notification system? Just wanna know what to expect. Or if my contributions are found lacking/more trouble than worth it.

No, don't worry, your contributions were good. I wrote above that I would review the branch, but haven't had time yet to do so. Christian is probably waiting for my feedback, so I'm currently the blocker on this issue.

As for the priorities, I would love to see the multirepos branch merged soon, but that's Christian's branch and he will have to decide when it's ready.

[Christian Boos]

There were still 2 patches left to apply (done now in r8873, r8874), I've spotted an error in the first, and there were still some style issues like in comment:21.

Now that there are applied, it would be very helpful to go through the full patch, and test the different parts of the code that have been affected, as those changes are by nature quite prone to introduce errors, as we've seen.

Maybe it would even make sense to rebase the patch on the latest trunk before performing this thorough testing. After that, Remy could do a final review before merge.

And yes, while the priority should be the multirepos merge and documentation, I still would like to get #1106 in 0.12, and I count on you for that ;-)

[Remy Blank]

Replying to cboos:

Maybe it would even make sense to rebase the patch on the latest trunk before performing this thorough testing.

Yes, that would be very useful.

[anonymous]

Replying to cboos:

And yes, while the priority should be the multirepos merge and documentation, I still would like to get #1106 in 0.12, and I count on you for that ;-)

It was the main reason why I did this refactoring. So as soon as the code basis on which I should implement the rename is clear, I will start with it. And since I have done work on it before, I don't expect it to take too long after that.

And I probably still should do trac/test.py as the last piece apart from the versioncontrol. Which I'm gonna start now I guess. Expect another patch in a bit.

[shookie@…]

Ok, looked into test.py, decided it would be a bad idea to introduce the with_transaction theme there. Things are too different in this test environment to work with the with_transaction scheme seamlessly. The absence of a real environment in the helper functions, the passed db that needs the transaction handled for it. Just too different. So I guess that's it.

After a rebase I will start implementing rename then on this branch in parallel to your testing. Or would that be a bad idea?

[Christian Boos]

Just in case this was not entirely clear: don't wait for me to do the rebase, just take the existing changes (diff:sandbox/8751-with_transaction@8670//sandbox/8751-with_transaction), refresh that patch on current trunk (r8957 or so) and attach the new patch here. Thanks!

[shookie@…]

Long time no see ;)

I just wanna say, that I'm maintaining an own branch of the with_transaction changes and keep it in sync with trunk via git svn rebase. Way easier for me than having to constantly create new patches manually and post them here. Also much less work for you.

So basically, I'm waiting for the multirepo changes to make it into trunk, to finish up the transaction work in the versioncontrol/ subtree. The renaming of wiki pages at #1106 is still dependent on that, and that I will do after.

If there is interest I could generate a diff to create a new with_transaction branch from trunk, with the changes that have been accepted here so far. But I'm the only one working and testing on this branch, so it won't do much good, unless those changes get merged into trunk already.

[Christian Boos]

Replying to shookie@…:

Long time no see ;)

Hello Jan, good to hear back from you.

I just wanna say, that I'm maintaining an own branch of the with_transaction changes and keep it in sync with trunk via git svn rebase. Way easier for me than having to constantly create new patches manually and post them here. Also much less work for you.

Perfect!

So basically, I'm waiting for the multirepo changes to make it into trunk, to finish up the transaction work in the versioncontrol/ subtree. The renaming of wiki pages at #1106 is still dependent on that, and that I will do after.

Well, #1106 doesn't depend on the versioncontrol/ subtree, so you can already stack the wiki-rename work on top of your current branch, so that you're ready to go with that as well ;-)

If there is interest I could generate a diff to create a new with_transaction branch from trunk, with the changes that have been accepted here so far. But I'm the only one working and testing on this branch, so it won't do much good, unless those changes get merged into trunk already.

Yes, why not. If you generate the patch, we can then test it on trunk and check it on multirepos to see how much interference there are. If not too many, we'll merge it on trunk. Be aware that Remy did quite some changes recently on the ticket module, so be very careful when merging with those (no indent mishap!!).

[shookie@…]

Be aware that Remy did quite some changes recently on the ticket module, so be very careful when merging with those (no indent mishap!!).

Yes, noticed that. But wasn't too difficult to clean up. But should be reviewed I guess. I'll attach the diff.

[shookie@…]

put with_transaction into current trunk

[Christian Boos]

Replying to shookie@…:

Yes, noticed that. But wasn't too difficult to clean up. But should be reviewed I guess. I'll attach the diff.

Well done! I've checked it applies cleanly on trunk, as well on multirepos, the tests all pass on the 3 database backends on both branches. So it's good to merge for me.

Two details we can address before or after the merge:

• the following idiom:

  cursor = self.env.get_db_cnx().cursor()
  

  which I introduced in comment:4 has been found to be problematic since then (r8878)
• also in ticket/model.py, when we use rdb = self._get_db(), we should also pass that one to the with_transaction(self.env, rdb) call that follows, even though it makes no actual difference (comment:3), it looks clearer that way

There are probably other details (or even bugs!), but at this stage I'm happy with the patch. Remy?

[shookie@…]

• also in ticket/model.py, when we use rdb = self._get_db(), we should also pass that one to the with_transaction(self.env, rdb) call that follows, even though it makes no actual difference (comment:3), it looks clearer that way

I think that can be a problem. When you get a db before the transaction to make some checks, you definitely have a db, so the transaction wrapper won't manage it ever. As you remember, the semantics is, if a db gets passed to the wrapper, it is assumed that it is part of a larger transaction on that db, so it won't manage it, but leaves it to the outer transaction.

[Remy Blank]

I have a few comments:

• In Ticket.save_changes(), why use a separate rdb instead of extending the transaction for the whole method? In addition, you only create one cursor from rdb, but use it within the transaction scope (which has a separate db) to execute statements. So there's a bit of a mix-up there.

• In the session code, there's a possibility of a "double rollback". Doesn't that trigger an error on PostgreSQL?

• In WikiPage, the wiki page name cache doesn't need to be invalidated when only the readonly flag changes (this was already the case before, it's not Jan's fault).

Other than that, the patch reads nicely, good job! I have run the test suite on Linux (SQLite), OS X (SQLite, MySQL, PostgreSQL) and Windows 7 (SQLite), and all tests pass.

I'd like the first item above fixed before the merge, the other two can be verified and fixed if necessary afterward.

[shookie@…]

• In Ticket.save_changes(), why use a separate rdb instead of extending the transaction for the whole method? In addition, you only create one cursor from rdb, but use it within the transaction scope (which has a separate db) to execute statements. So there's a bit of a mix-up there.

That I explaine3d above, not pulling a new cursor in the transaction is a genuine mistake though.

• In the session code, there's a possibility of a "double rollback". Doesn't that trigger an error on PostgreSQL?

I'll look into that, but I think I have done that pretty 1:1 to what was there before

[anonymous]

Ok, looked into the double rollback, and I think it cannot occur. Because before that can occur after the first rollback, another db update needs to be done for another exception to be thrown. And then it's a different rollback.

[Christian Boos]

applies on transaction.diff and fixes the main issues mentioned in comment:32 and comment:34

[Christian Boos]

Replying to rblank:

• In the session code, there's a possibility of a "double rollback". Doesn't that trigger an error on PostgreSQL?

Speaking of that, and in light of #8443, I think we also have to take care of rolling back after errors happening in SELECT queries. This means we have to do it at the cursor wrapper level (IterableCursor). Once this is done, does that mean we'll have to remove the rollback from the with_transaction decorator? Do you have some more background on that error? On the command line, it seems we just get a "NOTICE: no transaction in progress.

[Remy Blank]

Replying to cboos:

Speaking of that, and in light of #8443, I think we also have to take care of rolling back after errors happening in SELECT queries.

Really? Have you already tested that this will help, or is this an assumption? I find it a bit surprising that a SELECT, which doesn't start a transaction, should be rolled back… But then again, I'm by far no DB expert.

Once this is done, does that mean we'll have to remove the rollback from the with_transaction decorator? Do you have some more background on that error? On the command line, it seems we just get a "NOTICE: no transaction in progress.

I only vaguely remember reading something about that, but I couldn't find the source anymore, so maybe I'm mixing things. My comment was more a sanity check in case you remembered that issue. If you don't, you can safely assume that my memory is bad :-)

[Christian Boos]

Replying to rblank:

Replying to cboos:

Speaking of that, and in light of #8443, I think we also have to take care of rolling back after errors happening in SELECT queries.

Really? Have you already tested that this will help, or is this an assumption? I find it a bit surprising that a SELECT, which doesn't start a transaction, should be rolled back…

No, haven't tested yet, but I think that an aborted or erroneous SELECT can leave a connection in the idle in transaction state. How such a connection remains in such state after a request has completed (i.e. when a try_rollback should have happened) is still unclear.

Besides, I also think that with PostgreSQL at least, an erroneous SELECT makes the connection unusable there's a rollback (ProgrammingError: current transaction is aborted, 'commands ignored until end of transaction block') (and yes, I should also verify this theory with some sample programs ;-) ).

[shookie@…]

Replying to cboos: Is it still ok, to finish up the with_transaction rewrite in the versioncontrol subtree, now that multirepo is merged? I intend to finish that up over the weekend, e.g. apply the last two patches here to trunk, and work on top of that.

[Christian Boos]

Replying to shookie@…:

Replying to cboos: Is it still ok, to finish up the with_transaction rewrite in the versioncontrol subtree, now that multirepo is merged? I intend to finish that up over the weekend, e.g. apply the last two patches here to trunk, and work on top of that.

Sure, go ahead. If Remy is OK with that, I could even apply the current patch + my fixes in the meantime.

[Remy Blank]

Replying to cboos:

If Remy is OK with that, I could even apply the current patch + my fixes in the meantime.

Yes, please do. It's probably easier to apply this first, then I can rebase #6466 on top of that.

[shookie@…]

Ok, I was starting on applying the patches, since they are not merged yet, and noticed, that they cause problems now.

For one, there really must be a separate rdb (readonly db) for preliminary checks before the real transaction with the writes starts. As I have said above, if you use the same db in the transaction, it assumes to be part of a bigger transaction and does not to do the transaction management.

There are several ways to deal with that, but just passing along the db that we got in the function is not one of them, it is semantically false.

Lets take ticket/model.py:save_changes as an example:

#not the db parameter, meaning this fucntion can be part of a larger transaction
def save_changes(self, author, comment, when=None, db=None, cnum=''):
    # checks without db
    ...
    # getting readonly db
    rdb = self._get_db()
    cursor = rdb.cursor()

    # checks within db that can lead to abort before the db has been altered
    ...   
    # now start transaction
    # we can't pass rdb on here, because db function parameter can be None
    # in which case with_transaction has to get an own db
    # and do the commits and rollbacks
    # for the same reason we can't just call our rdb db, they must be different

        @with_transaction(self.env, db)
        def do_save(db):
            cursor = db.cursor() # always the first line, if SQL inlined here
            # do db updates
            # any uncaught exception leads to a rollback and abort

[shookie@…]

I was converting the versioncontrol subtree to @with_transaction now too. Won't convert ​sync though, too complicated for the wrapper, I think.

The following two problems turned up:

• when ​modify_repository is converted, it triggers the #8247 regression test all of a sudden, and I have no idea how that can possibly happen
• when ​get_repository_id is converted, the setting up of the functional testenv fails with a type error in the db backend, without get_repository_id even showing up in the stack trace.

What is really strange in both cases is, that the methods in question are really simple ones actually. Textbook examples for the with_transaction scheme. So I must be missing something important somewhere.

When I don't convert those two methods, the whole testsuite finishes without any problems.

[shookie@…]

Finished with the with_transaction conversion. Those two errors mentioned above were just a result of me being tired. Fixed them pretty quickly after sleeping. sync I still won't convert though. Spent the rest of the day doing a lot of tests, and trying to get rid of the last rdb reference in ​save_changes. But for some reason the cnum parameter is not defined anymore inside the do_save transaction method. Must be some weird Python variable scope problem again, and I would really appreciate, if someone could tell me what is going on there.

Other than that, I'm really satisfied with how it went.

[shookie@…]

Complete conversion on top of trunk

[Remy Blank]

Replying to cboos:

If Remy is OK with that, I could even apply the current patch + my fixes in the meantime.

Do you still intend to apply this shortly? I'd be ready to apply #6466 afterward. Or should I go ahead and commit #6466 first (it will probably generate many conflicts)?

[Christian Boos]

Replying to rblank:

Do you still intend to apply this shortly? I'd be ready to apply #6466 afterward. Or should I go ahead and commit #6466 first (it will probably generate many conflicts)?

I'm tweaking and testing it right now ;-)

[Christian Boos]

Replying to shookie@…:

Spent the rest of the day doing a lot of tests, and trying to get rid of the last rdb reference in ​save_changes. But for some reason the cnum parameter is not defined anymore inside the do_save transaction method. Must be some weird Python variable scope problem again, and I would really appreciate, if someone could tell me what is going on there.

Simply because of the way ​nested scopes work in Python?

… Note that nested scopes work only for reference and not for assignment which will always write to the innermost scope.

(here we would read cnum in the nested scope before writing to it - writing into it makes it belong to the inner scope)

Here's save-changes.diff​. Remy, I didn't have time to review the versioncontrol changes yet, so maybe you can do it or I'll do it in the coming days.

[shookie@…]

Replying to cboos:

Simply because of the way ​nested scopes work in Python?

… Note that nested scopes work only for reference and not for assignment which will always write to the innermost scope.

(here we would read cnum in the nested scope before writing to it - writing into it makes it belong to the inner scope)

Right, I need to keep remembering that scoping works differently for reading and writing. Was the same thing for the wrapper itself …

Ok, and next weekend I'm gonna do #1106.

[Christian Boos]

eliminate rdb from Ticket.save_changes, applies on top of full_with_transaction.diff​

[Christian Boos]

Replying to cboos:

… I didn't have time to review the versioncontrol changes yet, so maybe you can do it or I'll do it in the coming days.

Did a review. The changes in trac/versioncontrol/api.py were OK, but the RepositoryCache.sync_changeset changes were buggy and obviously untested as even the import was missing (see fix-sync_changeset.diff​).

I also did a conversion of RepositoryCache.sync, but the result (while working) looks much too contrived. Hopefully with the real with transaction, it will go smoother but for now I agree to keep explicit rollback and commit there.

[Christian Boos]

fix usage of with_transaction in RepositoryCache.sync_changeset, applies on top of full_with_transaction.diff​

[shookie@…]

Replying to cboos:

Did a review. The changes in trac/versioncontrol/api.py were OK, but the RepositoryCache.sync_changeset changes were buggy and obviously untested as even the import was missing (see fix-sync_changeset.diff​).

Now that I will have to look into once I'm home again tomorrow. Only way I can imagine that to happen is by making a diff from the wrong commit (in my local git) or something like that. Which just shows that many smaller diffs to send are better … damn laziness.

Many thanks for the review and the comments.

[shookie@…]

the consolidated/combined transaction patch, I use as base for #1106, just for convinience

[Remy Blank]

Cleanup + minor fixes, applies on top of full_with_transaction.diff​, save-changes.diff​ and fix-sync_changeset.diff​.

[Remy Blank]

I suggest the additional patch above:

• Mainly cleanup
• Fixed get_repository_id()
• Made the behavior of some functions more strictly identical to the current behavior

[shookie@…]

Replying to rblank:

I suggest the additional patch above:

• Mainly cleanup
• Fixed get_repository_id()
• Made the behavior of some functions more strictly identical to the current behavior

Many thanks, was no problem merging that to my current rename work.

[Christian Boos]

I committed full_with_transaction.diff​ + fix-sync_changeset.diff​ as r9183, and save-changes.diff​ as r9184.

Replying to rblank:

I suggest the additional patch above:

Sure, nice catches. Can you commit it, please?

[Remy Blank]

Replying to cboos:

Sure, nice catches. Can you commit it, please?

Ok, will do. It might not be tonight, though, I'm as sick as a dog :(

[Remy Blank]

One comment about [9185]: the additional whitespace added by making the SQL statements multi-line strings has no effect, so it's ok. That's not necessarily the case when making the error message in a TracError construction a multi-line string. It will work fine when the message is displayed as HTML, but not on the console. So maybe we should keep the current practice in that case.

[Christian Boos]

Replying to rblank:

… TracError construction a multi-line string vs. console.

Ah yes, I hadn't thought about that. Fixed in r9186.

Hope you'll be "fixed" in time for PyCon ;-)

[Christian Boos]

Replying to rblank:

Replying to cboos:

Sure, nice catches. Can you commit it, please?

Ok, will do. It might not be tonight

+ you could include the one-liner fix for #9059.

[shookie@…]

I gotta ask, what is your testing routine? Obviously I'm missing quite a few things, when I just run the test suite. And not only because of the occasional lapse. And obviously I'm not as familiar with the code base as you are, which also causes some problems.

[Remy Blank]

Replying to shookie@…:

I gotta ask, what is your testing routine?

Not sure if you were talking about Christian or me, but I have only been reading the patch and the patched sources. Which is bad (in a way), because it means that our test suite didn't catch the errors. OTOH, most of the changes in my last patch were only "cosmetic" and not errors, so it's not as bad as it sounds.

We definitely have some locations that are not tested (typically sync_changeset()), and ideally we should try to add the tests at some point.

[Remy Blank]

Applied 8751-cleanup-fixes-r9181.patch​ in [9194], together with the fix for #9059.

So, anything left to be done here, or can we close?

[Christian Boos]

Well, there's the open point about transaction nesting discussed in #9060, which is a possible further refinement, but the bulk of the change has been done.

Many thanks to Jan for his first major contribution!

[shookie@…]

Replying to cboos:

Many thanks to Jan for his first major contribution!

Do I get a cookie? ;)

[Christian Boos]

Not yet :-)

[Remy Blank]

Replying to shookie@…:

Do I get a cookie? ;)

You already got one. Check your browser's privacy settings ;-)

[Christian Boos]

Conclusion on this topic was done for 0.13, with the introduction of the with env.db_transaction/db_query syntax. See TracDev/DatabaseApi#Trac0.13API and #9536.