You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by John Szakmeister <jo...@szakmeister.net> on 2003/08/01 00:37:02 UTC
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
On Thursday 31 July 2003 10:14, cmpilato@collab.net wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > Here is take 2 on this patch. I re-worked a couple of things. I
> > removed the typedef for the function prototype and changed it to
> > match retry_txn's style of decleration, and fixed some of the
> > formatting problems I had before (I hopefully got them all this time
> >
> > :-).
>
> This patch doesn't apply cleanly for me. I get failed hunks in
> svn_fs.h and fs.c.
Weird my copy of the patch seems to be fine... maybe I did something wrong...
for some reason the one that I posted is slightly different that the one
stored on my drive. I'll have to keep an eye on this... perhaps my e-mail
client is messing this up. Hopefully this one applies cleanly!
>
> > Index: subversion/include/svn_fs.h
> > ===================================================================
> > --- subversion/include/svn_fs.h (revision 6521)
> > +++ subversion/include/svn_fs.h (working copy)
> > @@ -1327,18 +1327,30 @@
> >
> > /** Populate @a *uuid with the UUID associated with @a fs. */
> > svn_error_t *
> > -svn_fs_get_uuid(svn_fs_t *fs,
> > - const char **uuid,
> > - apr_pool_t *pool);
> > +svn_fs_get_uuid (svn_fs_t *fs,
> > + const char **uuid,
> > + apr_pool_t *pool);
> >
> >
> > /** Associate @a *uuid with @a fs. */
> > svn_error_t *
> > -svn_fs_set_uuid(svn_fs_t *fs,
> > - const char *uuid,
> > - apr_pool_t *pool);
> > +svn_fs_set_uuid (svn_fs_t *fs,
> > + const char *uuid,
> > + apr_pool_t *pool);
>
> These should really be in a separate patch, but whatever.
Removed. I'll send in another one to fix that.
>
> > +/* Verify */
>
> Really should toss a ^L above this line (and maybe "Verification" --
> note that other headings are nouns).
The diff actually borrowed the one that used be there for "Non-historical
Properties" and added another at the end of the block. I only mention it
because this patch looks the same way. BTW, changed "Verify" to "Content
Verification."
>
> > + /* Create a database cursor to list the transaction names. */
> > + SVN_ERR (BDB_WRAP (fs, "reading transaction list (opening cursor)",
> > + fs->representations->cursor (fs->representations,
> > + trail->db_txn,
> > + &cursor,
> > + 0)));
> > + /* Build a null-terminated array of keys in the transactions table. */
>
> Hmm, I wonder if maybe you copied this code from the ^^^^^^^^^^^^^^^^^^
Why would you ever think that? :-)
>
> (I think you missed some s/transactions/representations/)
I fixed up the comments better. Hopefully they tell the truth now. :-)
>
> > Index: subversion/libsvn_fs/bdb/reps-table.h
> > ===================================================================
> > --- subversion/libsvn_fs/bdb/reps-table.h (revision 6521)
> > +++ subversion/libsvn_fs/bdb/reps-table.h (working copy)
> > @@ -74,6 +74,17 @@
> > trail_t *trail);
> >
> >
> > +
> > +/*** Walking the representation table. ***/
>
> No need for the section boundary -- this is still "storing and
> retrieving reps".
Removed.
>
> > Index: subversion/libsvn_fs/fs.c
> > ===================================================================
> > --- subversion/libsvn_fs/fs.c (revision 6521)
> > +++ subversion/libsvn_fs/fs.c (working copy)
> > @@ -33,6 +33,8 @@
> > #include "err.h"
> > #include "dag.h"
> > #include "svn_private_config.h"
> > +#include "reps-strings.h"
> > +#include "svn_string.h"
>
> Oops. Leftover #include from the previous attempt at this patch. You
> don't actually use svn_string_t's anymore.
Heh, definitely missed that. Sometimes no matter how hard you look they slip
right on by.
>
> > +
> > +/* A simple callback function used when initiating the
> > + walking of the repository */
> > +static svn_error_t *
> > +txn_body_verify_reps (void *baton,
> > + trail_t *trail)
> > +{
> > + svn_fs_t *fs = baton;
> > +
> > + SVN_ERR (svn_fs__bdb_walk_reps (fs, verify_reps_handler, trail));
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +/* The exposed function used to kick off the verification process */
> > +svn_error_t *
> > +svn_fs_verify_contents (svn_fs_t *fs,
> > + apr_pool_t *pool)
> > +{
> > + SVN_ERR (svn_fs__retry_txn (fs, txn_body_verify_reps, fs, pool));
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> We don't typically use docstrings on the implementation of public
> functions (that's just asking for them to get out of sync with the
> public header file's docstring of the same function. And we don't put
> docstrings with the txn_body_ functions either, since their names and
> types are dead giveaways as to what they exist to do.
Understood. Removed the docstrings.
>
> > Index: subversion/libsvn_fs/reps-strings.h
> > ===================================================================
> > --- subversion/libsvn_fs/reps-strings.h (revision 6521)
> > +++ subversion/libsvn_fs/reps-strings.h (working copy)
> > @@ -103,7 +103,9 @@
> >
> > If USE_TRAIL_FOR_READS is TRUE, the stream's reads are part
> > of TRAIL; otherwise, each read happens in an internal, one-off
> > - trail (though TRAIL is still required). POOL may be TRAIL->pool. */
> > + trail (though TRAIL is still required). POOL may be TRAIL->pool.
> > +
> > + Note: this function will verify the checksum as chunks are being
> > read. */
>
> "... and return the error SVN_ERR_FS_CORRUPT if the stored checksum
> doesn't match the one calculated from the data."
Hope you don't mind me copying that. :-)
>
> > +/* This implements `svn_opt_subcommand_t'. */
> > +static svn_error_t *
> > +subcommand_verify (apr_getopt_t *os, void *baton, apr_pool_t *pool)
> > +{
> > +
> > + struct svnadmin_opt_state *opt_state = baton;
> > + svn_repos_t *repos;
> > + svn_fs_t *fs;
> > +
> > + SVN_ERR (svn_repos_open (&repos, opt_state->repository_path, pool));
> > + fs = svn_repos_fs (repos);
> > +
> > + SVN_ERR (svn_fs_verify_contents (fs, pool));
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> Nits: your indentation is off here (function bodies start at column
> 2). Also, you could avoid the 'fs' convenience variable and do:
Grrr! SlickEdit is driving me crazy with this. Must learn to use emacs,
again!
>
> subcommand_verify (apr_getopt_t *os, void *baton, apr_pool_t *pool)
> {
> struct svnadmin_opt_state *opt_state = baton;
> svn_repos_t *repos;
>
> SVN_ERR (svn_repos_open (&repos, opt_state->repository_path, pool));
> SVN_ERR (svn_fs_verify_contents (svn_repos_fs (repos), pool));
> return SVN_NO_ERROR;
> }
Try this patch and see if things look any better.
-John
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
On Wednesday 06 August 2003 00:48, cmpilato@collab.net wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > Perhaps the best thing to do is recommend that people run 'svnadmin dump'
> > every once in a while to make sure nothing has been corrupted.
>
> Ooh! Here's a thought. What if we added a --dry-run option to
> 'svnadmin dump'? That way at least folks don't pay the cost of the
> output I/O. Basically, this option would cause the "stream" passed to
> svn_repos_dump to be NULL, and we'd just check for non-NULL before
> each call to svn_stream_printf() and svn_stream_write() in
> libsvn_repos/dump.c. Note that we'd still do the *read* of the
> file/directory data, so we get the checksum verification. Also, we
> still pass a valid "feedback_stream" to the function, so we can see
> progress and warnings.
Hehe, I was actually going to mention something like that but I thought I'd
better check and make sure that feature wasn't already there. :-)
Yes, I think that is a better way to go... and it's something that I'd use.
:-) Should I take a stab at doing this?
-John
PS I'll be out for the following 4 days so it'll be a little bit before I get
started.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
[snip]
> > But if the purpose of the command is to check integrity, then let's
> > have its name reflect that. It may be 'dump --dry-run' under the
> > hood, but 'svnadmin verify' or something is a more accurate name for
> > what it does.
Great idea!
>
> Oh, +1. Good thought, Captain UI.
>
> John: go for it, and if that means starting 4 days from now ... well,
> I know *I* have plenty of other fish to fry in the meantime. :-)
Will do.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
On Thursday 14 August 2003 20:15, cmpilato@collab.net wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > > John: go for it, and if that means starting 4 days from now ... well,
> > > I know *I* have plenty of other fish to fry in the meantime. :-)
> >
> > So should I implement both the option and the subcommand, or just the
> > subcommand then? I'm personally leaning towards just the
> > subcommand.
>
> Just the subcommand. Thanks, dude.
Done. However, I only have a small test set so I'm not sure if my results are
reliable in any way. Right now I'm seeing only a small difference (<1
second) in the time it takes to verify the repository and to dump it to
/dev/null. Does someone have a repo that I can play with that contains more
revisions and paths? I'd really like to see if there is really going to be a
difference in performance or not.
Any suggestions for testing?
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by cm...@collab.net.
John Szakmeister <jo...@szakmeister.net> writes:
> > John: go for it, and if that means starting 4 days from now ... well,
> > I know *I* have plenty of other fish to fry in the meantime. :-)
>
> So should I implement both the option and the subcommand, or just the
> subcommand then? I'm personally leaning towards just the
> subcommand.
Just the subcommand. Thanks, dude.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
On Wednesday 06 August 2003 15:00, cmpilato@collab.net wrote:
[snip]
> kfogel@collab.net writes:
> > Cool implementation!
> >
> > But if the purpose of the command is to check integrity, then let's
> > have its name reflect that. It may be 'dump --dry-run' under the
> > hood, but 'svnadmin verify' or something is a more accurate name for
> > what it does.
>
> Oh, +1. Good thought, Captain UI.
>
> John: go for it, and if that means starting 4 days from now ... well,
> I know *I* have plenty of other fish to fry in the meantime. :-)
So should I implement both the option and the subcommand, or just the
subcommand then? I'm personally leaning towards just the subcommand.
I'll get to work on it soon... I'm still trying to catch up from my 2 days off
of work. :-)
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by cm...@collab.net.
kfogel@collab.net writes:
> cmpilato@collab.net writes:
> > Ooh! Here's a thought. What if we added a --dry-run option to
> > 'svnadmin dump'? That way at least folks don't pay the cost of the
> > output I/O. Basically, this option would cause the "stream" passed to
> > svn_repos_dump to be NULL, and we'd just check for non-NULL before
> > each call to svn_stream_printf() and svn_stream_write() in
> > libsvn_repos/dump.c. Note that we'd still do the *read* of the
> > file/directory data, so we get the checksum verification. Also, we
> > still pass a valid "feedback_stream" to the function, so we can see
> > progress and warnings.
>
> Cool implementation!
>
> But if the purpose of the command is to check integrity, then let's
> have its name reflect that. It may be 'dump --dry-run' under the
> hood, but 'svnadmin verify' or something is a more accurate name for
> what it does.
Oh, +1. Good thought, Captain UI.
John: go for it, and if that means starting 4 days from now ... well,
I know *I* have plenty of other fish to fry in the meantime. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by kf...@collab.net.
cmpilato@collab.net writes:
> Ooh! Here's a thought. What if we added a --dry-run option to
> 'svnadmin dump'? That way at least folks don't pay the cost of the
> output I/O. Basically, this option would cause the "stream" passed to
> svn_repos_dump to be NULL, and we'd just check for non-NULL before
> each call to svn_stream_printf() and svn_stream_write() in
> libsvn_repos/dump.c. Note that we'd still do the *read* of the
> file/directory data, so we get the checksum verification. Also, we
> still pass a valid "feedback_stream" to the function, so we can see
> progress and warnings.
Cool implementation!
But if the purpose of the command is to check integrity, then let's
have its name reflect that. It may be 'dump --dry-run' under the
hood, but 'svnadmin verify' or something is a more accurate name for
what it does.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by cm...@collab.net.
John Szakmeister <jo...@szakmeister.net> writes:
> Perhaps the best thing to do is recommend that people run 'svnadmin dump'
> every once in a while to make sure nothing has been corrupted.
Ooh! Here's a thought. What if we added a --dry-run option to
'svnadmin dump'? That way at least folks don't pay the cost of the
output I/O. Basically, this option would cause the "stream" passed to
svn_repos_dump to be NULL, and we'd just check for non-NULL before
each call to svn_stream_printf() and svn_stream_write() in
libsvn_repos/dump.c. Note that we'd still do the *read* of the
file/directory data, so we get the checksum verification. Also, we
still pass a valid "feedback_stream" to the function, so we can see
progress and warnings.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
On Tuesday 05 August 2003 11:26, cmpilato@collab.net wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > How do I get 'next-key' value from the repo then? I mean, right now I'm
> > building a cursor into the repo, and close it when I'm done iterating.
> > Or, are you saying I should create a function to get called by
> > retry_txn() that will take a current key and return the next-key?
> > Perhaps a NULL value for the current key will mean to grab the first key
> > and return it?
>
> We store a row in the table whose key is "next-key" (or the symbol
> svn_fs_next_key_key). So, you might need a new function down in
> bdb/reps-table.c, perhaps svn_fs_bdb__rep_next_key(), to fetch the
> value of this key.
Sounds fair.
[snip]
> > Sounds easy enough. :-) Although, while we're talking about some of
> > the pros and cons over one method versus another, I have a slight
> > issue with something. For me, I'd like to know that when I type
> > 'svnadmin verify' that everything checks out okay. If we only walks
> > the reps table and check checksums, all we are doing is verifying
> > that the data for a file is correct. Technically, there could be a
> > problem in the node representation and you won't have a clue until
> > you do an 'svnadmin dump' or try to access that particular node
> > revision.
>
> HahaHAHAha. I don't know *how many* times I've started to type this
> same concern to you, but didn't.
>
> See, the original issue report complains that a repository admin won't
> know about brokenness in his repository until running svnadmin dump.
> So, the reaction was to say, "Oh, we need 'svnadmin verify', and it
> should verify checksums." But my respose to that (which I apologize
> for not airing sooner) was that if you're going to write a subcommand
> specifically about verifying the data in your repos, it really should
> check everything, not just file contents. But here's the thing --
> 'svnadmin dump' already effectively does this, so why write another
> subcommand?
That's pretty much what I thought. Actually, we have 3 svn repositories at my
place of business now, and I perform a hot-backup and a dump of the
repository every night... I'm not sure I would alter the script to perform
the 'svnadmin verify' command the way we've done it thus far.
[snip]
> I dunno, dude. I'm tempted to recommend that we not add the code
> clutter of this extra verification step. My gut instinct is: if we're
> writing whole new codepaths just to answer the "Would my dump fail?"
> question, and those codepaths are gonna take every bit as long to run
> as just running 'svnadmin dump', we're wasting our time and effort.
I tend to agree. If we aren't going to get much out of this, or we can't say
without a doubt that an 'svnadmin dump' would fail, then I have hard time
accepting that we should put this code in. It's more to maintain with little
payoff.
Perhaps the best thing to do is recommend that people run 'svnadmin dump'
every once in a while to make sure nothing has been corrupted.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by cm...@collab.net.
John Szakmeister <jo...@szakmeister.net> writes:
> How do I get 'next-key' value from the repo then? I mean, right now I'm
> building a cursor into the repo, and close it when I'm done iterating. Or,
> are you saying I should create a function to get called by retry_txn() that
> will take a current key and return the next-key? Perhaps a NULL value for
> the current key will mean to grab the first key and return it?
We store a row in the table whose key is "next-key" (or the symbol
svn_fs_next_key_key). So, you might need a new function down in
bdb/reps-table.c, perhaps svn_fs_bdb__rep_next_key(), to fetch the
value of this key.
>
> > 3. Call svn_fs__prev_key()* on that value (this will be "the current
> > key"). 4. Try to fetch the representation at the current key. If you
> > fail, no sweat.
> > 5. But if you succeed, read the rep data (which checks the checksums).
> > 6. If "the current key" != "0", goto step 3 and repeat, otherwise,
> > you're done.
> >
> > So, at most, the only time you spend in a given trail is the time
> > taken to fetch a single representation, or the time taken to read a
> > chunk of data from it.
> >
> > Questions? Comments? Are you ready to throw up your hands and quit
> > yet? (please say no -- you've been a terrific sport through this)
> >
> > :-)
>
> Nah, I like SVN... I'm here to stay. :-)
Well, that's certainly great to hear.
> Sounds easy enough. :-) Although, while we're talking about some of
> the pros and cons over one method versus another, I have a slight
> issue with something. For me, I'd like to know that when I type
> 'svnadmin verify' that everything checks out okay. If we only walks
> the reps table and check checksums, all we are doing is verifying
> that the data for a file is correct. Technically, there could be a
> problem in the node representation and you won't have a clue until
> you do an 'svnadmin dump' or try to access that particular node
> revision.
HahaHAHAha. I don't know *how many* times I've started to type this
same concern to you, but didn't.
See, the original issue report complains that a repository admin won't
know about brokenness in his repository until running svnadmin dump.
So, the reaction was to say, "Oh, we need 'svnadmin verify', and it
should verify checksums." But my respose to that (which I apologize
for not airing sooner) was that if you're going to write a subcommand
specifically about verifying the data in your repos, it really should
check everything, not just file contents. But here's the thing --
'svnadmin dump' already effectively does this, so why write another
subcommand?
Let's walk through the design of the perfect verifier. There are
various ways to do this -- various levels at which we can slip into
the filesystem, requiring various degrees of trust that at least
*something* in the filesystem is infallible. Here are a few
suggestions, with some pro/con commentary.
(a) for revision in revisions:
- crawl the tree under that revision, verifying dirs and files
for txn in txns:
if not txn.committed:
- do the same tree check
# This algorithm would cover verification of the tree structure of
# the filesystem, both for committed and unfinished transactions
# (though, we could drop the checks of unfinished transactions).
# The major problem with the algorithm is that because of our DAG
# filesystem layout, we'll be doing a **TON** of unnecessary
# checks. So, we revise.
(b) for revision in revisions:
- crawl the tree under that revision, verifying dirs and files
that were created or modified in this revision
for txn in txns:
if not txn.committed:
- do the same tree check
# Okay, now we aren't doing so much redundant verification. This
# is a definate improvement, but isn't really a full solution.
# Why? We haven't verified copies or change records yet, both of
# which are crucial to 'svnadmin dump'. Revise.
(c) for revision in revisions:
- read the changes associated with this revisions's underlying txn
- crawl the tree under that revision, verifying dirs and files
that were created or modified in this revision, and
checking that non-bubble-up changes and copies are represented
in the changes list
for txn in txns:
if not txn.committed:
- do the same tree check
# Getting better, getting better. This algorithm doesn't verify
# 'delete' change records (since you can't "see" a deletion by
# walking a tree). I'm not sure exactly how to verify deletions,
# save maybe checking that path@revision+1 either doesn't exist, or
# does exist as a new thing.
As you can see, to come even close to doing verification correctly,
you effectively must verify each tree under each revision, preferably
without verifying the same data more than once. But when it's all
said and done, this is pretty much what 'svnadmin dump' *does*.
I dunno, dude. I'm tempted to recommend that we not add the code
clutter of this extra verification step. My gut instinct is: if we're
writing whole new codepaths just to answer the "Would my dump fail?"
question, and those codepaths are gonna take every bit as long to run
as just running 'svnadmin dump', we're wasting our time and effort.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
On Monday 04 August 2003 10:40, cmpilato@collab.net wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > Any recommendations? I can't say that I'm very familiar with Berkeley's
> > methodology, so I'm at a loss for any sort of suggestion, other than to
> > say that 'svnadmin dump' seems to work okay. :-) What does that code
> > path do differently than this technique (it calls svn_repos_dir_delta()
> > to iterate over the contents)? I've never seen an instance where it
> > fails.
>
> Actually, these days 'svnadmin dump' uses svn_repos_replay() (the new
> and improved fork of svn_repos_dir_delta). The reason we don't lock
> issues with this, as with most other filesystem functionalities, is
> because we use the Berkeley locks (which, for the purposes of this
> discussion, can be mapped to Subversion's trails) for a short time
> each -- we get into the database, find some smallish piece of data (like
> a directory entries list, the change records for a specific revision),
> and get outta there. So what happens is that we are calling
> begin_trail() and commit_trail() crazy number of times, but each trail
> is used for a relatively small task.
>
> Your patch, however, creates a trail at the start of processing, and
> we keep it open while we read every representation and string in the
> database -- an arbitrarily large dataset. (And I'm not very clear on
> this, but I think Berkeley locks things per-page).
>
> The downside is that the solution is nasty. It requires that you:
Given the above comments about the locking, I'd have to agree... it is a nasty
solution.
> 1. Lose the representations walker. Instead you are now doing all
> your work up in the main svn_fs_verify() command.
> 2. Use a trail to get the 'next-key' value from the repo.
How do I get 'next-key' value from the repo then? I mean, right now I'm
building a cursor into the repo, and close it when I'm done iterating. Or,
are you saying I should create a function to get called by retry_txn() that
will take a current key and return the next-key? Perhaps a NULL value for
the current key will mean to grab the first key and return it?
> 3. Call svn_fs__prev_key()* on that value (this will be "the current
> key"). 4. Try to fetch the representation at the current key. If you
> fail, no sweat.
> 5. But if you succeed, read the rep data (which checks the checksums).
> 6. If "the current key" != "0", goto step 3 and repeat, otherwise,
> you're done.
>
> So, at most, the only time you spend in a given trail is the time
> taken to fetch a single representation, or the time taken to read a
> chunk of data from it.
>
> Questions? Comments? Are you ready to throw up your hands and quit
> yet? (please say no -- you've been a terrific sport through this)
>
> :-)
Nah, I like SVN... I'm here to stay. :-)
Sounds easy enough. :-) Although, while we're talking about some of the pros
and cons over one method versus another, I have a slight issue with
something. For me, I'd like to know that when I type 'svnadmin verify' that
everything checks out okay. If we only walks the reps table and check
checksums, all we are doing is verifying that the data for a file is correct.
Technically, there could be a problem in the node representation and you
won't have a clue until you do an 'svnadmin dump' or try to access that
particular node revision.
> *svn_fs__prev_key() doesn't really exist, at least not in the trunk
> code. I wrote it (thinking I needed it) on the fs-schema-changes
> branch. You can snarf it from:
>
>
> http://svn.collab.net/repos/svn/branches/fs-schema-changes/subversion/libsv
>n_fs/key-gen.c
> http://svn.collab.net/repos/svn/branches/fs-schema-changes/subversion/libsv
>n_fs/key-gen.h
I'll take a look at this soon.
-John
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by cm...@collab.net.
John Szakmeister <jo...@szakmeister.net> writes:
> Any recommendations? I can't say that I'm very familiar with Berkeley's
> methodology, so I'm at a loss for any sort of suggestion, other than to say
> that 'svnadmin dump' seems to work okay. :-) What does that code path do
> differently than this technique (it calls svn_repos_dir_delta() to iterate
> over the contents)? I've never seen an instance where it fails.
Actually, these days 'svnadmin dump' uses svn_repos_replay() (the new
and improved fork of svn_repos_dir_delta). The reason we don't lock
issues with this, as with most other filesystem functionalities, is
because we use the Berkeley locks (which, for the purposes of this
discussion, can be mapped to Subversion's trails) for a short time
each -- we get into the database, find some smallish piece of data (like
a directory entries list, the change records for a specific revision),
and get outta there. So what happens is that we are calling
begin_trail() and commit_trail() crazy number of times, but each trail
is used for a relatively small task.
Your patch, however, creates a trail at the start of processing, and
we keep it open while we read every representation and string in the
database -- an arbitrarily large dataset. (And I'm not very clear on
this, but I think Berkeley locks things per-page).
The downside is that the solution is nasty. It requires that you:
1. Lose the representations walker. Instead you are now doing all
your work up in the main svn_fs_verify() command.
2. Use a trail to get the 'next-key' value from the repo.
3. Call svn_fs__prev_key()* on that value (this will be "the current key").
4. Try to fetch the representation at the current key. If you
fail, no sweat.
5. But if you succeed, read the rep data (which checks the checksums).
6. If "the current key" != "0", goto step 3 and repeat, otherwise,
you're done.
So, at most, the only time you spend in a given trail is the time
taken to fetch a single representation, or the time taken to read a
chunk of data from it.
Questions? Comments? Are you ready to throw up your hands and quit
yet? (please say no -- you've been a terrific sport through this)
:-)
*svn_fs__prev_key() doesn't really exist, at least not in the trunk
code. I wrote it (thinking I needed it) on the fs-schema-changes
branch. You can snarf it from:
http://svn.collab.net/repos/svn/branches/fs-schema-changes/subversion/libsvn_fs/key-gen.c
http://svn.collab.net/repos/svn/branches/fs-schema-changes/subversion/libsvn_fs/key-gen.h
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
On Monday 04 August 2003 02:59, cmpilato@collab.net wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > My e-mail client was definitely messing up the patch. This should work
> > better.
>
> Oh yeah. That applied cleanly. And for what it's worth, "well done"
> to you.
Thanks! And thanks for being patient with all of the nits. :-) I'm still
trying to get used to svn's style... and modifying my editor so that I don't
have to think about it as much. :-)
> But now (and here is where you can start hating me for not thinking of
> this earlier) I have a fear about the algorith. Well, not the
> algorithm -- it's the fact that this command will be reading the
> entire 'representations' table, and all the strings dangling off those
> representations, in a single Berkeley DB transaction. On any
> decently-sized repository, I have a feeling this operation will fail
> to complete, instead dying due to a lack of available locks.
>
> How big a dataset have you been testing this patch on? My fears may
> turn out to be irrational, but something nagging at the back of my
> mind warns me of otherwise.
I took my copy of subversion and imported it into a repository. Not much of a
dataset since it doesn't have multiple revisions, and nothing else is going
on while I'm verifying the repo (not that concurrency should be an issue).
Any recommendations? I can't say that I'm very familiar with Berkeley's
methodology, so I'm at a loss for any sort of suggestion, other than to say
that 'svnadmin dump' seems to work okay. :-) What does that code path do
differently than this technique (it calls svn_repos_dir_delta() to iterate
over the contents)? I've never seen an instance where it fails.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by cm...@collab.net.
John Szakmeister <jo...@szakmeister.net> writes:
> My e-mail client was definitely messing up the patch. This should work
> better.
Oh yeah. That applied cleanly. And for what it's worth, "well done"
to you.
But now (and here is where you can start hating me for not thinking of
this earlier) I have a fear about the algorith. Well, not the
algorithm -- it's the fact that this command will be reading the
entire 'representations' table, and all the strings dangling off those
representations, in a single Berkeley DB transaction. On any
decently-sized repository, I have a feeling this operation will fail
to complete, instead dying due to a lack of available locks.
How big a dataset have you been testing this patch on? My fears may
turn out to be irrational, but something nagging at the back of my
mind warns me of otherwise.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1074: Need a svnadmin command to verify that the repository is not corrupted (Take 2)
Posted by John Szakmeister <jo...@szakmeister.net>.
My e-mail client was definitely messing up the patch. This should work
better.
--------------------------------------
Resolves Issue #1074 (Need a svnadmin command to verify that the repository
is not corrupted).
* subversion/libsvn_fs/fs.c
(svn_fs_verify_contents): Added this high level function to verify the data
stored in the fs.
(txn_body_verify_reps): Helper function.
(verity_reps_handler): Helper function.
* subversion/include/svn_fs.h
(svn_fs_verify_contents): Added function prototype.
* subversion/libsvn_fs/bdb/reps-table.c
(svn_fs__bdb_walk_reps): Added this utility function to walk the
representation table calling a callback function for each row.
* subversion/libsvn_fs/bdb/reps-table.h
(svn_fs__bdb_walk_reps): Added function prototype.
(svn_fs__rep_contents_read_stream): Modified docstring to reflect the
function's responsibility to verify the checksum.
* subversion/svnadmin/main.c
(subcommand_verify): Added new function to verify the data stored in the
repository.