Re: [Issue 4129] predecessors links on root node-revision skip revisions

On Wed, Jul 25, 2012 at 2:09 PM, Daniel Shahaf <danielsh_at_elego.de> wrote:

> Philip Martin wrote on Wed, Jul 25, 2012 at 12:06:48 +0100:
> > Daniel Shahaf <danielsh_at_elego.de> writes:
> >
> > > Philip Martin wrote on Wed, Jul 25, 2012 at 11:09:54 +0100:
> > >> Daniel Shahaf <danielsh_at_elego.de> writes:
> > >>
> > >> > Philip Martin wrote on Wed, Jul 25, 2012 at 10:49:36 +0100:
> > >> >>
> > >> >> I attached to the issue a repository that demonstrates the
> corruption;
> > >> >> verify doesn't report a problem on that repository. What does
> verify
> > >> >> check?
> > >> >
> > >> > validate_root_noderev() catches an instance of the #4129 corruption
> and
> > >> > in its docstring xrefs svn_fs_fs__verify(), so I think the checks
> in the
> > >> > former are done in the latter too.
> > >>
> > >> validate_root_noderev is only called by write_final_rev and not during
> > >> verify. svn_fs_fs__verify calls svn_fs_fs__verify_root and that does:
> > >>
> > >> /* Issue #4129: bogus predecessors. */
> > >> /* Check 1: predecessor must be an earlier revision.
> > >> */
> > >> if (pred_rev >= root->rev)
> > >> return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
> > >> "r%ld's root node's predecessor is
> r%ld"
> > >> " but must be earlier revision",
> > >> root->rev, pred_rev);
> > >
> > > The check was included in svn_fs_fs__verify_root() in r1349313:
> > >
> > > /* Verify explicitly the predecessor of the root. */
> > > {
> > > ...
> > > /* Check the predecessor's revision. */
> > > if (pred_id)
> > > {
> > > SVN_ERR(svn_fs_fs__dag_get_node(&pred, root->fs, pred_id,
> pool));
> > > SVN_ERR(svn_fs_fs__dag_get_revision(&pred_rev, pred, pool));
> > > if (pred_rev+1 != root->rev)
> > > /* Issue #4129. */
> > > return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
> > > "r%ld's root node's predecessor is
> r%ld",
> > > root->rev, pred_rev);
> > > }
> > > {
> >
> > That check was relaxed by:
> >
> > r1358322 | stefan2 | 2012-07-06 19:04:09 +0100 (Fri, 06 Jul 2012) | 5
> lines
> >
> > Relax overly strict consistency check that is not covered by the FSFS
> > format specification.
> >
> > * subversion/libsvn_fs_fs/tree.c
> > (svn_fs_fs__verify_root): fix test and add commentary
> >
>
> Thanks for digging it. I think that change is wrong --- the predecessor
> of the root noderev of rN MUST be the root noderev of r(N-1) --- that's
> how Subversion filesystems behave (not just FSFS).
>
> Stefan2, WDYT?
>

This is why I did that change: I knew that the usual
type of #4129 corruption is a ridiculous large rev number.
Now, I saw a delta of 2 instead of 1 on some of my repos.
My reaction: "Of course! That is must be a side-effect
of directory deltification."

Yesterday, I debugged the code and found out why r(N-2)
would be reported. This was due to is-fresh-txn-root
being set on some of the root noderevs. Some of the
affected repositories don't use directory deltification.
Maybe, I'm able to look deeper into how that might
have happened.

I think we found another form of corruption. Since the
fix is simply to ignore the flag, the question is whether
we may ignore it during (de-)serializing noderevs.

rI365918 reverts the relaxation.

-- Stefan^2.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download