How do we now check that property changes don't conflict?
Before this patch we assumed that the client code would do the verification work. Are property changes now just overwritten?
Or is your patch limited to one round of property changes?
I think changes like this need more documentation than just mentioning that it is a scalability issue that can be fixed 'easily'. There were quite a few design decisions behind this limitation when I looked at it a few years ago.
This might break some of the promises that make the 'incomplete' handling capable of resuming an update. Both the status of the properties and entries of the directory are tied to that.
Bert
-----Original Message-----
From: "stefan2_at_apache.org" <stefan2_at_apache.org>
Sent: 14-1-2014 23:09
To: "commits_at_subversion.apache.org" <commits_at_subversion.apache.org>
Subject: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Author: stefan2
Date: Tue Jan 14 22:09:01 2014
New Revision: 1558224
URL: http://svn.apache.org/r1558224
Log:
For FSFS and FSX, fix a major merge scalability issue described here:
http://svn.haxx.se/dev/archive-2014-01/0057.shtml
The bottom line is that we allow incoming prop changes on directories
that have no addition nor deletion of entries relative to the TXN
base revision. This patch updates existing tests and provided a
specific new one.
* subversion/libsvn_fs_fs/tree.c
(compare_dir_structure): A new utility function checking that all
dirents are still the same, although
possibly at different revisions.
(merge): An dir prop change in a txn should not conflict with
simply node upgrades within that directory.
* subversion/libsvn_fs_x/tree.c
(compare_dir_structure,
merge): Similar implementation as for FSFS with a slight adaptation
due to different directory container types.
* subversion/tests/libsvn_fs/fs-test.c
(unordered_txn_dirprops): Update expected results.
(dir_prop_merge): New test.
(test_compat_version): Register the new test.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/tree.c
subversion/trunk/subversion/libsvn_fs_x/tree.c
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Tue Jan 14 22:09:01 2014
@@ -1654,6 +1654,53 @@ conflict_err(svn_stringbuf_t *conflict_p
_("Conflict at '%s'"), path);
}
+/* Compare the directory representations at nodes LHS and RHS and set
+ * *CHANGED to TRUE, if at least one entry has been added or removed them.
+ * Use POOL for temporary allocations.
+ */
+static svn_error_t *
+compare_dir_structure(svn_boolean_t *changed,
+ dag_node_t *lhs,
+ dag_node_t *rhs,
+ apr_pool_t *pool)
+{
+ apr_array_header_t *lhs_entries;
+ apr_array_header_t *rhs_entries;
+ int i;
+
+ SVN_ERR(svn_fs_fs__dag_dir_entries(&lhs_entries, lhs, pool));
+ SVN_ERR(svn_fs_fs__dag_dir_entries(&rhs_entries, rhs, pool));
+
+ /* different number of entries -> some addition / removal */
+ if (lhs_entries->nelts != rhs_entries->nelts)
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+
+ /* Since directories are sorted by name, we can simply compare their
+ entries one-by-one without binary lookup etc. */
+ for (i = 0; i < lhs_entries->nelts; ++i)
+ {
+ svn_fs_dirent_t *lhs_entry
+ = APR_ARRAY_IDX(lhs_entries, i, svn_fs_dirent_t *);
+ svn_fs_dirent_t *rhs_entry
+ = APR_ARRAY_IDX(rhs_entries, i, svn_fs_dirent_t *);
+
+ if (strcmp(lhs_entry->name, rhs_entry->name)
+ || !svn_fs_fs__id_part_eq(svn_fs_fs__id_node_id(lhs_entry->id),
+ svn_fs_fs__id_node_id(rhs_entry->id))
+ || !svn_fs_fs__id_part_eq(svn_fs_fs__id_copy_id(lhs_entry->id),
+ svn_fs_fs__id_copy_id(rhs_entry->id)))
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+ }
+
+ *changed = FALSE;
+ return SVN_NO_ERROR;
+}
/* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
* and TARGET must be distinct node revisions. TARGET_PATH should
@@ -1828,10 +1875,23 @@ merge(svn_stringbuf_t *conflict_p,
it doesn't do a brute-force comparison on textual contents, so
it won't do that here either. Checking to see if the propkey
atoms are `equal' is enough. */
- if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
- return conflict_err(conflict_p, target_path);
if (! svn_fs_fs__noderev_same_rep_key(src_nr->prop_rep, anc_nr->prop_rep))
return conflict_err(conflict_p, target_path);
+
+ /* The directory entries got changed in the repository but the directory
+ properties did not. */
+ if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
+ {
+ /* There is an incoming prop change for this directory.
+ We will accept it only if the directory changes were mere updates
+ to its entries, i.e. there were no additions or removals.
+ Those could cause update problems to the working copy. */
+ svn_boolean_t changed;
+ SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
+
+ if (changed)
+ return conflict_err(conflict_p, target_path);
+ }
}
/* ### todo: it would be more efficient to simply check for a NULL
Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/tree.c Tue Jan 14 22:09:01 2014
@@ -1630,6 +1630,56 @@ conflict_err(svn_stringbuf_t *conflict_p
_("Conflict at '%s'"), path);
}
+/* Compare the directory representations at nodes LHS and RHS and set
+ * *CHANGED to TRUE, if at least one entry has been added or removed them.
+ * Use POOL for temporary allocations.
+ */
+static svn_error_t *
+compare_dir_structure(svn_boolean_t *changed,
+ dag_node_t *lhs,
+ dag_node_t *rhs,
+ apr_pool_t *pool)
+{
+ apr_hash_t *lhs_entries;
+ apr_hash_t *rhs_entries;
+ apr_hash_index_t *hi;
+
+ SVN_ERR(svn_fs_x__dag_dir_entries(&lhs_entries, lhs, pool));
+ SVN_ERR(svn_fs_x__dag_dir_entries(&rhs_entries, rhs, pool));
+
+ /* different number of entries -> some addition / removal */
+ if (apr_hash_count(lhs_entries) != apr_hash_count(rhs_entries))
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+
+ /* Since the number of dirents is the same, we simply need to do a
+ one-sided comparison. */
+ for (hi = apr_hash_first(pool, lhs_entries); hi; hi = apr_hash_next(hi))
+ {
+ svn_fs_dirent_t *lhs_entry;
+ svn_fs_dirent_t *rhs_entry;
+ const char *name;
+ apr_ssize_t klen;
+
+ apr_hash_this(hi, (const void **)&name, &klen, (void **)&lhs_entry);
+ rhs_entry = apr_hash_get(rhs_entries, name, klen);
+
+ if (!rhs_entry
+ || !svn_fs_x__id_part_eq(svn_fs_x__id_node_id(lhs_entry->id),
+ svn_fs_x__id_node_id(rhs_entry->id))
+ || !svn_fs_x__id_part_eq(svn_fs_x__id_copy_id(lhs_entry->id),
+ svn_fs_x__id_copy_id(rhs_entry->id)))
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+ }
+
+ *changed = FALSE;
+ return SVN_NO_ERROR;
+}
/* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
* and TARGET must be distinct node revisions. TARGET_PATH should
@@ -1803,10 +1853,23 @@ merge(svn_stringbuf_t *conflict_p,
it doesn't do a brute-force comparison on textual contents, so
it won't do that here either. Checking to see if the propkey
atoms are `equal' is enough. */
- if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
- return conflict_err(conflict_p, target_path);
if (! svn_fs_x__noderev_same_rep_key(src_nr->prop_rep, anc_nr->prop_rep))
return conflict_err(conflict_p, target_path);
+
+ /* The directory entries got changed in the repository but the directory
+ properties did not. */
+ if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
+ {
+ /* There is an incoming prop change for this directory.
+ We will accept it only if the directory changes were mere updates
+ to its entries, i.e. there were no additions or removals.
+ Those could cause update problems to the working copy. */
+ svn_boolean_t changed;
+ SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
+
+ if (changed)
+ return conflict_err(conflict_p, target_path);
+ }
}
/* ### todo: it would be more efficient to simply check for a NULL
Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1558224&r1=1558223&r2=1558224&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Jan 14 22:09:01 2014
@@ -4582,6 +4582,7 @@ unordered_txn_dirprops(const svn_test_op
svn_fs_root_t *txn_root, *txn_root2;
svn_string_t pval;
svn_revnum_t new_rev, not_rev;
+ svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
/* This is a regression test for issue #2751. */
@@ -4638,10 +4639,21 @@ unordered_txn_dirprops(const svn_test_op
/* Commit the first one first. */
SVN_ERR(test_commit_txn(&new_rev, txn, NULL, pool));
- /* Then commit the second -- but expect an conflict because the
- directory wasn't up-to-date, which is required for propchanges. */
- SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
- SVN_ERR(svn_fs_abort_txn(txn2, pool));
+ /* Some backends are clever then others. */
+ if (is_bdb)
+ {
+ /* Then commit the second -- but expect an conflict because the
+ directory wasn't up-to-date, which is required for propchanges. */
+ SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
+ SVN_ERR(svn_fs_abort_txn(txn2, pool));
+ }
+ else
+ {
+ /* Then commit the second -- there will be no conflict despite the
+ directory being out-of-data because the properties as well as the
+ directory structure (list of nodes) was up-to-date. */
+ SVN_ERR(test_commit_txn(¬_rev, txn2, NULL, pool));
+ }
return SVN_NO_ERROR;
}
@@ -5222,6 +5234,80 @@ test_compat_version(const svn_test_opts_
return SVN_NO_ERROR;
}
+static svn_error_t *
+dir_prop_merge(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_revnum_t head_rev;
+ svn_fs_root_t *root;
+ svn_fs_txn_t *txn, *mid_txn, *top_txn, *sub_txn, *c_txn;
+ svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
+
+ /* Create test repository. */
+ SVN_ERR(svn_test__create_fs(&fs, "test-dir_prop-merge", opts, pool));
+
+ SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, txn, pool));
+
+ /* Create and verify the greek tree. */
+ SVN_ERR(svn_test__create_greek_tree(root, pool));
+ SVN_ERR(test_commit_txn(&head_rev, txn, NULL, pool));
+
+ /* Start concurrent transactions */
+
+ /* 1st: modify a mid-level directory */
+ SVN_ERR(svn_fs_begin_txn2(&mid_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, mid_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
+ svn_string_create("val1", pool), pool));
+ svn_fs_close_root(root);
+
+ /* 2st: modify a top-level directory */
+ SVN_ERR(svn_fs_begin_txn2(&top_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, top_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A", "test-prop",
+ svn_string_create("val2", pool), pool));
+ svn_fs_close_root(root);
+
+ SVN_ERR(svn_fs_begin_txn2(&sub_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, sub_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A/D/G", "test-prop",
+ svn_string_create("val3", pool), pool));
+ svn_fs_close_root(root);
+
+ /* 3rd: modify a conflicting change to the mid-level directory */
+ SVN_ERR(svn_fs_begin_txn2(&c_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, c_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
+ svn_string_create("valX", pool), pool));
+ svn_fs_close_root(root);
+
+ /* Prop changes to the same node should conflict */
+ SVN_ERR(test_commit_txn(&head_rev, mid_txn, NULL, pool));
+ SVN_ERR(test_commit_txn(&head_rev, c_txn, "/A/D", pool));
+ SVN_ERR(svn_fs_abort_txn(c_txn, pool));
+
+ /* Changes in a sub-tree should not conflict with prop changes to some
+ parent directory but some backends are clever then others. */
+ if (is_bdb)
+ {
+ SVN_ERR(test_commit_txn(&head_rev, top_txn, "/A", pool));
+ SVN_ERR(svn_fs_abort_txn(top_txn, pool));
+ }
+ else
+ {
+ SVN_ERR(test_commit_txn(&head_rev, top_txn, NULL, pool));
+ }
+
+ /* The inverted case is not that trivial to handle. Hence, conflict.
+ Depending on the checking order, the reported conflict path differs. */
+ SVN_ERR(test_commit_txn(&head_rev, sub_txn, is_bdb ? "/A/D" : "/A", pool));
+ SVN_ERR(svn_fs_abort_txn(sub_txn, pool));
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
@@ -5315,5 +5401,7 @@ struct svn_test_descriptor_t test_funcs[
"commit timestamp"),
SVN_TEST_OPTS_PASS(test_compat_version,
"test svn_fs__compatible_version"),
+ SVN_TEST_OPTS_PASS(dir_prop_merge,
+ "test merge directory properties"),
SVN_TEST_NULL
};
Received on 2014-01-14 23:40:53 CET