You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2015/11/25 22:31:20 UTC
svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Author: rhuijben
Date: Wed Nov 25 21:31:20 2015
New Revision: 1716548
URL: http://svn.apache.org/viewvc?rev=1716548&view=rev
Log:
Create simpler reproduction recipe for BDB issue julian found using
svnmover+BDB+ra_serf.
* subversion/tests/libsvn_fs/fs-test.c
(reopen_trivial_transaction): Extend test.
Modified:
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
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=1716548&r1=1716547&r2=1716548&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Wed Nov 25 21:31:20 2015
@@ -225,6 +225,8 @@ reopen_trivial_transaction(const svn_tes
/* Don't use the subpool, txn_name must persist beyond the current txn */
SVN_ERR(svn_fs_txn_name(&txn_name, txn, pool));
+ SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 0);
+
/* Create a third transaction - we don't want that one to reopen. */
SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, subpool));
@@ -238,7 +240,43 @@ reopen_trivial_transaction(const svn_tes
SVN_ERR(svn_fs_txn_root(&root, txn, subpool));
SVN_TEST_STRING_ASSERT(svn_fs_txn_root_name(root, subpool), txn_name);
+ SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 0);
+
+ {
+ const char *conflict;
+ svn_revnum_t new_rev;
+ SVN_ERR(svn_fs_commit_txn(&conflict, &new_rev, txn, subpool));
+ SVN_TEST_STRING_ASSERT(conflict, NULL);
+ SVN_TEST_ASSERT(new_rev == 1);
+ }
+
/* Close the transaction ... again. */
+ svn_pool_clear(subpool);
+
+ /* Begin another transaction that is based on revision 1. */
+ SVN_ERR(svn_fs_begin_txn(&txn, fs, 1, subpool));
+
+ /* Don't use the subpool, txn_name must persist beyond the current txn */
+ SVN_ERR(svn_fs_txn_name(&txn_name, txn, pool));
+
+ SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 1);
+
+ /* Keep the txn name in pool */
+ SVN_ERR(svn_fs_txn_name(&txn_name, txn, pool));
+
+ /* Close the transaction ... again. */
+ svn_pool_clear(subpool);
+
+ /* Reopen the transaction by name ... again */
+ SVN_ERR(svn_fs_open_txn(&txn, fs, txn_name, subpool));
+
+ /* Does it have the same name? ... */
+ SVN_ERR(svn_fs_txn_root(&root, txn, subpool));
+ SVN_TEST_STRING_ASSERT(svn_fs_txn_root_name(root, subpool), txn_name);
+
+ /* And the same base revision? */
+ SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 1);
+
svn_pool_destroy(subpool);
return SVN_NO_ERROR;
Re: svn commit: r1716548 -
/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Bert Huijben wrote on Sat, Nov 28, 2015 at 21:58:00 +0100:
>
>
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:danielsh@apache.org]
> > Sent: zaterdag 28 november 2015 18:32
> > To: Philip Martin <ph...@wandisco.com>
> > Cc: Bert Huijben <be...@qqmail.nl>; 'Branko Čibej' <br...@apache.org>;
> > dev@subversion.apache.org
> > Subject: Re: svn commit: r1716548 -
> > /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
> >
> > On Thu, Nov 26, 2015 at 06:17:07PM +0000, Philip Martin wrote:
> > > Philip Martin <ph...@wandisco.com> writes:
> > >
> > > > I suppose one way to fix this would be to ensure that every BDB
> revision
> > > > generates a new node-revision-id.
> > >
> > > To do this we make '/' mutable either when creating the txn, or when
> > > commiting the txn. Patches to do both below. Not sure which one is
> > > best.
> >
> > Making '/' mutable at transaction creation would violate an explicit API
> > promise:
>
> If you interpret it this way FSFS always broke the promise
> >
> > /** Set @a *revision to the revision in which @a path under @a root
> was
> > * created. Use @a pool for any temporary allocations. @a *revision
> will
> > * be set to #SVN_INVALID_REVNUM for uncommitted nodes (i.e.
> > modified nodes
> > * under a transaction root). Note that the root of an unmodified
> > transaction
> > * is not itself considered to be modified; in that case, return the
> revision
> > * upon which the transaction was based.
> > */
> > svn_error_t *
> > svn_fs_node_created_rev(svn_revnum_t *revision,
>
> And strictly the root directory is not *under* the root.
>
I interpret the docstring as saying that calling
svn_fs_node_created_rev(.., path="", ...) is well-defined behaviour, and
I see no issue with that part of the docstring.
> I'm not sure what the right way to fix this problem is... but calling fsfs
> completely broken is not the solution for fixing this inconsistency.
>
No one was calling anything "completely broken". If FSFS was always
violating that particular API promise, then we should probably withdraw
that promise (by issueing an erratum) — especially now that no
non-deprecated backend meets the promise.
>
> And if you open a transaction against r3... then what is the revision the
> transaction is based on?
>
r3.
Even if r3 is a noop commit, I think the answer should be "r3",
regardless of whether (in that case) r3 and r2 share a root noderev.
> (The commit can even be applied on top of r4, if this doesn't conflict)
So, suppose a concurrent thread commits an r4 which is a noop commit
either before the open_txn(base=r3) call, or concurrently to it; should
the base revision be r3 or r4?
I think I would lean on the side of considering the choice of a base
revision a form of user data and preserve it, i.e., consider the base
revision r3, notwithstanding that the changes would not have conflicted
if they had been based on some other revision (e.g., r4).
Cheers,
Daniel
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:danielsh@apache.org]
> Sent: zaterdag 28 november 2015 18:32
> To: Philip Martin <ph...@wandisco.com>
> Cc: Bert Huijben <be...@qqmail.nl>; 'Branko Čibej' <br...@apache.org>;
> dev@subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> On Thu, Nov 26, 2015 at 06:17:07PM +0000, Philip Martin wrote:
> > Philip Martin <ph...@wandisco.com> writes:
> >
> > > I suppose one way to fix this would be to ensure that every BDB
revision
> > > generates a new node-revision-id.
> >
> > To do this we make '/' mutable either when creating the txn, or when
> > commiting the txn. Patches to do both below. Not sure which one is
> > best.
>
> Making '/' mutable at transaction creation would violate an explicit API
> promise:
If you interpret it this way FSFS always broke the promise
>
> /** Set @a *revision to the revision in which @a path under @a root
was
> * created. Use @a pool for any temporary allocations. @a *revision
will
> * be set to #SVN_INVALID_REVNUM for uncommitted nodes (i.e.
> modified nodes
> * under a transaction root). Note that the root of an unmodified
> transaction
> * is not itself considered to be modified; in that case, return the
revision
> * upon which the transaction was based.
> */
> svn_error_t *
> svn_fs_node_created_rev(svn_revnum_t *revision,
And strictly the root directory is not *under* the root.
I'm not sure what the right way to fix this problem is... but calling fsfs
completely broken is not the solution for fixing this inconsistency.
And if you open a transaction against r3... then what is the revision the
transaction is based on?
r3?
FSFS and FSX always think so
r1?
If both r2 and r3 didn't contain modifications, BDB thinks so (see the
recently added RA C test)
(The commit can even be applied on top of r4, if this doesn't conflict)
Bert
Re: svn commit: r1716548 -
/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Daniel Shahaf <da...@apache.org>.
On Thu, Nov 26, 2015 at 06:17:07PM +0000, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
> > I suppose one way to fix this would be to ensure that every BDB revision
> > generates a new node-revision-id.
>
> To do this we make '/' mutable either when creating the txn, or when
> commiting the txn. Patches to do both below. Not sure which one is
> best.
Making '/' mutable at transaction creation would violate an explicit API
promise:
/** Set @a *revision to the revision in which @a path under @a root was
* created. Use @a pool for any temporary allocations. @a *revision will
* be set to #SVN_INVALID_REVNUM for uncommitted nodes (i.e. modified nodes
* under a transaction root). Note that the root of an unmodified transaction
* is not itself considered to be modified; in that case, return the revision
* upon which the transaction was based.
*/
svn_error_t *
svn_fs_node_created_rev(svn_revnum_t *revision,
I only ran into that while answering Ren Wang on users@.
Cheers,
Daniel
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip@codematters.co.uk]
> Sent: donderdag 26 november 2015 19:37
> To: Bert Huijben <be...@qqmail.nl>
> Cc: 'Branko Čibej' <br...@apache.org>; dev@subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Philip Martin <ph...@wandisco.com> writes:
>
> > Philip Martin <ph...@wandisco.com> writes:
> >
> >> I suppose one way to fix this would be to ensure that every BDB revision
> >> generates a new node-revision-id.
> >
> > To do this we make '/' mutable either when creating the txn, or when
> > commiting the txn. Patches to do both below. Not sure which one is
> > best.
>
> Seems like this behaviour had been observed in the past, with one of my
> patches we also need to stop special-casing BDB in ra-tests:
>
> Index: subversion/tests/libsvn_ra/ra-test.c
> ==========================================================
> =========
> --- subversion/tests/libsvn_ra/ra-test.c (revision 1716725)
> +++ subversion/tests/libsvn_ra/ra-test.c (working copy)
> @@ -1606,12 +1606,7 @@ commit_empty_last_change(const
> svn_test_opts_t *op
> SVN_TEST_ASSERT(dirent != NULL);
> SVN_TEST_STRING_ASSERT(dirent->last_author, "jrandom");
>
> - /* BDB only updates last_changed on the repos_root when there is an
> - actual change. Our other filesystems handle this differently */
> - if (!opts->fs_type || !strcasecmp(opts->fs_type, "BDB"))
> - SVN_TEST_ASSERT(dirent->created_rev == 1);
> - else
> - SVN_TEST_ASSERT(dirent->created_rev == 2);
> + SVN_TEST_ASSERT(dirent->created_rev == 2);
>
> return SVN_NO_ERROR;
> }
I added this specific test early this morning/today in r1716579, as part of verifying if this behavior was BDB or ra_serf+BDB specific.
Feel free to change it :-)
Bert
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> I suppose one way to fix this would be to ensure that every BDB revision
>> generates a new node-revision-id.
>
> To do this we make '/' mutable either when creating the txn, or when
> commiting the txn. Patches to do both below. Not sure which one is
> best.
Seems like this behaviour had been observed in the past, with one of my
patches we also need to stop special-casing BDB in ra-tests:
Index: subversion/tests/libsvn_ra/ra-test.c
===================================================================
--- subversion/tests/libsvn_ra/ra-test.c (revision 1716725)
+++ subversion/tests/libsvn_ra/ra-test.c (working copy)
@@ -1606,12 +1606,7 @@ commit_empty_last_change(const svn_test_opts_t *op
SVN_TEST_ASSERT(dirent != NULL);
SVN_TEST_STRING_ASSERT(dirent->last_author, "jrandom");
- /* BDB only updates last_changed on the repos_root when there is an
- actual change. Our other filesystems handle this differently */
- if (!opts->fs_type || !strcasecmp(opts->fs_type, "BDB"))
- SVN_TEST_ASSERT(dirent->created_rev == 1);
- else
- SVN_TEST_ASSERT(dirent->created_rev == 2);
+ SVN_TEST_ASSERT(dirent->created_rev == 2);
return SVN_NO_ERROR;
}
--
Philip
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> I suppose one way to fix this would be to ensure that every BDB revision
> generates a new node-revision-id.
To do this we make '/' mutable either when creating the txn, or when
commiting the txn. Patches to do both below. Not sure which one is
best.
Index: subversion/libsvn_fs_base/revs-txns.c
===================================================================
--- subversion/libsvn_fs_base/revs-txns.c (revision 1716725)
+++ subversion/libsvn_fs_base/revs-txns.c (working copy)
@@ -695,6 +695,7 @@ txn_body_begin_txn(void *baton, trail_t *trail)
struct begin_txn_args *args = baton;
const svn_fs_id_t *root_id;
const char *txn_id;
+ dag_node_t *clone;
SVN_ERR(svn_fs_base__rev_get_root(&root_id, trail->fs, args->base_rev,
trail, trail->pool));
@@ -751,6 +752,13 @@ txn_body_begin_txn(void *baton, trail_t *trail)
SVN_ERR(txn_body_change_txn_prop(&cpargs, trail));
}
+#if 0
+ /* We want every txn to have a mutable root as then the new revision
+ will have a distinct root node-revision-id. */
+ SVN_ERR(svn_fs_base__dag_clone_root(&clone, trail->fs, txn_id,
+ trail, trail->pool));
+#endif
+
*args->txn_p = make_txn(trail->fs, txn_id, args->base_rev, trail->pool);
return SVN_NO_ERROR;
}
Index: subversion/libsvn_fs_base/tree.c
===================================================================
--- subversion/libsvn_fs_base/tree.c (revision 1716725)
+++ subversion/libsvn_fs_base/tree.c (working copy)
@@ -2661,7 +2661,7 @@ txn_body_commit(void *baton, trail_t *trail)
svn_revnum_t youngest_rev;
const svn_fs_id_t *y_rev_root_id;
- dag_node_t *txn_base_root_node;
+ dag_node_t *txn_base_root_node, *txn_root_node;
/* Getting the youngest revision locks the revisions table until
this trail is done. */
@@ -2694,6 +2694,19 @@ txn_body_commit(void *baton, trail_t *trail)
discovered locks. */
SVN_ERR(verify_locks(txn_name, trail, trail->pool));
+#if 0
+ /* We want every txn to have a mutable root as then the new revision
+ will have a distinct root node-revision-id. */
+ SVN_ERR(svn_fs_base__dag_txn_root(&txn_root_node, fs, txn_name,
+ trail, trail->pool));
+ if (!svn_fs_base__dag_check_mutable(txn_root_node, txn->id))
+ {
+ dag_node_t *clone;
+ SVN_ERR(svn_fs_base__dag_clone_root(&clone, fs, txn->id,
+ trail, trail->pool));
+ }
+#endif
+
/* Else, commit the txn. */
return svn_fs_base__dag_commit_txn(&(args->new_rev), txn, trail,
trail->pool);
--
Philip Martin
WANdisco
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: vrijdag 27 november 2015 10:28
> To: 'Daniel Shahaf' <d....@daniel.shahaf.name>; 'Philip Martin'
> <ph...@wandisco.com>
> Cc: 'Branko Čibej' <br...@apache.org>; dev@subversion.apache.org
> Subject: RE: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
> =======
> And all of this brings more interesting cases to the discussion on what
> changes are... So some of this should probably be added to Julian's
> document on what path changes are.
>
> I'm pretty sure this 'problem' adds other nice features where dump+load
> may slightly alter behavior... even though the repository is still 100% the
> same in textual+property changes.
One interesting historic note is that in Subversion 1.0 we didn't have an RA api to fetch the last-* values on the repository root directly.
We worked around that in later versions by 'just assuming' that the root always changes and fetching the values on the revision.
Bert
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: vrijdag 27 november 2015 08:50
> To: Philip Martin <ph...@wandisco.com>
> Cc: Bert Huijben <be...@qqmail.nl>; 'Branko Čibej' <br...@apache.org>;
> dev@subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Philip Martin wrote on Thu, Nov 26, 2015 at 13:46:39 +0000:
> > I suppose one way to fix this would be to ensure that every BDB revision
> > generates a new node-revision-id.
>
> I wouldn't call this a fix; I think it is a workaround. A "fix" would
> be to figure out why bdb reports the wrong revision number, not to make
> the place it wrongly looks the value up in contain the right value.
>
> To be clear, I'm not opposed to applying your patches — I do think they
> are an improvement. I just want it to be clear: re-using the noderev
> wasn't wrong; some other code is wrong, and we don't know which code
> exactly that is.
>
> Can the problem happen on nodes other than the root? I haven't tried
> it, but I wonder if a open_root/open_directory/close_directory/close_edit
> editor drive might lead to an instance of this problem on the directory
> that was opened and closed without modification.
I'm not sure what the real bug is here:
* For 1.10 I added some verifications on incoming revision numbers in mod_dav_svn. These expect that the base revision numbers passed to editor functions are always lower than the revision against we commit. I still think this is a good check.
(But I'm not 100% sure if we correctly find the revision we commit against)
Before our svn-HTTPv2 we had a similar base revision check in a different code path... But we skip this code path for our streamlined v2 protocol.
* To handle this we open a transaction and reload it a few times via its name. Once created it returns one revision. Once reloaded it returns a different -older- revision. I think *this behavior* is broken. We should either directly return the older revision... or always return the newer revision.
-> This could be as simple as storing it in the transaction name, by appending something like "/r123" to the name if we detect this case.
=======
And when researching this issue I found that the root of the repository still has the same 'last-changed-*' values in this case of a no-op revision.
With 1.9 we optimized some code in the repos layer under the assumption that the repos-root 'always changes' in each and every revision. I think this still works ok, but I'm not 100% sure.
=======
And all of this brings more interesting cases to the discussion on what changes are... So some of this should probably be added to Julian's document on what path changes are.
I'm pretty sure this 'problem' adds other nice features where dump+load may slightly alter behavior... even though the repository is still 100% the same in textual+property changes.
Bert
Re: svn commit: r1716548 -
/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Fri, Nov 27, 2015 at 09:27:40 +0000:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Philip Martin wrote on Thu, Nov 26, 2015 at 13:46:39 +0000:
> >> I suppose one way to fix this would be to ensure that every BDB revision
> >> generates a new node-revision-id.
> >
> > I wouldn't call this a fix; I think it is a workaround. A "fix" would
> > be to figure out why bdb reports the wrong revision number, not to make
> > the place it wrongly looks the value up in contain the right value.
>
> I don't understand whay you want. BDB does not store the base revision
> in the txn, it stores the base node-revision-id instead. Revisions
> usually map 1:1 to the base node-revision-ids but the editor drive
> "begin_txn, commit" breaks this because it creates a revision that
> shares the base node-revision-id with the previous revision. My patch
> makes the mapping 1:1 so storing the node-revision-id works.
>
The tree layer assumes the DAG layer never shares root noderevs among
revisions. That assumption is false. You proposed to make the DAG
layer meet the assumption. I proposed to make the tree layer not make
the assumption.
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: zaterdag 12 december 2015 09:20
> To: Julian Foad <ju...@gmail.com>
> Cc: Philip Martin <ph...@wandisco.com>; Bert Huijben
> <be...@qqmail.nl>; dev@subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Julian Foad wrote on Thu, Dec 10, 2015 at 14:27:44 +0000:
> > Philip Martin wrote:
> > > This discussion seems to have died out. Are we going to leave the BDB
> > > code as is, in which case we should mark the failing test XFAIL, or
make
> > > a change? I still prefer the option that makes the root path mutable
on
> > > commit, primarily because for the vast majority of commits there is no
> > > change at all.
> >
> > Stepping back a little, I want to pose the rhetorical question: Who is
> > to say which FS layer implementation is the wrong one? BDB is the
> > earlier implementation. If we define correctness by precedent, then
> > it's the FSFS behaviour that we should consider to be wrong. On the
> > other hand, if we define correctness by specification, then we need to
> > specify this behaviour somewhere, not just "randomly" change it.
> >
> > So let's try to enumerate the issues.
> >
> > (1) In FS-BDB, a no-op commit may or may not produce a new root
> > node-rev (depending on the specific form of the commit), whereas in
> > FSFS, every commit produces a new root node-rev.
> >
> > (2) FS-BDB reports a different result from svn_fs_txn_base_revision()
> > before and after reloading by name, when the no-new-node-rev situation
> > in (1) occurs.
> >
> > (3) A recently added check can reject valid delete operations when (1)
> > and (2) occur.
> >
> > Which of those are bugs, which are acceptable, which need to stay as
> > they for backward compatibility even if they are bugs, and so on?
> >
> > It seems to me that (2) is definitely a bug, but I'm not sure about
> > (1) and therefore not sure about (3).
> >
>
> About (2): I think svn_fs_txn_base_revision() should return the value of
> the REV argument of the svn_fs_begin_txn() call that created the txn.
> Therefore, (2) is a bug.
>
> About (1): I think that, unless we have made specific API promises about
> when noderevs are or aren't shared, the decision of whether to share
> noderevs is an implementation detail of the backend: the backend may
> choose to share or not to share, but neither choice is more "right" than
> the other.
>
> Therefore: if making BDB never share the root path's noderevs fixes the
> bug, then I think it's a fine way to proceed. I just don't think it's
> the *only* correct way to proceed. For example, I think the FSFS f7
> logical addressing file format supports noderev sharing of the root path
> of revisions within a single pack file, so FSFS 1.10 could conceivably
> start sharing the root path's noderevs in some circumstances.
>
> So... yes, we can go ahead and make libsvn_fs_base never share the root
> path's noderevs. No problem at all with that. But higher-level code
> shouldn't rely on that: if it hasn't been documented as an API promise,
> it's an implementation detail and subject to change.
>
> I hope this clarifies my viewpoint...
Thanks for this explanation...
With this explanation I think we can go ahead and commit the fix suggested
by philip to fix the actual problem we found... (2).
But we explicitly *don't* make (1) promised behavior...
Which results in fixing (3), and happy buildbots :-)
With my recent work on ra-git, I really don't want to attach anything to
noderev information... We can map our whole filesystem api implementation
over git repositories, except for the noderevision thing.
We moved far enough to node relations in our backend to avoid implementing
actual noderevisions in libsvn_fs_git now...
Bert
Re: svn commit: r1716548 -
/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Thu, Dec 10, 2015 at 14:27:44 +0000:
> Philip Martin wrote:
> > This discussion seems to have died out. Are we going to leave the BDB
> > code as is, in which case we should mark the failing test XFAIL, or make
> > a change? I still prefer the option that makes the root path mutable on
> > commit, primarily because for the vast majority of commits there is no
> > change at all.
>
> Stepping back a little, I want to pose the rhetorical question: Who is
> to say which FS layer implementation is the wrong one? BDB is the
> earlier implementation. If we define correctness by precedent, then
> it's the FSFS behaviour that we should consider to be wrong. On the
> other hand, if we define correctness by specification, then we need to
> specify this behaviour somewhere, not just "randomly" change it.
>
> So let's try to enumerate the issues.
>
> (1) In FS-BDB, a no-op commit may or may not produce a new root
> node-rev (depending on the specific form of the commit), whereas in
> FSFS, every commit produces a new root node-rev.
>
> (2) FS-BDB reports a different result from svn_fs_txn_base_revision()
> before and after reloading by name, when the no-new-node-rev situation
> in (1) occurs.
>
> (3) A recently added check can reject valid delete operations when (1)
> and (2) occur.
>
> Which of those are bugs, which are acceptable, which need to stay as
> they for backward compatibility even if they are bugs, and so on?
>
> It seems to me that (2) is definitely a bug, but I'm not sure about
> (1) and therefore not sure about (3).
>
About (2): I think svn_fs_txn_base_revision() should return the value of
the REV argument of the svn_fs_begin_txn() call that created the txn.
Therefore, (2) is a bug.
About (1): I think that, unless we have made specific API promises about
when noderevs are or aren't shared, the decision of whether to share
noderevs is an implementation detail of the backend: the backend may
choose to share or not to share, but neither choice is more "right" than
the other.
Therefore: if making BDB never share the root path's noderevs fixes the
bug, then I think it's a fine way to proceed. I just don't think it's
the *only* correct way to proceed. For example, I think the FSFS f7
logical addressing file format supports noderev sharing of the root path
of revisions within a single pack file, so FSFS 1.10 could conceivably
start sharing the root path's noderevs in some circumstances.
So... yes, we can go ahead and make libsvn_fs_base never share the root
path's noderevs. No problem at all with that. But higher-level code
shouldn't rely on that: if it hasn't been documented as an API promise,
it's an implementation detail and subject to change.
I hope this clarifies my viewpoint...
Cheers,
Daniel
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@gmail.com> writes:
> Stepping back a little, I want to pose the rhetorical question: Who is
> to say which FS layer implementation is the wrong one? BDB is the
> earlier implementation. If we define correctness by precedent, then
> it's the FSFS behaviour that we should consider to be wrong. On the
> other hand, if we define correctness by specification, then we need to
> specify this behaviour somewhere, not just "randomly" change it.
We could leave it unspecified, or explicitly specify that the backend
has a choice.
> So let's try to enumerate the issues.
>
> (1) In FS-BDB, a no-op commit may or may not produce a new root
> node-rev (depending on the specific form of the commit), whereas in
> FSFS, every commit produces a new root node-rev.
>
> (2) FS-BDB reports a different result from svn_fs_txn_base_revision()
> before and after reloading by name, when the no-new-node-rev situation
> in (1) occurs.
>
> (3) A recently added check can reject valid delete operations when (1)
> and (2) occur.
I'd say the check relies on (2). The check is not explicitly assuming
anything about the root nodes. It is a backend bug that BDB uses the
root node as a proxy for the base revision and causes the bug.
> Which of those are bugs, which are acceptable, which need to stay as
> they for backward compatibility even if they are bugs, and so on?
>
> It seems to me that (2) is definitely a bug, but I'm not sure about
> (1) and therefore not sure about (3).
(2) is a bug. We fix it by making BDB always make a new root node.
> Daniel Shahaf wrote on 27 Nov:
>> Can the problem happen on nodes other than the root? I haven't tried
>> it, but I wonder if a open_root/open_directory/close_directory/close_edit
>> editor drive might lead to an instance of this problem on the directory
>> that was opened and closed without modification.
>
> Yes, that's an important question too. In other words, what semantics
> do we want for a "no-op change" in general, within a commit?
>
> And is it possible to make a commit which starts with opening (as the
> root of the commit) a directory that is not the root of the
> repository, and if so, what about that case?
I don't think that is relevant. This about the FS API so the calls are
begin_txn and commit_txn.
--
Philip Martin
WANdisco
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Julian Foad <ju...@gmail.com>.
Philip Martin wrote:
> This discussion seems to have died out. Are we going to leave the BDB
> code as is, in which case we should mark the failing test XFAIL, or make
> a change? I still prefer the option that makes the root path mutable on
> commit, primarily because for the vast majority of commits there is no
> change at all.
Stepping back a little, I want to pose the rhetorical question: Who is
to say which FS layer implementation is the wrong one? BDB is the
earlier implementation. If we define correctness by precedent, then
it's the FSFS behaviour that we should consider to be wrong. On the
other hand, if we define correctness by specification, then we need to
specify this behaviour somewhere, not just "randomly" change it.
So let's try to enumerate the issues.
(1) In FS-BDB, a no-op commit may or may not produce a new root
node-rev (depending on the specific form of the commit), whereas in
FSFS, every commit produces a new root node-rev.
(2) FS-BDB reports a different result from svn_fs_txn_base_revision()
before and after reloading by name, when the no-new-node-rev situation
in (1) occurs.
(3) A recently added check can reject valid delete operations when (1)
and (2) occur.
Which of those are bugs, which are acceptable, which need to stay as
they for backward compatibility even if they are bugs, and so on?
It seems to me that (2) is definitely a bug, but I'm not sure about
(1) and therefore not sure about (3).
Daniel Shahaf wrote on 27 Nov:
> Can the problem happen on nodes other than the root? I haven't tried
> it, but I wonder if a open_root/open_directory/close_directory/close_edit
> editor drive might lead to an instance of this problem on the directory
> that was opened and closed without modification.
Yes, that's an important question too. In other words, what semantics
do we want for a "no-op change" in general, within a commit?
And is it possible to make a commit which starts with opening (as the
root of the commit) a directory that is not the root of the
repository, and if so, what about that case?
If we don't address these questions as well, we might not be making
much progress by addressing (1) and (2) and (3) only.
- Julian
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> I prefer the solution that makes the root path mutable before commit so
>> that the revision has a distinct root node-revision-id.
>
> I don't know which solution is better. I just posted the other option.
This discussion seems to have died out. Are we going to leave the BDB
code as is, in which case we should mark the failing test XFAIL, or make
a change? I still prefer the option that makes the root path mutable on
commit, primarily because for the vast majority of commits there is no
change at all.
--
Philip Martin
WANdisco
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: vrijdag 27 november 2015 13:46
> To: Bert Huijben <be...@qqmail.nl>
> Cc: 'Daniel Shahaf' <d....@daniel.shahaf.name>; 'Branko Čibej'
> <br...@apache.org>; dev@subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> > Another option would be to just store the revision number in the
> > string representation of the txn-id and keep the on-disk behavior the
> > same as it is today. Can we just add an (optional) component there?
>
> I suppose that would work. It might break any third-party code that
> parses BDB txn-ids. There probably isn't any such code for BDB but I am
> aware of a comparable issue in some 3rd party FSFS code
>
> > Any fix that would make us load the txn back the way it was before
> > serializing it would be a good fix for me.
> >
> > I would also be +0 on a fix that makes the base rev lower directly on
> > creating the txn. (I'll fix mod_dav in that case)
>
> I think that is the wrong solution. Our public API passes a base
> revision to svn_fs_begin_txn() and I think it would be odd if
> svn_fs_txn_base_revision() did not return the same revision. I think
> that is preserving the behaviour that is most likely to break 3rd party
> code.
>
> I prefer the solution that makes the root path mutable before commit so
> that the revision has a distinct root node-revision-id.
I don't know which solution is better. I just posted the other option.
I always assumed that the textual representation of txn-id was black box though...
I wouldn't have called that part of our API promise. (We should just be able to use ids created by older versions)
@julian, didn't you update the documentation on the related function recently?
Bert
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> Another option would be to just store the revision number in the
> string representation of the txn-id and keep the on-disk behavior the
> same as it is today. Can we just add an (optional) component there?
I suppose that would work. It might break any third-party code that
parses BDB txn-ids. There probably isn't any such code for BDB but I am
aware of a comparable issue in some 3rd party FSFS code
> Any fix that would make us load the txn back the way it was before
> serializing it would be a good fix for me.
>
> I would also be +0 on a fix that makes the base rev lower directly on
> creating the txn. (I'll fix mod_dav in that case)
I think that is the wrong solution. Our public API passes a base
revision to svn_fs_begin_txn() and I think it would be odd if
svn_fs_txn_base_revision() did not return the same revision. I think
that is preserving the behaviour that is most likely to break 3rd party
code.
I prefer the solution that makes the root path mutable before commit so
that the revision has a distinct root node-revision-id.
--
Philip Martin
WANdisco
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: vrijdag 27 november 2015 10:28
> To: Daniel Shahaf <d....@daniel.shahaf.name>
> Cc: Bert Huijben <be...@qqmail.nl>; 'Branko Čibej' <br...@apache.org>;
> dev@subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Philip Martin wrote on Thu, Nov 26, 2015 at 13:46:39 +0000:
> >> I suppose one way to fix this would be to ensure that every BDB revision
> >> generates a new node-revision-id.
> >
> > I wouldn't call this a fix; I think it is a workaround. A "fix" would
> > be to figure out why bdb reports the wrong revision number, not to make
> > the place it wrongly looks the value up in contain the right value.
>
> I don't understand whay you want. BDB does not store the base revision
> in the txn, it stores the base node-revision-id instead. Revisions
> usually map 1:1 to the base node-revision-ids but the editor drive
> "begin_txn, commit" breaks this because it creates a revision that
> shares the base node-revision-id with the previous revision. My patch
> makes the mapping 1:1 so storing the node-revision-id works.
>
> If you expect a patch that causes BDB to change the txn storage on disk
> to include the base revision (or instead of the base node-revision-id?)
> then you will have to explain why that is better.
Another option would be to just store the revision number in the string representation of the txn-id and keep the on-disk behavior the same as it is today. Can we just add an (optional) component there?
Any fix that would make us load the txn back the way it was before serializing it would be a good fix for me.
I would also be +0 on a fix that makes the base rev lower directly on creating the txn. (I'll fix mod_dav in that case)
But I think we should at least behave the same before and after serializing the txn.
Bert
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Philip Martin wrote on Thu, Nov 26, 2015 at 13:46:39 +0000:
>> I suppose one way to fix this would be to ensure that every BDB revision
>> generates a new node-revision-id.
>
> I wouldn't call this a fix; I think it is a workaround. A "fix" would
> be to figure out why bdb reports the wrong revision number, not to make
> the place it wrongly looks the value up in contain the right value.
I don't understand whay you want. BDB does not store the base revision
in the txn, it stores the base node-revision-id instead. Revisions
usually map 1:1 to the base node-revision-ids but the editor drive
"begin_txn, commit" breaks this because it creates a revision that
shares the base node-revision-id with the previous revision. My patch
makes the mapping 1:1 so storing the node-revision-id works.
If you expect a patch that causes BDB to change the txn storage on disk
to include the base revision (or instead of the base node-revision-id?)
then you will have to explain why that is better.
--
Philip Martin
WANdisco
Re: svn commit: r1716548 -
/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Thu, Nov 26, 2015 at 13:46:39 +0000:
> I suppose one way to fix this would be to ensure that every BDB revision
> generates a new node-revision-id.
I wouldn't call this a fix; I think it is a workaround. A "fix" would
be to figure out why bdb reports the wrong revision number, not to make
the place it wrongly looks the value up in contain the right value.
To be clear, I'm not opposed to applying your patches — I do think they
are an improvement. I just want it to be clear: re-using the noderev
wasn't wrong; some other code is wrong, and we don't know which code
exactly that is.
Can the problem happen on nodes other than the root? I haven't tried
it, but I wonder if a open_root/open_directory/close_directory/close_edit
editor drive might lead to an instance of this problem on the directory
that was opened and closed without modification.
Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> It looks like BDB stores a 'base node-revision-id' in the transaction
> instead of the actual base revision passed.
Yes, and there is a difference between how BDB and FSFS allocate
node-revision-ids. Create a repository with an empty revision that can
be used to produce an empty dumpfile:
svnadmin create empty
svnmucc -mm propdel p file://`pwd`/empty
Load the empty dumpfile into FSFS and BDB:
svnadmin create fsfs
svnadmin dump empty | svnadmin load fsfs
svnadmin create bdb --fs-type bdb
svnadmin dump empty | svnadmin load bdb
Look at the different node behaviour, for FSFS the empty revision
creates a new node-revision-id:
$ svnlook tree --show-ids -r0 fsfs
/ <0.0.r0/2>
$ svnlook tree --show-ids -r1 fsfs
/ <0.0.r1/2>
but for BDB the node-revision-id is unchanged:
$ svnlook tree --show-ids -r0 bdb
/ <0.0.0>
$ svnlook tree --show-ids -r1 bdb
/ <0.0.0>
This only happens for some no-op editor drives, the propdel used to
create the first empty revision does not cause this effect.
I suppose one way to fix this would be to ensure that every BDB revision
generates a new node-revision-id.
--
Philip Martin
WANdisco
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@apache.org]
> Sent: woensdag 25 november 2015 23:43
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> On 25.11.2015 23:27, Bert Huijben wrote:
> >> -----Original Message-----
> >> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> >> Sent: woensdag 25 november 2015 22:31
> >> To: commits@subversion.apache.org
> >> Subject: svn commit: r1716548 -
> >> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
> >>
> >> Author: rhuijben
> >> Date: Wed Nov 25 21:31:20 2015
> >> New Revision: 1716548
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1716548&view=rev
> >> Log:
> >> Create simpler reproduction recipe for BDB issue julian found using
> >> svnmover+BDB+ra_serf.
> > Any BDB hackers around that know the background behind this?
> >
> > The question is: Why isn't the base revision properly saved in the txn when
> the HEAD revision is a no-op commit?
> >
> > I'm guessing that we safe something else and then accidentally fetch the
> last modified revision of that and use it as the BASE revision of the
> transaction.
>
> Used to be that BDB would take the BASE from the reporter as the BASE
> for the commit txn, and only change it in the rebase phase of the
> commit; whereas fsfs uses the current HEAD. Don't know if anyone ever
> changed that in BDB.
>
> In general it shouldn't matter which BASE the commit txn begins with;
> but if it's HEAD, we can bail out earlier in the presence of certain
> kinds of conflicts.
Minor update on my research:
It looks like BDB stores a 'base node-revision-id' in the transaction instead of the actual base revision passed.
New for 1.10 we started verifying incoming revisions in mod_dav_svn to produce better out of date messages in a few extreme corner cases.
In this case it checks that the passed BASE revision is not newer than the revision against which the transaction was opened.
But now we found that BDB has a case where the base revision after reloading the txn from disk may be lower than the one that was in there when the txn was started (before reloading).
I can see reasons why BDB wants to use that lower base revision... but having a txn produce a different result from svn_fs_txn_base_revision() before and after reloading by name is not good.
Bert
Re: svn commit: r1716548 -
/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Branko Čibej <br...@apache.org>.
On 25.11.2015 23:27, Bert Huijben wrote:
>> -----Original Message-----
>> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
>> Sent: woensdag 25 november 2015 22:31
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1716548 -
>> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>>
>> Author: rhuijben
>> Date: Wed Nov 25 21:31:20 2015
>> New Revision: 1716548
>>
>> URL: http://svn.apache.org/viewvc?rev=1716548&view=rev
>> Log:
>> Create simpler reproduction recipe for BDB issue julian found using
>> svnmover+BDB+ra_serf.
> Any BDB hackers around that know the background behind this?
>
> The question is: Why isn't the base revision properly saved in the txn when the HEAD revision is a no-op commit?
>
> I'm guessing that we safe something else and then accidentally fetch the last modified revision of that and use it as the BASE revision of the transaction.
Used to be that BDB would take the BASE from the reporter as the BASE
for the commit txn, and only change it in the rebase phase of the
commit; whereas fsfs uses the current HEAD. Don't know if anyone ever
changed that in BDB.
In general it shouldn't matter which BASE the commit txn begins with;
but if it's HEAD, we can bail out earlier in the presence of certain
kinds of conflicts.
-- Brane
RE: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> Sent: woensdag 25 november 2015 22:31
> To: commits@subversion.apache.org
> Subject: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Author: rhuijben
> Date: Wed Nov 25 21:31:20 2015
> New Revision: 1716548
>
> URL: http://svn.apache.org/viewvc?rev=1716548&view=rev
> Log:
> Create simpler reproduction recipe for BDB issue julian found using
> svnmover+BDB+ra_serf.
Any BDB hackers around that know the background behind this?
The question is: Why isn't the base revision properly saved in the txn when the HEAD revision is a no-op commit?
I'm guessing that we safe something else and then accidentally fetch the last modified revision of that and use it as the BASE revision of the transaction.
Bert
>
> * subversion/tests/libsvn_fs/fs-test.c
> (reopen_trivial_transaction): Extend test.
>
> Modified:
> subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> 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=1716548&r1=1716547&r2=1716548&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Wed Nov 25
> 21:31:20 2015
> @@ -225,6 +225,8 @@ reopen_trivial_transaction(const svn_tes
> /* Don't use the subpool, txn_name must persist beyond the current txn */
> SVN_ERR(svn_fs_txn_name(&txn_name, txn, pool));
>
> + SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 0);
> +
> /* Create a third transaction - we don't want that one to reopen. */
> SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, subpool));
>
> @@ -238,7 +240,43 @@ reopen_trivial_transaction(const svn_tes
> SVN_ERR(svn_fs_txn_root(&root, txn, subpool));
> SVN_TEST_STRING_ASSERT(svn_fs_txn_root_name(root, subpool),
> txn_name);
>
> + SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 0);
> +
> + {
> + const char *conflict;
> + svn_revnum_t new_rev;
> + SVN_ERR(svn_fs_commit_txn(&conflict, &new_rev, txn, subpool));
> + SVN_TEST_STRING_ASSERT(conflict, NULL);
> + SVN_TEST_ASSERT(new_rev == 1);
> + }
> +
> /* Close the transaction ... again. */
> + svn_pool_clear(subpool);
> +
> + /* Begin another transaction that is based on revision 1. */
> + SVN_ERR(svn_fs_begin_txn(&txn, fs, 1, subpool));
> +
> + /* Don't use the subpool, txn_name must persist beyond the current txn
> */
> + SVN_ERR(svn_fs_txn_name(&txn_name, txn, pool));
> +
> + SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 1);
> +
> + /* Keep the txn name in pool */
> + SVN_ERR(svn_fs_txn_name(&txn_name, txn, pool));
> +
> + /* Close the transaction ... again. */
> + svn_pool_clear(subpool);
> +
> + /* Reopen the transaction by name ... again */
> + SVN_ERR(svn_fs_open_txn(&txn, fs, txn_name, subpool));
> +
> + /* Does it have the same name? ... */
> + SVN_ERR(svn_fs_txn_root(&root, txn, subpool));
> + SVN_TEST_STRING_ASSERT(svn_fs_txn_root_name(root, subpool),
> txn_name);
> +
> + /* And the same base revision? */
> + SVN_TEST_ASSERT(svn_fs_txn_base_revision(txn) == 1);
> +
> svn_pool_destroy(subpool);
>
> return SVN_NO_ERROR;
>