David Glasser wrote on Thu, 8 Oct 2009 at 13:47 -0700:
> On Thu, Oct 8, 2009 at 11:07 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > C. Michael Pilato wrote on Thu, 8 Oct 2009 at 12:51 -0400:
> >> Branko Cibej wrote:
> >> > The rep-cache database gets opened deep within svn_fs_fs__open and
> >> > svn_fs_fs__create. We don't really have a way to distinguish between
> >> > open-for-read and open-for-write in svn_fs_open. I can't form an opinion
> >> > right now on whether that's a serious omission or not, but in any case
> >> > adding an open-mode would be a huge conceptional change, of the svn-2.0
> >> > kind, IMHO.
> >>
> >> I can't see why we'd need to add different access modes. Why not simply
> >> make the code avoid opening the cache database until it is needed?
> >
> > I took a stab, see attached.
>
> Took a look at this and it generally looks good. I was concerned at
> first that there could be a downside to a failure to update the DB
> after a successful commit_body, but that was back from when the table
> had a "ref count" for each rep; the current code has a different more
> arbitrary uniquifier, so this should work.
>
Cool.
> This raises a question, though: do we even need to do
> write_reps_to_cache inside the FSFS write lock? I actually don't
> think so. [...]
Yes, write_reps_to_cache() only takes care of updating rep-cache.db, so
there is no real "need" for it to be inside the write lock.
(I'm not familiar with sqlite, but I'm assuming it will not have a problem
handling the resulting additional concurrent accesses to the DB.)
> I think that the DB has to obey this invariant:
>
> "If at any point, it is possible to read
> HASH=>(rev,offset,size,expanded_size) from the DB, then there must
> exist a rep for HASH at (rev,offset) with the given size and expanded
> size for the indefinite future."
>
> But it doesn't actually need to obey anything stronger than that: it's
> not necessary that that entry remain in the database, for example.
>
Agreed. (It also assumes that the HASH values of different reps don't
collide.)
> So yeah, your patch looks good but I'd go two steps farther and run
> the set_rep calls outside of the write lock and not in a transaction.
>
Per your other email, I'll move it outside the write lock.
As suggested upthread, we may still want to make the code ignore BUSY
errors when opening the DB (either for read or for write) --- this will
only be significant, however, in an environment where commits touch many
files per second (since the patch makes the DB only be opened by FS
writers).
> --dave
>
>
Thanks for the review!
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2405353
Received on 2009-10-09 00:22:52 CEST