SQLite3 source in Vienna -- why?

An RSS/Atom newsreader with features comparable to commercial newsreaders.
dcrosta
Muffin
Posts: 28
Joined: Thu Aug 13, 2009 2:00 pm

SQLite3 source in Vienna -- why?

Postby dcrosta » Sun Feb 14, 2010 11:56 pm

Is the SQLite3 source included in Vienna for legacy reasons, perhaps when Vienna was started, SQLite was not distributed with OS X? Is there any reason we shouldn't switch to the system-supplied version? Benefits would be a smaller executable (and thus smaller distribution) by 2.8MB (compiled for PPC and x86); risks, I suppose, would be that changes to the system-supplied version might cause problems if they entail changes to the underlying database format.

In a related note: why don't we set SQLITE_THREADSAFETY to 0 and then enforce our own thread safety with methods in Database.m? Why not let the underlying library manage this for us?

User avatar
Michael Ströck
Vienna Team
Posts: 303
Joined: Mon Mar 06, 2006 9:21 am
Location: Vienna, Austria
Contact:

Re: SQLite3 source in Vienna -- why?

Postby Michael Ströck » Mon Feb 15, 2010 9:29 pm

Good questions - no idea. I do know that Jeff tried using the system-supplied version of SQLite while we were working on 2.4, and things broke, so he switched back to this. I then later switched to not compiling it as our own framework, and just compiling it into the executable with everything else because that's faster and smaller.

In a related note: why don't we set SQLITE_THREADSAFETY to 0 and then enforce our own thread safety with methods in Database.m? Why not let the underlying library manage this for us?


You're confusing me here - which exactly are you proposing to do :-)

In any case, you're very welcome to try things out, see what breaks, and try to make it better!

dcrosta
Muffin
Posts: 28
Joined: Thu Aug 13, 2009 2:00 pm

Re: SQLite3 source in Vienna -- why?

Postby dcrosta » Tue Feb 16, 2010 2:31 pm

Michael Ströck wrote:Good questions - no idea. I do know that Jeff tried using the system-supplied version of SQLite while we were working on 2.4, and things broke, so he switched back to this. I then later switched to not compiling it as our own framework, and just compiling it into the executable with everything else because that's faster and smaller.


Hmm, OK, perhaps I'll experiment with that. Jeff if you swing by and see this, any additional information you have about what didn't work or what caused trouble wold be appreciated

Michael Ströck wrote:
dcrosta wrote:In a related note: why don't we set SQLITE_THREADSAFETY to 0 and then enforce our own thread safety with methods in Database.m? Why not let the underlying library manage this for us?


You're confusing me here - which exactly are you proposing to do :-)


Sorry! I'm proposing that we remove -(void)verifyThreadSafety in class Database, and set SQLITE_THREADSAFETY to 1. This will (as I understand it) allow the database to be used by multiple threads, without having to rely on work-arounds like -(void)performSelectorOnMainThread.

Michael Ströck wrote:In any case, you're very welcome to try things out, see what breaks, and try to make it better!


Ahh, the open source software default answer :)

User avatar
Michael Ströck
Vienna Team
Posts: 303
Joined: Mon Mar 06, 2006 9:21 am
Location: Vienna, Austria
Contact:

Re: SQLite3 source in Vienna -- why?

Postby Michael Ströck » Tue Feb 16, 2010 4:37 pm

Ah, I see - very weird. I was under the impression that we compiled SQLite as thread safe. I'll ask Steve about this, but I think your suggestion makes sense.

User avatar
Michael Ströck
Vienna Team
Posts: 303
Joined: Mon Mar 06, 2006 9:21 am
Location: Vienna, Austria
Contact:

Re: SQLite3 source in Vienna -- why?

Postby Michael Ströck » Wed Feb 17, 2010 3:02 pm

Steve told me that he intended verifyThreadSaftey only for purposes of checking design while in debug mode. It was essentially a bug that it was being called all the time.

I've now essentially followed all of your advice, see my last commit: We compile SQLite as thread safe, and verifyThreadSafety no longer gets called in Deployment (Release) builds.

stevepa
Vienna Team
Posts: 469
Joined: Fri Jan 13, 2006 3:13 pm

Re: SQLite3 source in Vienna -- why?

Postby stevepa » Wed Feb 17, 2010 10:41 pm

Just to clarify, it wasn't a bug that this was being called constantly. In release mode it was effectively a no-op but since it was fairly cheap, the overhead of having it called was reasonable. Rather it was intended to catch incidents of database access being made on a different thread from the connection while debugging. Prior to SQLite 3.3.1 doing so was a bug and liable to cause database issues. With that and later builds this was no longer an issue but the database code was left as it was to reduce the possibility of intermixed writes from different threads causing database consistency issues. There are probably limited cases where this occurs now - [createArticle] is probably the most extreme example at the moment - but without safeguards there's always the risk that somebody, later, could add some database access logic that may just work fine except during some race conditions across different threads. That's a headache I always preferred to avoid.

With recent versions of SQLite, the atomicity of the updates occurs as the single SQL statement level so we can guarantee that within a single statement, the integrity is assured. Within a transaction, I'm less confident. Despite the fact that 3.3.1 supports thread, the official recommendation from the SQLite author is not to use them and consequently given the current design of Vienna, I see no strong reason to change this design. If we do have to change this, I hope we've considered all the issues of simultaneous access from multiple threads.

An additional data point. When Mac OSX bundled SQLite to support Spotlight, the version was newer that the one Vienna used so to avoid a database upgrade, I opted to continue to compile in our own version directly. It's just easier to manage potential SQLite changes if you've got the version under your own control. The other point was that the SQLite version that shipped with Mac OSX incorporated a fix that supported locking the database remotely thus theoretically allowing simultaneous access. Unfortunately without additional code in Vienna to actually synchronise the client when the remote database was changed it wasn't entirely useful anyway.

What problem are we trying to solve here? The database is a pretty key piece of the code and I'd rather not change it unless we're solving a crucial problem.

User avatar
Michael Ströck
Vienna Team
Posts: 303
Joined: Mon Mar 06, 2006 9:21 am
Location: Vienna, Austria
Contact:

Re: SQLite3 source in Vienna -- why?

Postby Michael Ströck » Wed Feb 17, 2010 11:24 pm

Hi Steve,

What problem are we trying to solve here? The database is a pretty key piece of the code and I'd rather not change it unless we're solving a crucial problem.


The problem is that NSAssert in a release build is arguably a bug. It's just a crash - there's nothing graceful about it. It's not recommended in release builds for that reason. In my opinion, we're safer with a thread-safe SQLite that doesn't crash. Anything we do to it is at least guaranteed to be valid SQL.

... there's always the risk that somebody, later, could add some database access logic that may just work fine except during some race conditions across different threads. That's a headache I always preferred to avoid ... the official recommendation from the SQLite author is not to use them and consequently given the current design of Vienna, I see no strong reason to change this design.


Actually, he flat out says that SQLite is completely thread safe, as long as you compile it as such. He does not recommend it more for philosophical reasons :-)

But in any case, I'm not proposing using different threads now. I just think that this is safer turned on, and doing something else is better than having it crash if we use a wrong thread.

We should do something else instead in the release builds, like immediately quitting gracefully. That's why I left the non-debug version of the new macro blank. We'll just have to decide on a safe way to get out in the release builds.

And: It will still immediately crash at accessing the database from anything but the main thread while in development mode, while logging why it does so.

User avatar
Michael Ströck
Vienna Team
Posts: 303
Joined: Mon Mar 06, 2006 9:21 am
Location: Vienna, Austria
Contact:

Re: SQLite3 source in Vienna -- why?

Postby Michael Ströck » Thu Feb 18, 2010 12:04 am

Sorry, I'm a bit clumsy in getting my point across today. What I'm proposing is that in all places where we just use NSAssert now, we should actually be following something like this pattern:

Code: Select all

-(void)verifyThreadSafety
{
#ifdef DEBUG
   NSAssert([NSThread currentThread] == mainThread, @"Calling database on wrong thread!"); // Make it crash in debug builds so that it stands out
#else
   if (!([NSThread currentThread] == mainThread))
   {
      NSLog(@"Calling database on wrong thread!");
      runOKAlertPanel(NSLocalizedString(@"Database Access Error", nil), NSLocalizedString(@"Vienna has encountered a critical database error and has to quit.", nil));
      [NSApp terminate:nil]; // Quit gracefully in release builds.
   }
#endif
}

stevepa
Vienna Team
Posts: 469
Joined: Fri Jan 13, 2006 3:13 pm

Re: SQLite3 source in Vienna -- why?

Postby stevepa » Thu Feb 18, 2010 5:26 am

I understand what you're proposing but my main arguments are:

1. Why make this change now? Is there an existing customer report that this fixes? This change is being made in 2.5 that is currently in beta and which we want to ship very soon. Changing the SQLite library to be thread-safe does actually introduce new functionality, albeit into SQLite and if we're not solving a real problem then anything at this level would be better moved to the 2.6 codebase. The database code is very low level and while this may ultimately be safe, I'm personally tending to take a more sceptical attitude toward my own code at this stage. :-)

2. The NSAssert might not be ideal but it should never be hit and I'd rather it be hit in retail anyway. Your proposed change also guarantees this but the current implementation has had all its code paths tested so that it shouldn't be hit and if it does then it's a bug that doesn't have any real recovery path. The error message you're proposing is only marginally nicer than the assert and it still shuts down the app. SQLite does commit after write so any changes up to this point will be committed - I've already validated this unless something changed recently.

That's separate from my other concern that making the database code open to being called on multiple threads when it was never designed to be done so is a risk and needs careful testing. I'll follow up with you on this offline though.

jeff_johnson_dev
Cocoaforge Admin
Posts: 1385
Joined: Wed Mar 01, 2006 9:12 pm

Re: SQLite3 source in Vienna -- why?

Postby jeff_johnson_dev » Thu Feb 18, 2010 6:59 am

From sqlite3.h: "Enabling mutexes incurs a measurable performance penalty."

User avatar
Michael Ströck
Vienna Team
Posts: 303
Joined: Mon Mar 06, 2006 9:21 am
Location: Vienna, Austria
Contact:

Re: SQLite3 source in Vienna -- why?

Postby Michael Ströck » Thu Feb 18, 2010 7:41 am

OK, I agree, I'll take thread safety out again.

The other thing I was made aware of by this is still valid though: The40+ NSAsserts that we are currently using are still 40+ easily fixable bugs in my opinion - I'd rather replace them with something that is nicer for the user and us. I'll propose a search/replace-able solution that is nicer for the user and for us.

User avatar
AlanR
Frappa
Posts: 235
Joined: Wed Sep 06, 2006 4:19 am
Contact:

Re: SQLite3 source in Vienna -- why?

Postby AlanR » Thu Feb 18, 2010 11:03 am

I'll stick my 2¢ in here FWIW.

You do have me and at least a few others who are using/testing whatever you do day by day. If you introduce a problem we may well see it, so you do have some early warning.

So far so good (R1522).

User avatar
Michael Ströck
Vienna Team
Posts: 303
Joined: Mon Mar 06, 2006 9:21 am
Location: Vienna, Austria
Contact:

Re: SQLite3 source in Vienna -- why?

Postby Michael Ströck » Thu Feb 18, 2010 4:33 pm

Hi Alan,

We really appreciate your support on the forum here, and also that you provide intermediate builds for people - that's awesome! But Steve is right, it's better to be prudent here :-)

I assume the 2.6 branch will open very soon, and if we get do the stuff that we have planned, there will be more than ample opportunity to test new stuff.


Return to “Vienna”

Who is online

Users browsing this forum: No registered users