Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Subject: Re: verify_as_revision_before_current_plus_plus() on a production
 repo?
To: Daniel Shahaf <d.s@daniel.shahaf.name>
References: <2116a47e-5a69-9f77-3d7f-05013f2af73e@apache.org>
 <20170131021918.GA4418@fujitsu.shahaf.local2>
Cc: dev@subversion.apache.org, Stefan Sperling <stsp@elego.de>
From: Julian Foad <julianfoad@apache.org>
Message-ID: <737c4203-29f6-f172-2e3b-ec53642e7e52@apache.org>
Date: Tue, 31 Jan 2017 10:22:06 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <20170131021918.GA4418@fujitsu.shahaf.local2>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
archived-at: Tue, 31 Jan 2017 10:22:11 -0000

Daniel, thanks for your comments.

Daniel Shahaf wrote:
> Julian Foad wrote on Mon, Jan 30, 2017 at 21:40:00 +0000:
>> it is a little "tricky": in particular, it opens a second FS instance and
>> fakes the "youngest revision seen" field and then relies on this FS instance
>> never reading the "current" file
>
> Right: if some code refreshes youngest_rev_cache (by open()ing
> 'current', reading a value from it, and setting youngest_rev_cache to
> that value), that will cause root->rev to be "newer than youngest".
>
> I think the 'verify' code already has to deal with this possibility,
> since 'recover' can backdate 'current' in the following situation:
> .
>     1. svn_fs.h consumer opens an svn_fs_root_t for r42
>     2. invisible monkeys delete db/revs/0/42 and db/current
>     3. admin runs 'svnadmin recover', which regenerates 'current' as 41
>     4. svn_fs.h consumer calls verify() on the root it had opened earlier

I don't buy this. verify_root() is not required to successfully verify 
r42 in this scenario.

> Moreover, the 'verify' code is inherently the place where violated
> invariants are least likely to cause trouble, and it's read-only.
> Therefore, while a bug might cause a false positive verification error
> that rejects a commit, I don't see any worse outcome.  (If there's
> a failure mode here that I overlooked, it's most likely to be in the f7
> code, since I haven't worked much with those parts of fsfs.)
>
> All that said, I agree that checking after the _verify_root() call that
> root->rev and youngest_rev_cache haven't changed would be an improvement.
[...]
>
>> nor using anything that would have been done post-commit.
>
> That's a good point: the svn_fs_fs__verify_root() must not add any
> permanent references to the revision it thinks is youngest — e.g., it
> must not add reps it traverses to rep-cache.db.  That's true today but
> not necessarily true forever.

Interesting observation. It would be good the verify could use a 
read-only FS instance, but I don't think we have such a mechanism.

I meant the opposite direction: a successful commit does some things 
post-commit (e.g. remove the txn directory, update the fulltext cache, 
update the rep cache) and this verify must not assume any of those 
things have been done already.

> I also wonder if having verify_as_revision_before_current_plus_plus()
> run in a child process would gain anything.
>
>> verify_as_revision_before_current_plus_plus() is currently compiled in to
>> debug builds but not to release builds. We can say therefore it gets
>> reasonable coverage in test suite runs but has had little or no real-world
>> testing.
>
> Indeed.  How about enabling that function in the alpha1 release so we
> can get some more feedback about it?

I like that idea: it seems like entirely appropriate behaviour for an 
alpha or beta release, and we'd probably get no direct feedback and this 
would be a good sign that it's working ok.

- Julian