Hi,
I've noticed that svnadmin recover and hotcopy commands are broken for old
repositories around trunk@1560210. The problem can be reproduced by creating
a new --compatible-version=1.3 / 1.2 / 1.1 FSFS repository, doing something
simple with it (say, running svn mkdir or svn checkout / add file / commit) and
attempting to recover or hotcopy it:
[[[
(svnadmin-trunk recover REPOS)
svnadmin: E200002: Serialized hash missing terminator
(svnadmin-trunk hotcopy REPOS REPOS_BACKUP)
svnadmin: E160006: No such revision 1
]]]
Alternatively, this can be reproduced by running svnadmin_tests.py with, for
example, --server-minor-version=3. This problem exists in both 1.8.x and
trunk. In 1.8.x, however, the errors for both hotcopy and recover are visually
identical:
[[[
(svnadmin-1.8.x recover REPOS)
svnadmin: E200002: Serialized hash missing terminator
(svnadmin-1.8.x hotcopy REPOS REPOS_BACKUP)
svnadmin: E200002: Serialized hash missing terminator
]]]
Overall, this looks like a regression since 1.7.x, where hotcopy works just
fine. The 1.7.x recover also fails for these repositories with an
"svnadmin: E000002: Can't open file 'X/db/revs/2': No such file or directory"
error, but this is a well-known issue 4213 [1], which should be fixed by
r1367683 [2].
Being unable to recover and hotcopy old repositories with the most recent
svnadmin version seems like an important issue, so I think it's worth a couple
of tests. [See the attached patch #1]
I did a quick investigation and spotted a couple of odd moments in the
related code:
1) First of all, there is a reason for the different error messages in trunk
and in the 1.8.x branch.
This is a regression introduced in r1503742 [3], because now calling the
svn_fs_fs__find_max_ids() routine during hotcopy creates a cyclic
dependency. Hotcopy needs to update the db/current file, so it uses this
routine to get the NEXT_NODE_ID / NEXT_COPY_ID for old repositories.
However, this currently behaves the following way:
hotcopy_update_current()
...
(wants to update the db/current file, but doesn't know the next ids)
svn_fs_fs__find_max_ids()
svn_fs_fs__rev_get_root()
svn_fs_fs__ensure_revision_exists()
...
(reads the most recent db/current file in order to check the presence
of this revision. it is reported missing, because we're right in the
middle of updating the db/current file and cannot do that yet,
because we still do not know the next ids)
So, basically, we're calling a routine that assumes the db/current file is
up-to-date in order to get the information on how to update the outdated
db/current file. This ends with an "E160006: No such revision 1" error.
2) The reason for the "hash missing terminator" error is a bit trickier.
The NEXT_NODE_ID / NEXT_COPY_ID recovery code recursively crawls
through the dir representations in the db, however, it assumes that the
EXPANDED_SIZE in the rep description always is non-zero. According to
the docstring in subversion/libsvn_fs_fs/fs.h, this assumption is incorrect:
[[[
/* The size of the representation in bytes as seen in the revision
file. */
svn_filesize_t size;
/* The size of the fulltext of the representation. If this is 0,
* the fulltext size is equal to representation size in the rev file, */
svn_filesize_t expanded_size;
]]]
I did a small experiment with different svn (1.7 / 1.8 / trunk) versions
writing dir reps to repositories with different --compatible-versions...
[[[
# text: REVISION OFFSET SIZE EXPANDED_SIZE
svn 1.7, --compatible-version=1.3 repository (db/revs/1)
text: 1 57 28 28
svn 1.8, --compatible-version=1.3 repository (db/revs/1)
text: 1 57 28 0
]]]
...and it looks like svn 1.7.x did have the EXPANDED_SIZE equal to SIZE, but
this behavior changed with 1.8.x. In 1.8.x, EXPANDED_SIZE is set to 0 for
these representations, so, attemping to read them ends with an
"E200002: Serialized hash missing terminator" error.
See the attached 'rep-description-dumps' file.
NOTE: This code is a part of the recover_find_max_ids() routine and, worth
mentioning, has some history. There is r1404675 [4], which added the "pick
the right one from EXPANDED_SIZE and SIZE" logic, however, it was reverted
in r1404708 [5]. Unfortunately, I do not yet understand the reasoning
behind this revert, so, I guess, I'll just have to continue investigating.
3) There is an "uniquifier" concept for the representations that makes the rep
caching possible. Each uniquifier requires a new NODE_ID, so, for example,
a transaction which requires 5 new NODE_IDs ends up with taking 10 of them.
This assumption is not shared by the recovery procedure, which recovers the
NEXT_NODE_ID / NEXT_COPY_ID for old repositories by finding the maximum
NODE_ID / COPY_ID and bumping them by one.
For example, for the old format Greek tree repository with db/current
containing "1 14 1" the recovery procedure will replace the db/current
contents with "1 13 1". '13' in this example is the recovered NEW_NODE_ID
(but it also is the NODE_ID reserved for the uniquifier of the last "real"
NODE_ID). As a consequence, hotcopying this repository (which utilizes the
recover_find_max_ids() routine) will end up with *mismatching db/current*
files in the hotcopy source and destination.
It looks like making these uniquifiers for old repositories is unnecessary
in the first place (they are only required for rep caching, which is only
available for the newer >= SVN_FS_FS__MIN_REP_SHARING_FORMAT
repositories).
See set_uniquifier() in subversion/libsvn_fs_fs/transaction.c
4) The hotcopy_update_current() does something quite odd for old repositories.
It grabs the MAX_NODE_ID / MAX_COPY_ID from svn_fs_fs__find_max_ids(),
claims them to be the NEXT_NODE_ID / NEXT_COPY_ID and writes them into the
db/current file. The maximum values found by the svn_fs_fs__find_max_ids()
routine should be bumped by one (the same way it is done in recover_body(),
subversion/libsvn_fs_fs/recovery.c).
Not doing so will result in the hotcopy destination having incorrect
NEXT_NODE_ID and NEXT_COPY_ID in the db/current file (pointing to
already taken ids).
If this might help to understand what is going on, I've made a quick-and-dirty
patch for 1.8.x branch, which explicitly points the problems and rushes toward
the green test suite. Doing this for 1.8.x allows to postpone the problem 1)
and focus on 2-4), because, as far as I suspect, fixing 1) might get a bit
messy. [See the attached patch #2]
An appropriate fix for this issue undoubtedly requires a lot more work.
I think I might be able to come up with a patch for the trunk in the nearby
future (in case someone does not fix it earlier).
[1] http://subversion.tigris.org/issues/show_bug.cgi?id=4213
[2] http://svn.apache.org/viewvc?view=revision&revision=r1367683
[3] http://svn.apache.org/viewvc?view=revision&revision=r1503742
[4] http://svn.apache.org/viewvc?view=revision&revision=r1404675
[5] http://svn.apache.org/viewvc?view=revision&revision=r1404708
Thanks and regards,
Evgeny Kotkov
Index: subversion/tests/cmdline/svnadmin_tests.py
===================================================================
--- subversion/tests/cmdline/svnadmin_tests.py (revision 1560210)
+++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
@@ -1941,8 +1941,8 @@ def mergeinfo_race(sbox):
@Issue(4213)
@Skip(svntest.main.is_fs_type_fsx)
-def recover_old(sbox):
- "recover --pre-1.4-compatible"
+def recover_old_empty(sbox):
+ "recover empty --compatible-version=1.3"
svntest.main.safe_rmtree(sbox.repo_dir, 1)
svntest.main.create_repos(sbox.repo_dir, minor_version=3)
svntest.actions.run_and_verify_svnadmin(None, None, [],
@@ -2213,6 +2213,37 @@ def verify_denormalized_names(sbox):
output, errput, exp_out, exp_err)
+@XFail()
+@SkipUnless(svntest.main.is_fs_type_fsfs)
+def fsfs_recover_old_non_empty(sbox):
+ "fsfs recover non-empty --compatible-version=1.3"
+
+ # Around trunk@1560210, 'svnadmin recover' wrongly errored out
+ # for the --compatible-version=1.3 Greek tree repository:
+ # svnadmin: E200002: Serialized hash missing terminator
+
+ sbox.build(create_wc=False, minor_version=3)
+ svntest.actions.run_and_verify_svnadmin(None, None, [], "recover",
+ sbox.repo_dir)
+
+
+@XFail()
+@SkipUnless(svntest.main.is_fs_type_fsfs)
+def fsfs_hotcopy_old_non_empty(sbox):
+ "fsfs hotcopy non-empty --compatible-version=1.3"
+
+ # Around trunk@1560210, 'svnadmin hotcopy' wrongly errored out
+ # for the --compatible-version=1.3 Greek tree repository:
+ # svnadmin: E160006: No such revision 1
+
+ sbox.build(create_wc=False, minor_version=3)
+ backup_dir, backup_url = sbox.add_repo_path('backup')
+ svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+ sbox.repo_dir, backup_dir)
+
+ check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+
+
########################################################################
# Run the tests
@@ -2248,10 +2279,12 @@ test_list = [ None,
hotcopy_incremental_packed,
locking,
mergeinfo_race,
- recover_old,
+ recover_old_empty,
verify_keep_going,
verify_invalid_path_changes,
verify_denormalized_names,
+ fsfs_recover_old_non_empty,
+ fsfs_hotcopy_old_non_empty,
]
if __name__ == '__main__':
Add two tests for 'svnadmin recover/hotcopy' failing with non-empty old FSFS
repositories (--compatible-version=1.3 / 1.2 / 1.1).
* subversion/tests/cmdline/svnadmin_tests.py
(recover_old): Rename this...
(recover_old_empty): ...to this. This helps to distinguish between old and
new tests for similiar cases. Also, tweak the docstring for consistency.
(fsfs_recover_old_non_empty): New test for the erroneous 'svnadmin recover'
behavior. Marked as XFail.
(fsfs_hotcopy_old_non_empty): New test for the erroneous 'svnadmin hotcopy'
behavior. Marked as XFail.
(test_list): Add references to new tests and update reference to the
renamed test.
Patch by: Evgeny Kotkov <evgeny.kotkov{_AT_}visualsvn.com>
# Format for the rep description is "text: REVISION OFFSET SIZE EXPANDED_SIZE",
# see read_rep_offsets_body() and svn_fs_fs__parse_representation().
svn 1.7, --compatible-version=1.1 repository (db/revs/1)
text: 1 57 28 28
svn 1.7, --compatible-version=1.2 repository (db/revs/1)
text: 1 57 28 28
svn 1.7, --compatible-version=1.3 repository (db/revs/1)
text: 1 57 28 28
svn 1.7, --compatible-version=1.4 repository (db/revs/1)
text: 1 57 28 28
svn 1.7, --compatible-version=1.5 repository (db/revs/0/1)
text: 1 59 30 30
svn 1.7, --compatible-version=1.6 repository (db/revs/0/1)
text: 1 59 30 30
svn 1.7, --compatible-version=1.7 repository (db/revs/0/1)
text: 1 59 30 30
svn 1.8, --compatible-version=1.1 repository (db/revs/1)
text: 1 57 28 0
svn 1.8, --compatible-version=1.2 repository (db/revs/1)
text: 1 57 28 0
svn 1.8, --compatible-version=1.3 repository (db/revs/1)
text: 1 57 28 0
svn 1.8, --compatible-version=1.4 repository (db/revs/1)
text: 1 57 28 0
svn 1.8, --compatible-version=1.5 repository (db/revs/0/1)
text: 1 59 30 0
svn 1.8, --compatible-version=1.6 repository (db/revs/0/1)
text: 1 59 30 0
svn 1.8, --compatible-version=1.7 repository (db/revs/0/1)
text: 1 59 30 0
svn 1.8, --compatible-version=1.8 repository (db/revs/0/1)
text: 1 59 30 0
svn 1.9, --compatible-version=1.1 repository (db/revs/1)
text: 1 57 28 0
svn 1.9, --compatible-version=1.2 repository (db/revs/1)
text: 1 57 28 0
svn 1.9, --compatible-version=1.3 repository (db/revs/1)
text: 1 57 28 0
svn 1.9, --compatible-version=1.4 repository (db/revs/1)
text: 1 57 28 0
svn 1.9, --compatible-version=1.5 repository (db/revs/0/1)
text: 1 59 30 0
svn 1.9, --compatible-version=1.6 repository (db/revs/0/1)
text: 1 59 42 30
svn 1.9, --compatible-version=1.7 repository (db/revs/0/1)
text: 1 59 42 30
svn 1.9, --compatible-version=1.8 repository (db/revs/0/1)
text: 1 59 42 30
svn 1.9, --compatible-version=1.9 repository (db/revs/0/1)
text: 1 4 42 30
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1560210)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -6944,7 +6944,6 @@ svn_fs_fs__set_entry(svn_fs_t *fs,
if (!rep || !rep->txn_id)
{
- const char *unique_suffix;
apr_hash_t *entries;
/* Before we can modify the directory, we need to dump its old
@@ -6964,8 +6963,16 @@ svn_fs_fs__set_entry(svn_fs_t *fs,
rep = apr_pcalloc(pool, sizeof(*rep));
rep->revision = SVN_INVALID_REVNUM;
rep->txn_id = txn_id;
- SVN_ERR(get_new_txn_node_id(&unique_suffix, fs, txn_id, pool));
- rep->uniquifier = apr_psprintf(pool, "%s/%s", txn_id, unique_suffix);
+
+ if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
+ {
+ const char *unique_suffix;
+
+ SVN_ERR(get_new_txn_node_id(&unique_suffix, fs, txn_id, pool));
+ rep->uniquifier = apr_psprintf(pool, "%s/%s", txn_id,
+ unique_suffix);
+ }
+
parent_noderev->data_rep = rep;
SVN_ERR(svn_fs_fs__put_node_revision(fs, parent_noderev->id,
parent_noderev, FALSE, pool));
@@ -7545,10 +7552,10 @@ static svn_error_t *
rep_write_contents_close(void *baton)
{
struct rep_write_baton *b = baton;
- const char *unique_suffix;
representation_t *rep;
representation_t *old_rep;
apr_off_t offset;
+ fs_fs_data_t *ffd = b->fs->fsap_data;
rep = apr_pcalloc(b->parent_pool, sizeof(*rep));
rep->offset = b->rep_offset;
@@ -7565,9 +7572,17 @@ rep_write_contents_close(void *baton)
/* Fill in the rest of the representation field. */
rep->expanded_size = b->rep_size;
rep->txn_id = svn_fs_fs__id_txn_id(b->noderev->id);
- SVN_ERR(get_new_txn_node_id(&unique_suffix, b->fs, rep->txn_id, b->pool));
- rep->uniquifier = apr_psprintf(b->parent_pool, "%s/%s", rep->txn_id,
- unique_suffix);
+
+ if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
+ {
+ const char *unique_suffix;
+
+ SVN_ERR(get_new_txn_node_id(&unique_suffix, b->fs,
+ rep->txn_id, b->pool));
+ rep->uniquifier = apr_psprintf(b->parent_pool, "%s/%s", rep->txn_id,
+ unique_suffix);
+ }
+
rep->revision = SVN_INVALID_REVNUM;
/* Finalize the checksum. */
@@ -8875,8 +8890,17 @@ svn_fs_fs__create(svn_fs_t *fs,
SVN_ERR(write_revision_zero(fs));
- SVN_ERR(write_config(fs, pool));
+ /* Grab the r1547454 from trunk. This is completely unrelated to the fix
+ but makes the check_hotcopy_fsfs helper work for the added 'hotcopy
+ non-empty --compatible-version=1.3' cmdline test:
+ Create the fsfs.conf file if supported. Older server versions would
+ simply ignore the file but that might result in a different behavior
+ than with the later releases. Also, hotcopy would ignore, i.e. not
+ copy, a fsfs.conf with old formats. */
+ if (ffd->format >= SVN_FS_FS__MIN_CONFIG_FILE)
+ SVN_ERR(write_config(fs, pool));
+
SVN_ERR(read_config(ffd, fs->path, pool));
/* Create the min unpacked rev file. */
@@ -9063,7 +9087,8 @@ recover_find_max_ids(svn_fs_t *fs, svn_revnum_t re
stored in the representation. */
baton.file = rev_file;
baton.pool = pool;
- baton.remaining = data_rep->expanded_size;
+ baton.remaining = data_rep->expanded_size ?
+ data_rep->expanded_size : data_rep->size;
stream = svn_stream_create(&baton, pool);
svn_stream_set_read(stream, read_handler_recover);
@@ -10893,8 +10918,10 @@ hotcopy_update_current(svn_revnum_t *dst_youngest,
svn_revnum_t new_youngest,
apr_pool_t *scratch_pool)
{
- char next_node_id[MAX_KEY_SIZE] = "0";
- char next_copy_id[MAX_KEY_SIZE] = "0";
+ char *next_node_id = NULL;
+ char *next_copy_id = NULL;
+ char next_node_id_buf[MAX_KEY_SIZE];
+ char next_copy_id_buf[MAX_KEY_SIZE];
fs_fs_data_t *dst_ffd = dst_fs->fsap_data;
if (*dst_youngest >= new_youngest)
@@ -10903,8 +10930,11 @@ hotcopy_update_current(svn_revnum_t *dst_youngest,
/* If necessary, get new current next_node and next_copy IDs. */
if (dst_ffd->format < SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT)
{
+ char max_node_id[MAX_KEY_SIZE] = "0";
+ char max_copy_id[MAX_KEY_SIZE] = "0";
apr_off_t root_offset;
apr_file_t *rev_file;
+ apr_size_t len;
if (dst_ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT)
SVN_ERR(update_min_unpacked_rev(dst_fs, scratch_pool));
@@ -10914,9 +10944,18 @@ hotcopy_update_current(svn_revnum_t *dst_youngest,
SVN_ERR(get_root_changes_offset(&root_offset, NULL, rev_file,
dst_fs, new_youngest, scratch_pool));
SVN_ERR(recover_find_max_ids(dst_fs, new_youngest, rev_file,
- root_offset, next_node_id, next_copy_id,
+ root_offset, max_node_id, max_copy_id,
scratch_pool));
SVN_ERR(svn_io_file_close(rev_file, scratch_pool));
+
+ /* Now that we have the maximum node-id and copy-id, we can bump them
+ to get the next of each. */
+ len = strlen(max_node_id);
+ svn_fs_fs__next_key(max_node_id, &len, next_node_id_buf);
+ next_node_id = next_node_id_buf;
+ len = strlen(max_copy_id);
+ svn_fs_fs__next_key(max_copy_id, &len, next_copy_id_buf);
+ next_copy_id = next_copy_id_buf;
}
/* Update 'current'. */
Index: subversion/tests/cmdline/svnadmin_tests.py
===================================================================
--- subversion/tests/cmdline/svnadmin_tests.py (revision 1560210)
+++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
@@ -1824,6 +1824,27 @@ def recover_old(sbox):
svntest.main.run_svnadmin("recover", sbox.repo_dir)
+@SkipUnless(svntest.main.is_fs_type_fsfs)
+def fsfs_recover_old_non_empty(sbox):
+ "fsfs recover non-empty --compatible-version=1.3"
+
+ sbox.build(create_wc=False, minor_version=3)
+ svntest.actions.run_and_verify_svnadmin(None, None, [], "recover",
+ sbox.repo_dir)
+
+
+@SkipUnless(svntest.main.is_fs_type_fsfs)
+def fsfs_hotcopy_old_non_empty(sbox):
+ "fsfs hotcopy non-empty --compatible-version=1.3"
+
+ sbox.build(create_wc=False, minor_version=3)
+ backup_dir, backup_url = sbox.add_repo_path('backup')
+ svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+ sbox.repo_dir, backup_dir)
+
+ check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+
+
########################################################################
# Run the tests
@@ -1860,6 +1881,8 @@ test_list = [ None,
locking,
mergeinfo_race,
recover_old,
+ fsfs_recover_old_non_empty,
+ fsfs_hotcopy_old_non_empty
]
if __name__ == '__main__':
