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 <bert_at_qqmail.nl> 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: stefan2_at_apache.org
> Sent: 14-1-2014 23:09
> To: 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-15 01:40:03 CET