Re: svn commit: r1716548 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c

[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

[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

[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

[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

[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