statement caching doesn't work in all cases #6

Closed
TomSwift opened this Issue Mar 9, 2011 · 12 comments

Comments

Projects
None yet
8 participants

TomSwift commented Mar 9, 2011

Statement caching (FMDatabase shouldCacheStatements) currently keys off of the sql of the query that the statement is based on. However it doesn't take into account parameter values. This means that similar queries having differing parameters will overwrite each others result-sets.

Consider the following, with statement caching ON:

BOOL b;
FMResultSet* rs1 = [myDB executeQuery: @"SELECT rowid FROM myTable WHERE myColumn = ?", [NSNumber numberWithLong: 1]]; // results in > 10 rows...
b = [rs1 next]; // success

FMResultSet* rs2 = [myDB executeQuery: @"SELECT rowid FROM myTable WHERE myColumn = ?", [NSNumber numberWithLong: 2]]; // results in 1 row
b = [rs2 next]; // success

b = [rs1 next]; // failure (returns FALSE), even though there should be more rows...

The reason for the failure is that the sqlite statement used by rs1 was altered when same query (but with different parameters) was subsequently executed. In the end, rs1 and rs2 are using the same statement and the results for the original query are lost.

With statement caching off, the testcase above works out fine.

zaplitny commented May 9, 2011

I had the same issue, so had to turn statement caching off

Owner

ccgus commented Jul 14, 2011

Besides turning off statement caching, are there any proposed fixes? I'm not sure that there is real solution to this. Nested result sets list this just aren't going to work without removing the benefits of cached statements. At least, that's the conclusion I've come to with a tiny bit of thought involved. I'm fine with being proven wrong :)

I'm probably missing something important but had a couple ideas:

A simple fix is to allow passing a custom key to use for the cached statement. If each level of nesting uses a unique key things should work as expected.

It may also be possible to handle this automatically:

  • FMStatement has a weak reference to the object using it.
  • The owning object (FMResultSet) zeros the reference on dealloc, happens automatically in some environments.
  • cachedStatements has an NSArray for each query. When fetching a statement it finds the first that has a nil owner. If there isn't one a new statement is created for the same query and added to the pool. (Since this is an edge case a class test could be used to only switch to an array when needed).

The danger here is if a lot of FMResultSets are created in a loop and not released until the loop ends you'll get a bunch of cached statements you don't need. But if you run this loop multiple times it will be fast :-)

Owner

ccgus commented Jul 15, 2011

Yea, it could probably have some sort of flag that says "I'm in use right now".

Since there's a reasonable workaround for now, it won't be a high priority for me. But if you do end up writing a patch, let me know.

My feeling is that FMDB should have a mechanism for exposing the FMStatement to the app, which the app can reuse as desired. Then there is no need to have "statement caching" at all.

Something like this:

  • (FMStatement_) statementForSql: (NSString_) sql;
  • (FMResultSet_) executeStatement: (FMStatement_) statement, ...;
Contributor

ChristianKienle commented Apr 5, 2012

Suggestion: Mention the issues with the caching in the tutorial. We just spent 6 hours debugging and then found out it was FMDBs caching that caused problems...

ccarse commented Jan 14, 2013

Is this still an issue?

Owner

ccgus commented Jan 14, 2013

Yes, it's still an issue.

noamtm commented Sep 24, 2013

This issue was reported 2.5 years ago.
We started using FMDB in a highly multithreaded, database driven, iOS application 1.5 years ago.

And we just spent about 2 days debugging our code, until someone thought it might be related to statement caching, disabled it, and found this issue here.

Is there a chance of it being fixed like @TomSwift suggests, or at least mentioning the issue in the tutorial as suggested by @ChristianKienle?
If we contribute the code, will it be added to master?

Owner

ccgus commented Sep 25, 2013

If the code is reasonable, has tests, and doesn't break existing installs, then yes I would be happy to add it to the master branch.

scottcc commented Feb 20, 2015

Curious - did issue #191 fix this?

Owner

ccgus commented Feb 20, 2015

Yep, looks like it. I'll close this bug.

@ccgus ccgus closed this Feb 20, 2015

khawkins pushed a commit to khawkins/fmdb that referenced this issue Jan 5, 2016

Merge pull request #6 from forcedotcom/encrypt
Use dynamic framework and add compile flag for encryption
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment