You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2011/07/01 15:05:48 UTC
do away with db/revprops/*/, and a question about 'upgrade'
concurrency (was: Re: Does fsfs revprop packing no longer allow usage of
traditional backup software?)
Hyrum K Wright wrote on Thu, Jun 30, 2011 at 16:33:16 -0500:
> On Thu, Jun 30, 2011 at 3:27 PM, Peter Samuelson <pe...@p12n.org> wrote:
> >
> > [Ivan Zhakov]
> >> It should be easy to implement editing revprops without using SQLite:
> >> in case someone modify revprop non-packed revprop file is created, in
> >> read operation non-packed revprop file should be considered as more
> >> up-to-date. In next svnadmin pack operation these non-packed files
> >> should be merged back to packed one.
> >
> > +1. This would basically mean there's only _one_ code path for writing
> > revprops, yes? 'svnadmin pack' gets a little more complex, but the
> > rest of libsvn_fs_fs gets simpler.
> >
> > Anyone have time to actually do this? Converting the packed format
> > from sqlite to the same format used for packed revs would be a bonus.
>
> I like this idea, but it would seem to introduce an additional stat()
> call* for every attempt to fetch a revprop, because you'd first have
> to check the "old" location, and then the packed one.
I don't like the idea of writing revprops outside the DB and moving them
back in. (I think I already said why elsethread?)
But I do like the idea from IRC of doing away with revprop shards in f5
completely. Specifically: either ALL revprops will be in the DB, or
*all* revprops will be in the potentially-sharded plain files as in f4.
That does imply a single stat() for FS objects that were open whilst the
transition (to DB) was done --- the same as the dance that's being done
today for opening a revision file. (It might even be possible to
support bidirectional transition --- DB<->shards and back --- but that
can wait.)
Unrelated question: how do we handle the case of a client, linked to
libsvn_fs 1.6, that opens an svn_fs_t object and then, concurrently,
someone runs 'svnadmin1.7 upgrade'?
The FS object is now invalid (and must be closed), but would the old
client detect that? For that matter, does our current code error out if
db/format is bumped under its feet?
And also, the same question for a client linked to libsvn_fs 1.7, that
opened an f4 FSFS that had gotten upgraded whilst it was running.
(I'll answer these myself later, right now I just want to document them
before I forget them)
> As far as I can
> see, you'd have to do this in every case; in other words, there isn't
> a single-stat() short cut for the common case of non-edited revprops.
>
> -Hyrum
>
> * - I don't know why we seem to have this obsession with stat() calls
> around here, but it appears to have rubbed off on me.
Re: do away with db/revprops/*/, and a question about 'upgrade'
concurrency (was: Re: Does fsfs revprop packing no longer allow usage of
traditional backup software?)
Posted by Peter Samuelson <pe...@p12n.org>.
[Hyrum K Wright]
> My understanding would be that we'd pack revprops just like we pack
> revision (one single concat'd file per shard), and then store any
> edits in separate files. We'd then "repack" the edits into the pack
> files when an admin subsequently runs 'svnadmin pack'.
>
> Aside from the complexity of having to attempt to open the non-packed
> file everywhere before failing through to the common case, I really
> like this solution. (So much so that I may go implement it...)
At the risk of derailing this thread for about the third time ... if
you _do_ implement this, I suggest a simpler _and_ faster variation on
the 'manifest' file: instead of ASCII digits and \n characters, I
suggest 64-bit binary integers, stored in a defined endianness[*].
Easy to do, though you do need a 64-bit version of htonl()/ntohl().[**]
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
[*] Which endian? A bikeshed. Big-endian is traditional for
"portable" files and protocols, but the old big-endian server CPUs
(Cray, S/390, SPARC, MIPS, PA-RISC) seem to have all given way to
x86-64.
[**] Here's a 64-bit byte swapper based on something I found buried in
the apr source:
#if APR_IS_BIGENDIAN
#define UINT64_TO_FROM_BE(in,out) do { } while(0)
#else
#define UINT64_TO_FROM_BE(in,out) do { \
apr_uint64_t tmp = (in); \
tmp = ((tmp & APR_UINT64_C(0xff00ff00ff00ff00)) >> 8) | \
((tmp & APR_UINT64_C(0x00ff00ff00ff00ff)) << 8); \
tmp = ((tmp & APR_UINT64_C(0xffff0000ffff0000)) >> 16) | \
((tmp & APR_UINT64_C(0x0000ffff0000ffff)) << 16); \
(out) = (tmp >> 32) | (tmp << 32); \
} while(0)
#endif /* APR_IS_BIGENDIAN */
Re: do away with db/revprops/*/, and a question about 'upgrade' concurrency
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <da...@elego.de> writes:
> Where does that happen? svn_repos_upgrade2() calls get_repos() which
> calls lock_repos() which is a no-op if the underlying filesystem is
> FSFS.
My mistake, I skipped over the bdb-only line.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: do away with db/revprops/*/, and a question about 'upgrade'
concurrency
Posted by Daniel Shahaf <da...@elego.de>.
Philip Martin wrote on Fri, Jul 01, 2011 at 15:01:52 +0100:
> Daniel Shahaf <da...@elego.de> writes:
>
> > Unrelated question: how do we handle the case of a client, linked to
> > libsvn_fs 1.6, that opens an svn_fs_t object and then, concurrently,
> > someone runs 'svnadmin1.7 upgrade'?
> >
> > The FS object is now invalid (and must be closed), but would the old
> > client detect that? For that matter, does our current code error out if
> > db/format is bumped under its feet?
>
> If all the clients goes via libsvn_repos then the upgrade will block
> until there are no readers and then readers will block until the upgrade
> is complete, at which point 1.6 readers will fail because they don't
> support the upgraded format.
Where does that happen? svn_repos_upgrade2() calls get_repos() which
calls lock_repos() which is a no-op if the underlying filesystem is
FSFS.
Re: do away with db/revprops/*/, and a question about 'upgrade'
concurrency (was: Re: Does fsfs revprop packing no longer allow usage of
traditional backup software?)
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Fri, Jul 01, 2011 at 21:30:15 +0300:
> Daniel Shahaf wrote on Fri, Jul 01, 2011 at 19:29:50 +0300:
> > Hyrum K Wright wrote on Fri, Jul 01, 2011 at 11:21:58 -0500:
> > > On Fri, Jul 1, 2011 at 8:05 AM, Daniel Shahaf <da...@elego.de> wrote:
> > > > Hyrum K Wright wrote on Thu, Jun 30, 2011 at 16:33:16 -0500:
> > > >> On Thu, Jun 30, 2011 at 3:27 PM, Peter Samuelson <pe...@p12n.org> wrote:
> > > >> >
> > > >> > [Ivan Zhakov]
> > > >> >> It should be easy to implement editing revprops without using SQLite:
> > > >> >> in case someone modify revprop non-packed revprop file is created, in
> > > >> >> read operation non-packed revprop file should be considered as more
> > > >> >> up-to-date. In next svnadmin pack operation these non-packed files
> > > >> >> should be merged back to packed one.
> > > >> >
> > > >> > +1. This would basically mean there's only _one_ code path for writing
> > > >> > revprops, yes? 'svnadmin pack' gets a little more complex, but the
> > > >> > rest of libsvn_fs_fs gets simpler.
> > > >> >
> > > >> > Anyone have time to actually do this? Converting the packed format
> > > >> > from sqlite to the same format used for packed revs would be a bonus.
> > > >>
> > > >> I like this idea, but it would seem to introduce an additional stat()
> > > >> call* for every attempt to fetch a revprop, because you'd first have
> > > >> to check the "old" location, and then the packed one.
> > > >
> > > > I don't like the idea of writing revprops outside the DB and moving them
> > > > back in. (I think I already said why elsethread?)
> > >
> > > (I will assume "the DB" means "the SQLite-backed revprop database".)
> > >
> > > I agree with you, but I don't think that's what the proposal was
> > > about. My understanding would be that we'd pack revprops just like we
> > > pack revision (one single concat'd file per shard), and then store any
> > > edits in separate files. We'd then "repack" the edits into the pack
> > > files when an admin subsequently runs 'svnadmin pack'.
> >
> > Yes, that's exactly what I don't like :-)
>
> I take that back --- what Hyrum described here is not the same as my
> previous understanding.
>
> How about storing just a single serialized hash per shard, but
> nameprefix the properties?
By the way --- I realize that I haven't yet expressed on an opinion on
the suggestion as described by Hyrum --- I am simply still trying to see
if I can find a race condition in there. (My previous example will fail
if the 'separate files' are stored at a path other than revprops/0/42.)
Re: do away with db/revprops/*/, and a question about 'upgrade'
concurrency (was: Re: Does fsfs revprop packing no longer allow usage of
traditional backup software?)
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Fri, Jul 01, 2011 at 19:29:50 +0300:
> Hyrum K Wright wrote on Fri, Jul 01, 2011 at 11:21:58 -0500:
> > On Fri, Jul 1, 2011 at 8:05 AM, Daniel Shahaf <da...@elego.de> wrote:
> > > Hyrum K Wright wrote on Thu, Jun 30, 2011 at 16:33:16 -0500:
> > >> On Thu, Jun 30, 2011 at 3:27 PM, Peter Samuelson <pe...@p12n.org> wrote:
> > >> >
> > >> > [Ivan Zhakov]
> > >> >> It should be easy to implement editing revprops without using SQLite:
> > >> >> in case someone modify revprop non-packed revprop file is created, in
> > >> >> read operation non-packed revprop file should be considered as more
> > >> >> up-to-date. In next svnadmin pack operation these non-packed files
> > >> >> should be merged back to packed one.
> > >> >
> > >> > +1. This would basically mean there's only _one_ code path for writing
> > >> > revprops, yes? 'svnadmin pack' gets a little more complex, but the
> > >> > rest of libsvn_fs_fs gets simpler.
> > >> >
> > >> > Anyone have time to actually do this? Converting the packed format
> > >> > from sqlite to the same format used for packed revs would be a bonus.
> > >>
> > >> I like this idea, but it would seem to introduce an additional stat()
> > >> call* for every attempt to fetch a revprop, because you'd first have
> > >> to check the "old" location, and then the packed one.
> > >
> > > I don't like the idea of writing revprops outside the DB and moving them
> > > back in. (I think I already said why elsethread?)
> >
> > (I will assume "the DB" means "the SQLite-backed revprop database".)
> >
> > I agree with you, but I don't think that's what the proposal was
> > about. My understanding would be that we'd pack revprops just like we
> > pack revision (one single concat'd file per shard), and then store any
> > edits in separate files. We'd then "repack" the edits into the pack
> > files when an admin subsequently runs 'svnadmin pack'.
>
> Yes, that's exactly what I don't like :-)
I take that back --- what Hyrum described here is not the same as my
previous understanding.
How about storing just a single serialized hash per shard, but
nameprefix the properties?
K 20
svn:fsfs:r1:svn:date
V 22
2011-07-01T18:19:54.259712Z
K 20
svn:fsfs:r0:svn:date
V 27
2011-07-01T18:19:54.259711Z
K
svn:fsfs:r1:svn:author
V 8
danielsh
END
or
K 2
r0
V 47
K 20
svn:date
V 27
2011-07-01T18:19:54.259711Z
K 2
r1
V 76
K 10
svn:author
V 8
danielsh
K 20
svn:date
V 27
2011-07-01T18:19:54.259712Z
END
or
(same as the last suggestion, but store the keys as unserialized
foo-endian uint64 per Peter's suggestion)
This doesn't require a manifest file. A propchange would read the
properties using a variant of svn_hash_read() that only includes keys
related to the given revision, and write the modifications by
* grabbing the FSFS write lock
* dumping the K-V hunk(s) for the affected revision to db/revprops/0.tmp
* concatenating the remainder of db/revprops/0 into 0.tmp,
excluding hunks pertaining to the revision just affected.
ie, move a bit of the heavy lifting to the hash writing/reading code.
Hmm. I'm not sure yet myself that I like this idea. But I'll throw it
out there anyway :)
Re: do away with db/revprops/*/, and a question about 'upgrade'
concurrency (was: Re: Does fsfs revprop packing no longer allow usage of
traditional backup software?)
Posted by Daniel Shahaf <da...@elego.de>.
Hyrum K Wright wrote on Fri, Jul 01, 2011 at 11:21:58 -0500:
> On Fri, Jul 1, 2011 at 8:05 AM, Daniel Shahaf <da...@elego.de> wrote:
> > Hyrum K Wright wrote on Thu, Jun 30, 2011 at 16:33:16 -0500:
> >> On Thu, Jun 30, 2011 at 3:27 PM, Peter Samuelson <pe...@p12n.org> wrote:
> >> >
> >> > [Ivan Zhakov]
> >> >> It should be easy to implement editing revprops without using SQLite:
> >> >> in case someone modify revprop non-packed revprop file is created, in
> >> >> read operation non-packed revprop file should be considered as more
> >> >> up-to-date. In next svnadmin pack operation these non-packed files
> >> >> should be merged back to packed one.
> >> >
> >> > +1. This would basically mean there's only _one_ code path for writing
> >> > revprops, yes? 'svnadmin pack' gets a little more complex, but the
> >> > rest of libsvn_fs_fs gets simpler.
> >> >
> >> > Anyone have time to actually do this? Converting the packed format
> >> > from sqlite to the same format used for packed revs would be a bonus.
> >>
> >> I like this idea, but it would seem to introduce an additional stat()
> >> call* for every attempt to fetch a revprop, because you'd first have
> >> to check the "old" location, and then the packed one.
> >
> > I don't like the idea of writing revprops outside the DB and moving them
> > back in. (I think I already said why elsethread?)
>
> (I will assume "the DB" means "the SQLite-backed revprop database".)
>
> I agree with you, but I don't think that's what the proposal was
> about. My understanding would be that we'd pack revprops just like we
> pack revision (one single concat'd file per shard), and then store any
> edits in separate files. We'd then "repack" the edits into the pack
> files when an admin subsequently runs 'svnadmin pack'.
Yes, that's exactly what I don't like :-)
I already outlined one case where I think the behaviour would be
incorrect. (And even stefan2's suggestion from IRC yesterday doesn't
solve that race condition --- unless we can depend on the OS providing
an atomic unlink_if_timestamp_is_not_X() functionality)
> Aside from the complexity of having to attempt to open the non-packed
> file everywhere before failing through to the common case, I really
> like this solution. (So much so that I may go implement it...)
As I said on #3944, I plan to implement the suggestion of "Either all
revprops are in sharded files, or all revprops are in the DB" (due to
Peter). So, let's agree on /what/ behaviour we want to implement,
before either of us goes and implements anything :-)
>
> -Hyrum
Re: do away with db/revprops/*/, and a question about 'upgrade' concurrency
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <da...@elego.de> writes:
> Unrelated question: how do we handle the case of a client, linked to
> libsvn_fs 1.6, that opens an svn_fs_t object and then, concurrently,
> someone runs 'svnadmin1.7 upgrade'?
>
> The FS object is now invalid (and must be closed), but would the old
> client detect that? For that matter, does our current code error out if
> db/format is bumped under its feet?
If all the clients goes via libsvn_repos then the upgrade will block
until there are no readers and then readers will block until the upgrade
is complete, at which point 1.6 readers will fail because they don't
support the upgraded format.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: do away with db/revprops/*/, and a question about 'upgrade'
concurrency (was: Re: Does fsfs revprop packing no longer allow usage of
traditional backup software?)
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Fri, Jul 1, 2011 at 8:05 AM, Daniel Shahaf <da...@elego.de> wrote:
> Hyrum K Wright wrote on Thu, Jun 30, 2011 at 16:33:16 -0500:
>> On Thu, Jun 30, 2011 at 3:27 PM, Peter Samuelson <pe...@p12n.org> wrote:
>> >
>> > [Ivan Zhakov]
>> >> It should be easy to implement editing revprops without using SQLite:
>> >> in case someone modify revprop non-packed revprop file is created, in
>> >> read operation non-packed revprop file should be considered as more
>> >> up-to-date. In next svnadmin pack operation these non-packed files
>> >> should be merged back to packed one.
>> >
>> > +1. This would basically mean there's only _one_ code path for writing
>> > revprops, yes? 'svnadmin pack' gets a little more complex, but the
>> > rest of libsvn_fs_fs gets simpler.
>> >
>> > Anyone have time to actually do this? Converting the packed format
>> > from sqlite to the same format used for packed revs would be a bonus.
>>
>> I like this idea, but it would seem to introduce an additional stat()
>> call* for every attempt to fetch a revprop, because you'd first have
>> to check the "old" location, and then the packed one.
>
> I don't like the idea of writing revprops outside the DB and moving them
> back in. (I think I already said why elsethread?)
(I will assume "the DB" means "the SQLite-backed revprop database".)
I agree with you, but I don't think that's what the proposal was
about. My understanding would be that we'd pack revprops just like we
pack revision (one single concat'd file per shard), and then store any
edits in separate files. We'd then "repack" the edits into the pack
files when an admin subsequently runs 'svnadmin pack'.
Aside from the complexity of having to attempt to open the non-packed
file everywhere before failing through to the common case, I really
like this solution. (So much so that I may go implement it...)
-Hyrum