Hi Bert, We only accept incoming prop changes if there were no prop changes at the directory.
-- Stefan^2. On Tue, Jan 14, 2014 at 11:39 PM, Bert Huijben <[email protected]> wrote: > 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 > ------------------------------ > From: [email protected] > Sent: 14-1-2014 23:09 > To: [email protected] > 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 > }; > > >
