Environment.db_query example should not call fetchall

[Ryan J Ollos <ryan.j.ollos@…>]

In Trac 1.0, a call to Environment.db_query can be traced to a call to trac.db.util.ConnectionWrapper.execute with readonly=True, which calls fetchall to return the iterable. Therefore it seems like the example in the documentation for using Environment.db_query needs to be updated to remove the call to fetchall.

This is a pretty minor issue, so I'm not sure it is worth a ticket, but I've been trying to understand when it is necessary to call fetchall, and couldn't think of another way to relay the info.

I grep'ed the source for instances in which fetchall is called in the application code (outside the db layer), and only found 3. The call in db21.py seems unnecessary.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11401 of the trunk.

[Christian Boos]

The documentation goes on like this:

 Example::
454	
455	            with env.db_query as db:
456	                cursor = db.cursor()
457	                cursor.execute("SELECT ...")
458	                for row in cursor.fetchall():
459	                    ...
460	
461	        Note that a connection retrieved this way can be "called"
462	        directly in order to execute a query::
463	
464	            with env.db_query as db:
465	                for row in db("SELECT ..."):

Note how the first example is similar to what ConnectionWrapper.execute does (your calls fetchall link):

120	        cursor = self.cnx.cursor()
121	        cursor.execute(query, params)
122	        rows = cursor.fetchall() if dql else None

So I see nothing wrong with that code. Now it's true that using the cursor is more a "legacy" way, given that we have a more direct API now.

The 2nd documentation in the example states that calling db(sql) is a shortcut to the first example. Maybe we could add that this shortcut is the same as doing db.execute(sql)?

Normally that 2nd form is just what is needed, but in case you want finer control, you can still get a cursor out of the connection, the DP API 2.0 way. But if you do so, then better take the habit of doing a .fetchall() on it, rather than iterating on the cursor or looping over .fetchone(). This is done order to "exhaust" the cursor the quickest way possible so that we can close it explicitly as soon as possible, which is something we have found to help a lot for reducing the #3446 problem. Well, in truth, for SQLite this is what we do under the hood anyway (except for the close), but it can't hurt to do it explicitly, as it probably also improves the concurrency behavior for other backends.

What I can do is try to reorganize the doc in order to explain the "preferred" way first, then explain what it does in terms of cursor and state that this more explicit way can still be used if necessary.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

Note how the first example is similar to what ConnectionWrapper.execute does (your calls fetchall link)

I was reading the code in the first documentation example as if it was:

with env.db_query as db:
    rows = db.execute("SELECT ...")
    for row in rows:
        ...

However, it is somewhat more obvious to me now that the cursor doesn't magically become rows!

So I see nothing wrong with that code. Now it's true that using the cursor is more a "legacy" way, given that we have a more direct API now.

The 2nd documentation in the example states that calling db(sql) is a shortcut to the first example. Maybe we could add that this shortcut is the same as doing db.execute(sql)?

As a novice, I would be less confused if the legacy way of using the cursor was shown last, and the first example showed db.execute(sql), followed by the example that uses for row in db("SELECT ..."):.

Normally that 2nd form is just what is needed, but in case you want finer control, you can still get a cursor out of the connection, the DP API 2.0 way. But if you do so, then better take the habit of doing a .fetchall() on it, rather than iterating on the cursor or looping over .fetchone(). This is done order to "exhaust" the cursor the quickest way possible so that we can close it explicitly as soon as possible, which is something we have found to help a lot for reducing the #3446 problem. Well, in truth, for SQLite this is what we do under the hood anyway (except for the close), but it can't hurt to do it explicitly, as it probably also improves the concurrency behavior for other backends.

This is the part I didn't understand, but maybe it is explained in some other part of the docs. I bet I can find a number of places I'm doing iterating over the cursor in the plugins I'm maintaining, and I only ever test with SQLite, so it sounds like I wouldn't have noticed the problems that could occur with other DB backends.

If needing the explicit fine-control of the cursor is very rare, and I have to think that is the case since I don't see many instances of it in the source code, I tend to think your next suggestion is the way to go …

What I can do is try to reorganize the doc in order to explain the "preferred" way first, then explain what it does in terms of cursor and state that this more explicit way can still be used if necessary.

Yes!

Well, sorry for the noise. I think you have some good ideas for making the docs more understandable to beginners like me though.

[Christian Boos]

All the tickets for {20} from last year have probably been seen multiple times by now, yet are still to be triaged…

[Ryan J Ollos]

I spent some time looking at this, and couldn't see a way to improve the documentation. The documentation seems good, and I don't think we should second guess it to try to fix my earlier misunderstanding, at least until we have more feedback from other users.