A change like this gives me the willies.
I've spent my share of time tracking down this class of bug, so I
understand the issues as play, but this just feels like papering over
the problems rather than solving them.
(If we do chose to use this method of "reset before use" we should do
it everywhere. I'm unsure of the performance implications of having
many un-reset statements in the database.)
-Hyrum
On Oct 31, 2009, at 9:41 AM, Bert Huijben wrote:
> Author: rhuijben
> Date: Sat Oct 31 07:41:48 2009
> New Revision: 40326
>
> Log:
> Automatically reset sqlite statements on retrieving when we 'forgot'
> to
> reset them on the previous usage.
>
> While we should always reset sqlite statements directly after using
> them,
> many error conditions don't do this. But when a sqlite statement is
> used
> again without resetting it, you get an unrecoverable "operation out of
> order" error from sqlite.
>
> This change resolves a class of errors most common in error handling,
> but it can also hides errors on reusing statements within an inner
> function.
>
> Our test suite should always run ok with and without this patch.
> (Maybe
> we should only enable this on release mode?)
>
> Not applying this fix, can bring a svn_wc_context_t in a state where
> it is
> no longer usable without destroying and recreating it, because
> specific
> sqlite statements get stuck. This could break existing clients that
> reuse
> a single svn_client_ctx_t for all their operations (Like javahl,
> sharpsvn,
> TortoiseSVN and several other bindings do in their current
> implementation).
> (Before introducing svn_wc_context_t all svn_client_*() functions
> automatically brought the context back in a stable state after
> exiting).
>
> * subversion/libsvn_subr/sqlite.c
> (svn_sqlite__stmt_t): Adds needs_reset boolean.
> (svn_sqlite__get_statement): If needs_reset is set, reset the
> statement
> before returning it.
> (svn_sqlite__prepare): Initialize needs_reset to false.
> (svn_sqlite__step): Set needs_reset to true, when not resetting
> directly.
> (svn_sqlite__reset): Set needs_reset to false.
>
> Modified:
> trunk/subversion/libsvn_subr/sqlite.c
>
> Modified: trunk/subversion/libsvn_subr/sqlite.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/sqlite.c?pathrev=40326&r1=40325&r2=40326
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- trunk/subversion/libsvn_subr/sqlite.c Sat Oct 31 06:19:40 2009
> (r40325)
> +++ trunk/subversion/libsvn_subr/sqlite.c Sat Oct 31 07:41:48 2009
> (r40326)
> @@ -69,6 +69,7 @@ struct svn_sqlite__stmt_t
> {
> sqlite3_stmt *s3stmt;
> svn_sqlite__db_t *db;
> + svn_boolean_t needs_reset;
> };
>
>
> @@ -125,6 +126,10 @@ svn_sqlite__get_statement(svn_sqlite__st
> db->result_pool));
>
> *stmt = db->prepared_stmts[stmt_idx];
> +
> + if ((*stmt)->needs_reset);
> + return svn_error_return(svn_sqlite__reset(*stmt));
> +
> return SVN_NO_ERROR;
> }
>
> @@ -134,6 +139,7 @@ svn_sqlite__prepare(svn_sqlite__stmt_t *
> {
> *stmt = apr_palloc(result_pool, sizeof(**stmt));
> (*stmt)->db = db;
> + (*stmt)->needs_reset = FALSE;
>
> SQLITE_ERR(sqlite3_prepare_v2(db->db3, text, -1, &(*stmt)->s3stmt,
> NULL), db);
>
> @@ -188,6 +194,7 @@ svn_sqlite__step(svn_boolean_t *got_row,
> }
>
> *got_row = (sqlite_result == SQLITE_ROW);
> + stmt->needs_reset = TRUE;
>
> return SVN_NO_ERROR;
> }
> @@ -484,6 +491,7 @@ svn_sqlite__reset(svn_sqlite__stmt_t *st
> {
> SQLITE_ERR(sqlite3_reset(stmt->s3stmt), stmt->db);
> SQLITE_ERR(sqlite3_clear_bindings(stmt->s3stmt), stmt->db);
> + stmt->needs_reset = FALSE;
> return SVN_NO_ERROR;
> }
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=2413250
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413276
Received on 2009-10-31 19:04:11 CET