You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2014/06/16 15:53:16 UTC
enable packing by default
Stefan2 pointed out that format 7 is less efficient if packing is disabled.
So to fully benefit from format 7 in the default configuration, users must
currently run 'svnadmin pack' or edit fsfs.conf to enable packing after commit.
Since format 7 adds locking support to pack, so it should be safe to trigger
packing at any time.
It looks like it makes sense to enable packing after commit by default
for format 7 repositories. Any objections?
[[[
Enable packing by default in format 7 FSFS repositories.
* subversion/libsvn_fs_fs/fs_fs.c
(read_config): If the file format supports the pack lock, default to
pack-after-commit.
]]]
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1602863)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -769,7 +769,8 @@ read_config(fs_fs_data_t *ffd,
SVN_ERR(svn_config_get_bool(config, &ffd->pack_after_commit,
CONFIG_SECTION_DEBUG,
CONFIG_OPTION_PACK_AFTER_COMMIT,
- FALSE));
+ ffd->format < SVN_FS_FS__MIN_PACK_LOCK_FORMAT
+ ? FALSE : TRUE));
}
else
{
Re: enable packing by default
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Jun 16, 2014 at 10:01 PM, Philip Martin <ph...@wandisco.com>
wrote:
> Stefan Sperling <st...@elego.de> writes:
>
> > On Mon, Jun 16, 2014 at 03:53:16PM +0200, Stefan Sperling wrote:
> >> Stefan2 pointed out that format 7 is less efficient if packing is
> disabled.
> >> So to fully benefit from format 7 in the default configuration, users
> must
> >> currently run 'svnadmin pack' or edit fsfs.conf to enable packing after
> commit.
> >> Since format 7 adds locking support to pack, so it should be safe to
> trigger
> >> packing at any time.
> >>
> >> It looks like it makes sense to enable packing after commit by default
> >> for format 7 repositories. Any objections?
> >
> > And here's a more complete patch, fixing test fallout.
> >
> > [[[
> > Enable packing by default in format 7 FSFS repositories.
> >
> > * subversion/libsvn_fs_fs/fs.h
> > (CONFIG_SECTION_DEBUG): Remove.
> > (CONFIG_SECTION_PACKED_REVS): New configuration section
> "packed-revisions",
> > which contains options controlling revision packing behaviour.
> (Perhaps this
> > should be called "packing" and merged with the "packed-revprops"
> section in
> > a backwards-compatible way.)
> >
> > * subversion/libsvn_fs_fs/fs_fs.c
> > (read_config): If the file format supports the pack lock, default to
> > pack-after-commit.
>
> One thing to consider is the behaviour of a large, unpacked, repository
> when upgraded from format 6 to format 7: if we pack by default the first
> commit after the upgrade will pack the whole repository which could take
> a long time and the committing client may well timeout. The admin could
> avoid this by running pack manually after the upgrade, or should upgrade
> invoke pack automatically (perhaps controlled by the same config flag)?
>
I think, upgrade should automatically pack by default -
with an option to override that via fsfs.conf. We gain
nothing by deferring the pack until the next full shard.
Packing itself is save in the sense that it can be (non-
gracefully) interrupted and restarted at any time with
no long-term ill effects. Partially written pack files will
simply linger around until they get cleaned up by the
next pack run.
-- Stefan^2.
Re: enable packing by default
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
> On Mon, Jun 16, 2014 at 03:53:16PM +0200, Stefan Sperling wrote:
>> Stefan2 pointed out that format 7 is less efficient if packing is disabled.
>> So to fully benefit from format 7 in the default configuration, users must
>> currently run 'svnadmin pack' or edit fsfs.conf to enable packing after commit.
>> Since format 7 adds locking support to pack, so it should be safe to trigger
>> packing at any time.
>>
>> It looks like it makes sense to enable packing after commit by default
>> for format 7 repositories. Any objections?
>
> And here's a more complete patch, fixing test fallout.
>
> [[[
> Enable packing by default in format 7 FSFS repositories.
>
> * subversion/libsvn_fs_fs/fs.h
> (CONFIG_SECTION_DEBUG): Remove.
> (CONFIG_SECTION_PACKED_REVS): New configuration section "packed-revisions",
> which contains options controlling revision packing behaviour. (Perhaps this
> should be called "packing" and merged with the "packed-revprops" section in
> a backwards-compatible way.)
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (read_config): If the file format supports the pack lock, default to
> pack-after-commit.
One thing to consider is the behaviour of a large, unpacked, repository
when upgraded from format 6 to format 7: if we pack by default the first
commit after the upgrade will pack the whole repository which could take
a long time and the committing client may well timeout. The admin could
avoid this by running pack manually after the upgrade, or should upgrade
invoke pack automatically (perhaps controlled by the same config flag)?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: enable packing by default
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Jun 16, 2014 at 03:53:16PM +0200, Stefan Sperling wrote:
> Stefan2 pointed out that format 7 is less efficient if packing is disabled.
> So to fully benefit from format 7 in the default configuration, users must
> currently run 'svnadmin pack' or edit fsfs.conf to enable packing after commit.
> Since format 7 adds locking support to pack, so it should be safe to trigger
> packing at any time.
>
> It looks like it makes sense to enable packing after commit by default
> for format 7 repositories. Any objections?
And here's a more complete patch, fixing test fallout.
[[[
Enable packing by default in format 7 FSFS repositories.
* subversion/libsvn_fs_fs/fs.h
(CONFIG_SECTION_DEBUG): Remove.
(CONFIG_SECTION_PACKED_REVS): New configuration section "packed-revisions",
which contains options controlling revision packing behaviour. (Perhaps this
should be called "packing" and merged with the "packed-revprops" section in
a backwards-compatible way.)
* subversion/libsvn_fs_fs/fs_fs.c
(read_config): If the file format supports the pack lock, default to
pack-after-commit.
* subversion/tests/cmdline/svnadmin_tests.py
(disable_pack_after_commit): New helper function.
(hotcopy_incremental_packed, verify_packed): Disable pack-after-commit
for these tests to keep them passing.
]]]
Index: subversion/libsvn_fs_fs/fs.h
===================================================================
--- subversion/libsvn_fs_fs/fs.h (revision 1602871)
+++ subversion/libsvn_fs_fs/fs.h (working copy)
@@ -117,7 +117,7 @@ extern "C" {
#define CONFIG_OPTION_BLOCK_SIZE "block-size"
#define CONFIG_OPTION_L2P_PAGE_SIZE "l2p-page-size"
#define CONFIG_OPTION_P2L_PAGE_SIZE "p2l-page-size"
-#define CONFIG_SECTION_DEBUG "debug"
+#define CONFIG_SECTION_PACKED_REVS "packed-revisions"
#define CONFIG_OPTION_PACK_AFTER_COMMIT "pack-after-commit"
/* The format number of this filesystem.
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1602871)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -767,9 +767,10 @@ read_config(fs_fs_data_t *ffd,
if (ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT)
{
SVN_ERR(svn_config_get_bool(config, &ffd->pack_after_commit,
- CONFIG_SECTION_DEBUG,
+ CONFIG_SECTION_PACKED_REVS,
CONFIG_OPTION_PACK_AFTER_COMMIT,
- FALSE));
+ ffd->format < SVN_FS_FS__MIN_PACK_LOCK_FORMAT
+ ? FALSE : TRUE));
}
else
{
@@ -987,6 +988,11 @@ write_config(svn_fs_t *fs,
"### Must be a power of 2." NL
"### p2l-page-size is 1024 kBytes by default." NL
"# " CONFIG_OPTION_P2L_PAGE_SIZE " = 1024" NL
+"" NL
+"[" CONFIG_SECTION_PACKED_REVS "]" NL
+"### This option controls whether to pack revisions after commit." NL
+"### The default is 'yes' for FSFS format 7 repositories, otherwise 'no'." NL
+"# " CONFIG_OPTION_PACK_AFTER_COMMIT " = no" NL
;
#undef NL
return svn_io_file_create(svn_dirent_join(fs->path, PATH_CONFIG, pool),
Index: subversion/tests/cmdline/svnadmin_tests.py
===================================================================
--- subversion/tests/cmdline/svnadmin_tests.py (revision 1602871)
+++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
@@ -41,6 +41,7 @@ from svntest.verify import SVNExpectedStdout, SVNE
from svntest.verify import SVNUnexpectedStderr
from svntest.verify import UnorderedOutput
from svntest.main import SVN_PROP_MERGEINFO
+from svntest.main import get_fsfs_conf_file_path
# (abbreviation)
Skip = svntest.testcase.Skip_deco
@@ -342,6 +343,21 @@ def set_changed_path_list(sbox, revision, changes)
fp.truncate()
fp.close()
+def disable_pack_after_commit(repo_dir):
+ """ Disable pack-after-commit option in FSFS repositories. """
+ fsfsconf = open(get_fsfs_conf_file_path(repo_dir), 'r')
+ tweaked_config = []
+ for line in fsfsconf.readlines():
+ if line.startswith('# pack-after-commit'):
+ tweaked_config.append('pack-after-commit = no\n')
+ else:
+ tweaked_config.append(line)
+ fsfsconf.close()
+ fsfsconf = open(get_fsfs_conf_file_path(repo_dir), 'w')
+ for line in tweaked_config:
+ fsfsconf.write(line)
+ fsfsconf.close()
+
######################################################################
# Tests
@@ -1795,6 +1811,8 @@ def hotcopy_incremental_packed(sbox):
format_file.write("6\nlayout sharded 2\n")
format_file.close()
+ disable_pack_after_commit(sbox.repo_dir)
+
# Pack revisions 0 and 1.
svntest.actions.run_and_verify_svnadmin(
None, ['Packing revisions in shard 0...done.\n'], [], "pack",
@@ -2352,6 +2370,8 @@ def verify_packed(sbox):
"verify packed with small shards"
sbox.build()
+ disable_pack_after_commit(sbox.repo_dir)
+
# Configure two files per shard to trigger packing.
if svntest.main.is_fs_type_fsx():
format = "1\nlayout sharded 2\n"
Re: enable packing by default
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 18, 2014 at 6:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 16 June 2014 15:53, Stefan Sperling <st...@elego.de> wrote:
> > Stefan2 pointed out that format 7 is less efficient if packing is
> disabled.
> > So to fully benefit from format 7 in the default configuration, users
> must
> > currently run 'svnadmin pack' or edit fsfs.conf to enable packing after
> commit.
> > Since format 7 adds locking support to pack, so it should be safe to
> trigger
> > packing at any time.
> >
> > It looks like it makes sense to enable packing after commit by default
> > for format 7 repositories. Any objections?
> >
> I strongly disagree with the proposed patch. With this patch every
> 1000th commit will take long time: because Subversion will have to
> combine 1000 revisions to one file. That's means with 1mb revisions
> Subversion will have to write 1gb to disk for the 1000th commit. This
> cannot be done fast and during this time svn client will hang.
>
Well, with 1M average revision sizes, f7 will not need
packing to be as fast or (much) faster than f6. It's the
<64kB revisions where the extra index file access
becomes noticeable.
FWIW, we can fork off the pack process instead of
letting the client wait (which you also mentioned below).
> We cannot broke one of the current fundamental characteristics of
> Subversion. The time of the commit should only depend on the size of
> the committed data. It shouldn't depend on the cumulative size of the
> previous 1000 commits!
>
That is not strictly true. The commit time is proportional
to the aggregated lengths of every modified node's history.
Until recently, that could mean O(#num_revs * #changes).
> The possible solutions are to pack incrementally after each commit or
> trigger packing to be performed asyncronously (i.e. in background),
> but I'm not sure if it's possible to implement easily.
>
We can simply fork() off as we do with post-commit scripts.
Even if it should fail for some reason, the next successful
pack run will catch up to HEAD.
-- Stefan^2.
Re: enable packing by default
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 16 June 2014 15:53, Stefan Sperling <st...@elego.de> wrote:
> Stefan2 pointed out that format 7 is less efficient if packing is disabled.
> So to fully benefit from format 7 in the default configuration, users must
> currently run 'svnadmin pack' or edit fsfs.conf to enable packing after commit.
> Since format 7 adds locking support to pack, so it should be safe to trigger
> packing at any time.
>
> It looks like it makes sense to enable packing after commit by default
> for format 7 repositories. Any objections?
>
I strongly disagree with the proposed patch. With this patch every
1000th commit will take long time: because Subversion will have to
combine 1000 revisions to one file. That's means with 1mb revisions
Subversion will have to write 1gb to disk for the 1000th commit. This
cannot be done fast and during this time svn client will hang.
We cannot broke one of the current fundamental characteristics of
Subversion. The time of the commit should only depend on the size of
the committed data. It shouldn't depend on the cumulative size of the
previous 1000 commits!
The possible solutions are to pack incrementally after each commit or
trigger packing to be performed asyncronously (i.e. in background),
but I'm not sure if it's possible to implement easily.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com