default mode of 'svn cleanup' changed

[Stefan Sperling <st...@elego.de>]

I have slightly changed the default mode of 'svn cleanup' in r1802797.
It will no longer remove unreferenced pristines from the pristine store.

Instead, the new --vacuum-pristines option must be used, added in r1802787.
This runs the same operation under a working copy write lock.

Note that the default mode of 'svn cleanup' breaks working copy locks.

The idea is prevent accidental working copy corruption in the unlikely
but possible case where a user runs 'svn cleanup' with the intention of
removing unused pristines while another SVN client is concurrently writing
to the working copy in the background.

Users will now find that this no longer works, and will find the new
option instead. This is also the first time we have a documented way of
cleaning up pristines (neither 'svn help' nor the book ever explained
how to do this).

If there are objections to this change, please raise them in this thread.
The changes are very small and easy to revert if necessary.

Another idea which was discussed on IRC was that the default mode of
'svn cleanup' could be changed to not break locks, and require an
option for that mode instead. However, I believe such a change would
be more disruptive and incompatible with the existing behaviour.

Re: default mode of 'svn cleanup' changed

[Stefan Sperling <st...@elego.de>]

On Tue, Jul 25, 2017 at 11:09:21AM +0000, Daniel Shahaf wrote:
> Stefan Sperling wrote on Mon, 24 Jul 2017 18:34 +0200:
> > Daniel, I'm thinking that reverting r1802797, which changed the default
> > behaviour of 'svn cleanup', may be good enough to address your backwards
> > compat concerns, while still giving people a choice to remove pristines
> > without risking a disturbance of other active clients.
> 
> I'm not sure I understand.  Are you saying that the default won't change, but
> there will be a new API that allows vacuuming pristines without breaking
> locks?  +1 to that.

Yes. By default 'svn cleanup' holds a lock while cleaning up pristines,
so I see no reason to change its default behaviour. Only one client will
be vacuuming pristines at a given time. A disturbed client will error out
next time it attempts to access wc.db after its lock was stolen.

The default remains "break locks, vacuum pristines, clean dav cache, etc.".

But the new option for just "vacuum pristines" is valuable in itself, since
you may not want to forcefully break locks for this.

And there is no new (libsvn_client) API. The existing API already provides
the feature. It was just not exposed at the CLI level so far.

Re: default mode of 'svn cleanup' changed

[Daniel Shahaf <d....@daniel.shahaf.name>]

Stefan Sperling wrote on Mon, 24 Jul 2017 18:34 +0200:
> Daniel, I'm thinking that reverting r1802797, which changed the default
> behaviour of 'svn cleanup', may be good enough to address your backwards
> compat concerns, while still giving people a choice to remove pristines
> without risking a disturbance of other active clients.

I'm not sure I understand.  Are you saying that the default won't change, but
there will be a new API that allows vacuuming pristines without breaking
locks?  +1 to that.

(If I misunderstood let's try to clarify on IRC for higher bandwidth there)

Re: default mode of 'svn cleanup' changed

[Julian Foad <ju...@apache.org>]

Stefan Sperling wrote:
> The only problem which remains is that the user is currently being given
> no choice to either steal locks or vacuum pristines. [...]
The other problem you uncovered is that 'vacuuming' pristines is not
mentioned at all in the help string.

- Julian

Re: default mode of 'svn cleanup' changed

[Stefan Sperling <st...@elego.de>]

On Mon, Jul 24, 2017 at 06:22:10PM +0200, Stefan Sperling wrote:
> On Mon, Jul 24, 2017 at 03:40:32PM +0000, Daniel Shahaf wrote:
> > It seems the real fix would be figuring out a way for 'cleanup' not to
> > break all locks it sees.
> 
> Actually, I missed that cleanup_internal() grabs a WC lock, so the
> entire point of my argument is moot. The other client manipulating
> WC state will error out if it runs any DB operation after 'cleanup'
> has stolen the lock.
> 
> So I guess I'll just revert my commits :-)

Responding to myself: The only problem which remains is that the user
is currently being given no choice to either steal locks or vacuum pristines.
They can only run these operations in combined form.

Daniel, I'm thinking that reverting r1802797, which changed the default
behaviour of 'svn cleanup', may be good enough to address your backwards
compat concerns, while still giving people a choice to remove pristines
without risking a disturbance of other active clients.

Re: default mode of 'svn cleanup' changed

[Stefan Sperling <st...@elego.de>]

On Mon, Jul 24, 2017 at 03:40:32PM +0000, Daniel Shahaf wrote:
> It seems the real fix would be figuring out a way for 'cleanup' not to
> break all locks it sees.

Actually, I missed that cleanup_internal() grabs a WC lock, so the
entire point of my argument is moot. The other client manipulating
WC state will error out if it runs any DB operation after 'cleanup'
has stolen the lock.

So I guess I'll just revert my commits :-)

Re: default mode of 'svn cleanup' changed

[Daniel Shahaf <d....@daniel.shahaf.name>]

Stefan Sperling wrote on Mon, 24 Jul 2017 14:09 +0200:
> I have slightly changed the default mode of 'svn cleanup' in r1802797.
> It will no longer remove unreferenced pristines from the pristine store.
> 
> Instead, the new --vacuum-pristines option must be used, added in r1802787.
> This runs the same operation under a working copy write lock.
> 
> Note that the default mode of 'svn cleanup' breaks working copy locks.
> 
> The idea is prevent accidental working copy corruption in the unlikely
> but possible case where a user runs 'svn cleanup' with the intention of
> removing unused pristines while another SVN client is concurrently writing
> to the working copy in the background.
> 
> Users will now find that this no longer works, and will find the new
> option instead. This is also the first time we have a documented way of
> cleaning up pristines (neither 'svn help' nor the book ever explained
> how to do this).
> 

No, this isn't:

    https://subversion.apache.org/docs/release-notes/1.7#wc-pristines
    In 1.7, we recommend to run `svn cleanup` periodically in order to
    reclaim the disk space of unreferenced pristines.

> If there are objections to this change, please raise them in this thread.
> The changes are very small and easy to revert if necessary.
> 

This change is backwards incompatible: users who upgrade to 1.10 and do
not start using the new options, will never garbage collect pristines,
and consequently their .svn/pristine/ would grow without bound until
they change scripts to work around this change.

I think that would be a disk space usage regression in some use patterns
(such as buildbot in "run 'svn update' between builds" mode), and thus I
would prefer to find a different way to plug the race condition you
started from.

(For example, I thought we might have 'cleanup' garbage collect
pristines older than 24 hours in its default mode, but that would need
to handle the case that the concurrent checkout operation wants to
re-use one of those old pristines.)

It seems the real fix would be figuring out a way for 'cleanup' not to
break all locks it sees.  There's a continuum of options between "break
all locks by default" and "require opt-in to break locks".  For example,
we could do what Vim does and record the (hostname, pid) pair that took
each lock, and then silently break locks running on the same hostname by
pid's that no longer exist.

To be clear, that was just an example.  There are plenty of other
options in that continuum.  My tldr is that I am concerned about a disk
space regression and think the right fix is to make 'cleanup' less
trigger-happy / greedy about locks.

> Another idea which was discussed on IRC was that the default mode of
> 'svn cleanup' could be changed to not break locks, and require an
> option for that mode instead. However, I believe such a change would
> be more disruptive and incompatible with the existing behaviour.

Cheers,

Daniel