Repository: borgbackup/borg

Issue:
master branch: check shadow_index for separate compact_segments()
Seems like the 1.1-maint code builds the `shadow_index` while doing a (borg create) transaction and then uses that in `compact_segments()` (which is done in the same borg run in 1.1).

In master, compact_segments was split into a separate command and usually not done within borg create, but later in a separate borg invoaction. Check what happens with the shadow_index data - is it lost?

Discussion:
ThomasWaldmann: will check if i can add a test for that somehow, but at least i hope that i've analyzed #4830 correctly.

@enkore can you have a look. your code looked a bit like it was already trying to use what I have added right now, see the end of `prepare_txn` (i wondered where shadow_index comes from when it is used there).
codecov-commenter: # [Codecov](https://codecov.io/gh/borgbackup/borg/pull/5263?src=pr&el=h1) Report
> Merging [#5263](https://codecov.io/gh/borgbackup/borg/pull/5263?src=pr&el=desc) into [master](https://codecov.io/gh/borgbackup/borg/commit/fc51522638480e40a3840d1e287a30ce0752f9e4&el=desc) will **decrease** coverage by `0.42%`.
> The diff coverage is `66.66%`.

[![Impacted file tree graph](https://codecov.io/gh/borgbackup/borg/pull/5263/graphs/tree.svg?width=650&height=150&src=pr&token=LssEpmW3o4)](https://codecov.io/gh/borgbackup/borg/pull/5263?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #5263      +/-   ##
==========================================
- Coverage   83.78%   83.35%   -0.43%     
==========================================
  Files          37       37              
  Lines        9951     9953       +2     
  Branches     1655     1655              
==========================================
- Hits         8337     8296      -41     
- Misses       1135     1168      +33     
- Partials      479      489      +10     
```


| [Impacted Files](https://codecov.io/gh/borgbackup/borg/pull/5263?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/borg/repository.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvcmVwb3NpdG9yeS5weQ==) | `84.04% <66.66%> (-0.25%)` | :arrow_down: |
| [src/borg/platform/\_\_init\_\_.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvcGxhdGZvcm0vX19pbml0X18ucHk=) | `72.00% <0.00%> (-12.00%)` | :arrow_down: |
| [src/borg/platform/xattr.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvcGxhdGZvcm0veGF0dHIucHk=) | `82.53% <0.00%> (-6.35%)` | :arrow_down: |
| [src/borg/xattr.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcveGF0dHIucHk=) | `48.83% <0.00%> (-3.49%)` | :arrow_down: |
| [src/borg/platform/base.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvcGxhdGZvcm0vYmFzZS5weQ==) | `77.58% <0.00%> (-3.45%)` | :arrow_down: |
| [src/borg/helpers/misc.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvaGVscGVycy9taXNjLnB5) | `77.02% <0.00%> (-2.03%)` | :arrow_down: |
| [src/borg/archive.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvYXJjaGl2ZS5weQ==) | `82.02% <0.00%> (-1.44%)` | :arrow_down: |
| [src/borg/helpers/parseformat.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvaGVscGVycy9wYXJzZWZvcm1hdC5weQ==) | `89.00% <0.00%> (-0.17%)` | :arrow_down: |
| [src/borg/cache.py](https://codecov.io/gh/borgbackup/borg/pull/5263/diff?src=pr&el=tree#diff-c3JjL2JvcmcvY2FjaGUucHk=) | `86.25% <0.00%> (-0.13%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/borgbackup/borg/pull/5263?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/borgbackup/borg/pull/5263?src=pr&el=footer). Last update [fc51522...9c42717](https://codecov.io/gh/borgbackup/borg/pull/5263?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ThomasWaldmann: Some tests fail (not the new ones, but some we had for long):
```
=================================== FAILURES ===================================
___________ RepositoryCommitTestCase.test_moved_deletes_are_tracked ____________
[gw0] linux -- Python 3.6.3 /home/travis/build/borgbackup/borg/.tox/py36/bin/python
self = <borg.testsuite.repository.RepositoryCommitTestCase testMethod=test_moved_deletes_are_tracked>
    def test_moved_deletes_are_tracked(self):
        self.repository.put(H(1), b'1')
        self.repository.put(H(2), b'2')
        self.repository.commit(compact=False)
        self.repository.delete(H(1))
        self.repository.commit(compact=True)
        last_segment = self.repository.io.get_latest_segment() - 1
        num_deletes = 0
        for tag, key, offset, size in self.repository.io.iter_objects(last_segment):
            if tag == TAG_DELETE:
                assert key == H(1)
                num_deletes += 1
        assert num_deletes == 1
        assert last_segment in self.repository.compact
        self.repository.put(H(3), b'3')
        self.repository.commit(compact=True)
        assert last_segment not in self.repository.compact
        assert not self.repository.io.segment_exists(last_segment)
        for segment, _ in self.repository.io.segment_iterator():
            for tag, key, offset, size in self.repository.io.iter_objects(segment):
>               assert tag != TAG_DELETE
E               AssertionError: assert 1 != 1
.tox/py36/lib/python3.6/site-packages/borg/testsuite/repository.py:334: AssertionError
------------------------------ Captured log call -------------------------------
INFO     borg.repository:logger.py:193 compaction freed about 101 B repository space.
INFO     borg.repository:logger.py:193 compaction freed about 59 B repository space.
_____________ RepositoryCommitTestCase.test_shadow_index_rollback ______________
[gw0] linux -- Python 3.6.3 /home/travis/build/borgbackup/borg/.tox/py36/bin/python
self = <borg.testsuite.repository.RepositoryCommitTestCase testMethod=test_shadow_index_rollback>
    def test_shadow_index_rollback(self):
        self.repository.put(H(1), b'1')
        self.repository.delete(H(1))
        assert self.repository.shadow_index[H(1)] == [0]
        self.repository.commit(compact=True)
        # note how an empty list means that nothing is shadowed for sure
>       assert self.repository.shadow_index[H(1)] == []
E       KeyError: b'00000000000000000000000000000001'
.tox/py36/lib/python3.6/site-packages/borg/testsuite/repository.py:376: KeyError
```
ThomasWaldmann: The lower one is because empty segment lists are now removed from shadow_index.
ThomasWaldmann: I reverted removing the empty segments lists from shadow_index because the code relies on them being there.

Guess they should be removed at some time though and should not stay in there forever?
enkore: Overall this looks correct to me.

> Guess they should be removed at some time though and should not stay in there forever?

Yeah. In general the repository unit tests are all super-low-level and kind of a headache to deal with; I pretty much just grabbed a pen and paper half the time to convince myself that the new behaviour, which caused a test failure, is correct.

Patch:
diff --git a/src/borg/repository.py b/src/borg/repository.py
index ad9fa66f..b3489c1a 100644
--- a/src/borg/repository.py
+++ b/src/borg/repository.py
@@ -160,6 +160,7 @@ def __init__(self, path, create=False, exclusive=False, lock_wait=None, lock=Tru
         # This is an index of shadowed log entries during this transaction. Consider the following sequence:
         # segment_n PUT A, segment_x DELETE A
         # After the "DELETE A" in segment_x the shadow index will contain "A -> [n]".
+        # .delete() is updating this index, it is persisted into "hints" file and is later used by .compact_segments().
         self.shadow_index = {}
         self._active_txn = False
         self.lock_wait = lock_wait
@@ -570,6 +571,7 @@ def prepare_txn(self, transaction_id, do_cleanup=True):
                 self.segments = hints[b'segments']
                 self.compact = FreeSpace()
                 self.storage_quota_use = 0
+                self.shadow_index = {}
                 for segment in sorted(hints[b'compact']):
                     logger.debug('Rebuilding sparse info for segment %d', segment)
                     self._rebuild_sparse(segment)
@@ -580,6 +582,7 @@ def prepare_txn(self, transaction_id, do_cleanup=True):
                 self.segments = hints[b'segments']
                 self.compact = FreeSpace(hints[b'compact'])
                 self.storage_quota_use = hints.get(b'storage_quota_use', 0)
+                self.shadow_index = hints.get(b'shadow_index', {})
             self.log_storage_quota()
             # Drop uncommitted segments in the shadow index
             for key, shadowed_segments in self.shadow_index.items():
@@ -600,6 +603,7 @@ def rename_tmp(file):
             b'segments': self.segments,
             b'compact': self.compact,
             b'storage_quota_use': self.storage_quota_use,
+            b'shadow_index': self.shadow_index,
         }
         integrity = {
             # Integrity version started at 2, the current hints version.
@@ -975,7 +979,7 @@ def report_error(msg):
         segments_transaction_id = self.io.get_segments_transaction_id()
         logger.debug('Segment transaction is    %s', segments_transaction_id)
         logger.debug('Determined transaction is %s', transaction_id)
-        self.prepare_txn(None)  # self.index, self.compact, self.segments all empty now!
+        self.prepare_txn(None)  # self.index, self.compact, self.segments, self.shadow_index all empty now!
         segment_count = sum(1 for _ in self.io.segment_iterator())
         logger.debug('Found %d segments', segment_count)
 


Test patch:
diff --git a/src/borg/testsuite/repository.py b/src/borg/testsuite/repository.py
index 9c318355..3a2d9236 100644
--- a/src/borg/testsuite/repository.py
+++ b/src/borg/testsuite/repository.py
@@ -792,6 +792,46 @@ def test_crash_before_compact(self):
             self.assert_equal(self.repository.get(H(0)), b'data2')
 
 
+class RepositoryHintsTestCase(RepositoryTestCaseBase):
+
+    def test_hints_persistence(self):
+        self.repository.put(H(0), b'data')
+        self.repository.delete(H(0))
+        self.repository.commit(compact=False)
+        shadow_index_expected = self.repository.shadow_index
+        compact_expected = self.repository.compact
+        segments_expected = self.repository.segments
+        # close and re-open the repository (create fresh Repository instance) to
+        # check whether hints were persisted to / reloaded from disk
+        self.reopen()
+        with self.repository:
+            # see also do_compact()
+            self.repository.put(H(42), b'foobar')  # this will call prepare_txn() and load the hints data
+            # check if hints persistence worked:
+            self.assert_equal(shadow_index_expected, self.repository.shadow_index)
+            self.assert_equal(compact_expected, self.repository.compact)
+            del self.repository.segments[2]  # ignore the segment created by put(H(42), ...)
+            self.assert_equal(segments_expected, self.repository.segments)
+
+    def test_hints_behaviour(self):
+        self.repository.put(H(0), b'data')
+        self.assert_equal(self.repository.shadow_index, {})
+        self.assert_true(len(self.repository.compact) == 0)
+        self.repository.delete(H(0))
+        self.repository.commit(compact=False)
+        # now there should be an entry for H(0) in shadow_index
+        self.assert_in(H(0), self.repository.shadow_index)
+        self.assert_equal(len(self.repository.shadow_index[H(0)]), 1)
+        self.assert_in(0, self.repository.compact)  # segment 0 can be compacted
+        self.repository.put(H(42), b'foobar')  # see also do_compact()
+        self.repository.commit(compact=True, threshold=0.0)  # compact completely!
+        # nothing to compact any more! no info left about stuff that does not exist any more:
+        self.assert_equal(self.repository.shadow_index[H(0)], [])
+        # segment 0 was compacted away, no info about it left:
+        self.assert_not_in(0, self.repository.compact)
+        self.assert_not_in(0, self.repository.segments)
+
+
 class RemoteRepositoryTestCase(RepositoryTestCase):
     repository = None  # type: RemoteRepository