Hyrum K Wright wrote on Wed, Jul 06, 2011 at 14:30:15 -0500:
> On Wed, Jul 6, 2011 at 11:47 AM, Daniel Shahaf <danie...@elego.de> wrote:
> > This thread is now the only non-FSFS release blocker (filed as #3944).
> > Last I checked there were at least three solutions suggested, but no
> > consensus on which solution to implement.
> >
> > Some suggestions were
> >
> >
> > 0. Leave things as they are
> >
> > 1. Allow packing revisions without packing revprops.
> > (revprops/ remains as in 1.6/f4)
> >
> > 2. Have all revprops in the DB all the time, never in plain files.
> >
> > 3. Swap the DB for some other "A thousand revision's revprops in one
> > file" solution. [several suggestions as to that file's format]
> >
> >
> > Can we decide on what to do here?
> >
> > Thanks,
> >
> > Daniel
> >
> >
> > ---------
> >
> > My opinion:
> >
> > * (1) is orthogonal to the others, but may be a good idea if we refactor
> > the FS so shortly before the release
> >
> > * (2) simplifies things, doesn't solve the problems with having an
> > SQLite db authoritative for parts of the FS storage
> > (read: cp(1) unsafe)
> >
> > * (3) has my +1, assuming it's sufficiently performant and the concrete
> > design is reasonable
> >
> > * (0) would mean that if we refactor revprop storage later, 1.8 servers
> > will have to try and read revprops from *three* places; and lots of
> > headache in the upgrade (and read-from-a-being-upgraded-FS) codepaths.
> > So, if f5 should be improved, I'd rather do that /before/ it's
> > released (and has to be indefinitely supported).
>
> After a bit of thinking and discussion, Daniel and I have come up with
> what we think is an acceptable solution, and I'm posting it here for
> validation. (Daniel, please correct me if I've gotten something
> wrong.)
>
Yes, that is what we discussed. I'll add some comments below.
> Revision properties will *not* be packed in an sqlite database, but
> will instead be packed in a single packfile, much like revision are to
> today. The key difference is that instead of having a separate
> manifest file, the manifest will be prepended to the packfile, meaning
> the two can be atomically replaced in the case of a propedit.
The manifest would consist of fixed-width records, to facilitate seeking
for rN's manifest entry: seek(record_size * (N % shard_size)).
> This solution has at least of couple of advantages:
> * No need to check a separate "edited" file before reading the packfile
> * The repo maintains consistency in the case of a filesystem copy
> (helpful for backups)
>
The process to edit a revprop that had been packed would be:
* grab write lock
* prepare a new pack-file-with-inline-manifest
* move-into-place, atomically replacing the previous pack file
* ungrab write lock
That is what guarantees cp(1) consistency that Hyrum mentions.
This also implies that propediting a revprop-that-had-been-packed will
have to rewrite a packfile containing a thousand revision's properties.
We expect that to be a reasonable cost given that revprop files are
small and historical revprops are rarely edited.
> Revprops wouldn't be packed until explicitly asked to do so by
> 'svnadmin pack' which means the frequent post-commit revprop editing
> wouldn't pose a performance problem. In addition, the revprop
> packfile manifest information won't be cached, since the manifest may
> change. We don't anticipate this to be a problem, since it only adds
> an extra seek() to the revprop lookup process (rather than the open()
> + seek() in the rev packing world).
>
Indeed. The svn_fs_revision_proplist() would do:
* when revision is packed:
+ open pack file
+ seek to manifest entry
+ seek to revision's hunk
+ svn_hash_read2() that
* when revision is not packed:
+ open the sharded file
- if it doesn't exist, refresh ffd->min_unpacked_revprop and retry
+ svn_hash_read2() that
We trade a cache lookup for a seek() within a file that we have to open
anyway.
> Comments?
