You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2012/12/14 20:11:30 UTC
svn commit: r1422042 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
Author: philip
Date: Fri Dec 14 19:11:29 2012
New Revision: 1422042
URL: http://svn.apache.org/viewvc?rev=1422042&view=rev
Log:
Fix issue 4210, rep-cache not reseting SQLite statements on error.
* subversion/libsvn_fs_fs/rep-cache.c
(svn_fs_fs__walk_rep_reference): Move a reset earlier, catch errors
and reset before returning.
(svn_fs_fs__get_rep_reference): Move a reset earlier.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c?rev=1422042&r1=1422041&r2=1422042&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c Fri Dec 14 19:11:29 2012
@@ -151,16 +151,15 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *
/* Check global invariants. */
if (start == 0)
{
- svn_sqlite__stmt_t *stmt2;
svn_revnum_t max;
- SVN_ERR(svn_sqlite__get_statement(&stmt2, ffd->rep_cache_db,
+ SVN_ERR(svn_sqlite__get_statement(&stmt, ffd->rep_cache_db,
STMT_GET_MAX_REV));
- SVN_ERR(svn_sqlite__step(&have_row, stmt2));
- max = svn_sqlite__column_revnum(stmt2, 0);
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+ max = svn_sqlite__column_revnum(stmt, 0);
+ SVN_ERR(svn_sqlite__reset(stmt));
if (SVN_IS_VALID_REVNUM(max)) /* The rep-cache could be empty. */
SVN_ERR(svn_fs_fs__revision_exists(max, fs, iterpool));
- SVN_ERR(svn_sqlite__reset(stmt2));
}
SVN_ERR(svn_sqlite__get_statement(&stmt, ffd->rep_cache_db,
@@ -174,6 +173,7 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *
{
representation_t *rep;
const char *sha1_digest;
+ svn_error_t *err;
/* Clear ITERPOOL occasionally. */
if (iterations++ % 16 == 0)
@@ -181,21 +181,29 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *
/* Check for cancellation. */
if (cancel_func)
- SVN_ERR(cancel_func(cancel_baton));
+ {
+ err = cancel_func(cancel_baton);
+ if (err)
+ return svn_error_compose_create(err, svn_sqlite__reset(stmt));
+ }
/* Construct a representation_t. */
rep = apr_pcalloc(iterpool, sizeof(*rep));
sha1_digest = svn_sqlite__column_text(stmt, 0, iterpool);
- SVN_ERR(svn_checksum_parse_hex(&rep->sha1_checksum,
- svn_checksum_sha1, sha1_digest,
- iterpool));
+ err = svn_checksum_parse_hex(&rep->sha1_checksum,
+ svn_checksum_sha1, sha1_digest,
+ iterpool);
+ if (err)
+ return svn_error_compose_create(err, svn_sqlite__reset(stmt));
rep->revision = svn_sqlite__column_revnum(stmt, 1);
rep->offset = svn_sqlite__column_int64(stmt, 2);
rep->size = svn_sqlite__column_int64(stmt, 3);
rep->expanded_size = svn_sqlite__column_int64(stmt, 4);
/* Walk. */
- SVN_ERR(walker(rep, walker_baton, fs, iterpool));
+ err = walker(rep, walker_baton, fs, iterpool);
+ if (err)
+ return svn_error_compose_create(err, svn_sqlite__reset(stmt));
SVN_ERR(svn_sqlite__step(&have_row, stmt));
}
@@ -247,10 +255,12 @@ svn_fs_fs__get_rep_reference(representat
else
*rep = NULL;
+ SVN_ERR(svn_sqlite__reset(stmt));
+
if (*rep)
SVN_ERR(rep_has_been_born(*rep, fs, pool));
- return svn_sqlite__reset(stmt);
+ return SVN_NO_ERROR;
}
svn_error_t *
Re: svn commit: r1422042 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>>> URL: http://svn.apache.org/viewvc?rev=1422042&view=rev
>>
>>> Log:
>>> Fix issue 4210, rep-cache not reseting SQLite statements on error.
>>
>> I noticed the other day that WC DB code in many places fails to reset
>> stmts on error, but that svn_sqlite__get_statement() automatically
>> resets the stmt if it hasn't been reset since last used. Does this
>> tally with your understanding of what you just fixed in the rep-cache
>> -- that is, it could be described as a functionally harmless coding
>> style violation?
>
> I'm not really sure. We go to a lot of trouble to reset statements in
> some places. Perhaps it doesn't really matter in this case because the
> server only uses a small number of statements so there can never be a
> large number of statements needing to be reset?
>
>> If so it would be good to say so in log msg & issue to indicate it
>> doesn't need back-porting.
Bert wrote on IRC[1]:
> Forgetting to reset statements is not harmless... It might cause so
> much trouble that we try to recover from this problem in a few places
> where we can. Forgetting to reset causes the db to stay locked;
> transactions to fail in unexpected locations. In many cases this
> appears harmless for short living processes like svn, but really
> problematic for long living clients.
>
> The problem is that there are so many error conditions in our wc db
> code that we won't be able to cover them all in the test suite, so
> there will always be bugs. And that get statement (and the error
> handling in transactions, etc.) tries to keep things working after
> such bugs.
OK -- so it does matter. I will bear this in mind and maybe do something about it.
- Julian
[1] <http://colabti.org/irclogger/irclogger_log/svn-dev?date=2012-12-14#l365>
Re: svn commit: r1422042 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
>> URL: http://svn.apache.org/viewvc?rev=1422042&view=rev
>
>> Log:
>> Fix issue 4210, rep-cache not reseting SQLite statements on error.
>
> I noticed the other day that WC DB code in many places fails to reset
> stmts on error, but that svn_sqlite__get_statement() automatically
> resets the stmt if it hasn't been reset since last used. Does this
> tally with your understanding of what you just fixed in the rep-cache
> -- that is, it could be described as a functionally harmless coding
> style violation?
I'm not really sure. We go to a lot of trouble to reset statements in
some places. Perhaps it doesn't really matter in this case because the
server only uses a small number of statements so there can never be a
large number of statements needing to be reset?
> If so it would be good to say so in log msg & issue to indicate it
> doesn't need back-porting.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1422042 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
Posted by Julian Foad <ju...@btopenworld.com>.
> URL: http://svn.apache.org/viewvc?rev=1422042&view=rev
> Log:
> Fix issue 4210, rep-cache not reseting SQLite statements on error.
I noticed the other day that WC DB code in many places fails to reset stmts on error, but that svn_sqlite__get_statement() automatically resets the stmt if it hasn't been reset since last used. Does this tally with your understanding of what you just fixed in the rep-cache -- that is, it could be described as a functionally harmless coding style violation?
If so it would be good to say so in log msg & issue to indicate it doesn't need back-porting.
- Julian
> * subversion/libsvn_fs_fs/rep-cache.c
> (svn_fs_fs__walk_rep_reference): Move a reset earlier, catch errors
> and reset before returning.
> (svn_fs_fs__get_rep_reference): Move a reset earlier.
> Modified: subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
> /* Check global invariants. */
> if (start == 0)
> {
> - svn_sqlite__stmt_t *stmt2;
> svn_revnum_t max;
>
> - SVN_ERR(svn_sqlite__get_statement(&stmt2, ffd->rep_cache_db,
> + SVN_ERR(svn_sqlite__get_statement(&stmt, ffd->rep_cache_db,
> STMT_GET_MAX_REV));
> - SVN_ERR(svn_sqlite__step(&have_row, stmt2));
> - max = svn_sqlite__column_revnum(stmt2, 0);
> + SVN_ERR(svn_sqlite__step(&have_row, stmt));
> + max = svn_sqlite__column_revnum(stmt, 0);
> + SVN_ERR(svn_sqlite__reset(stmt));
> if (SVN_IS_VALID_REVNUM(max)) /* The rep-cache could be empty. */
> SVN_ERR(svn_fs_fs__revision_exists(max, fs, iterpool));
> - SVN_ERR(svn_sqlite__reset(stmt2));
[...]