> -----Original Message-----
> From: Hyrum K. Wright [mailto:hyrum_at_hyrumwright.org]
> Sent: zaterdag 31 oktober 2009 7:04
> To: dev_at_subversion.tigris.org
> Subject: Re: svn commit: r40326 - trunk/subversion/libsvn_subr
>
> 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.
Greg said that he didn't intend to fix this class of issues in wc_db.c.
Almost every function in wc_db.c has an SVN_ERR() that doesn't reset its
statement..
If you are iterating over nodes one level above such a function that doesn't
reset it's statement on a path not found (like the one I fixed in r40318),
you can't handle the error in any other way than by exiting svn.. and
completely restart the command. (Would be nice if we have one caused from
the working queue).
This patch is another safety net, for specific code paths that 'forgets' the
reset.. We should never stop resetting explicitly. (Maybe we should just
assert when we catch such an error in SVN_DEBUG mode?)
But not catching these errors will certainly break clients encountering
working copy inconsistencies... And our testsuite will never test all our
error paths.
If a TortoiseSVN user would encounter such an error (Which is not unlikely)
the user would have to logout and login into Windows to recover from this
error.
Not everybody uses 'svn', which happens to exit after every command/error.
Bert
> (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&dsMessageI
> d=2413250
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageI
> d=2413276
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413281
Received on 2009-10-31 19:18:15 CET