Fix "put updates shadow index" #5636
Conversation
5841886 to
4e5cf7b
Compare
| self._delete(id, segment, offset) | ||
|
|
||
| def _delete(self, id, segment, offset): | ||
| # common code used by put and delete | ||
| self.shadow_index.setdefault(id, []).append(segment) |
There was a problem hiding this comment.
^ this line was missing in original put code.
Codecov Report
@@ Coverage Diff @@
## master #5636 +/- ##
==========================================
- Coverage 83.09% 82.86% -0.24%
==========================================
Files 38 38
Lines 10139 10135 -4
Branches 1680 1680
==========================================
- Hits 8425 8398 -27
- Misses 1213 1231 +18
- Partials 501 506 +5
Continue to review full report at Codecov.
|
|
OK, so looks like we had a bug here for quite a while. But borg does usually not
I just checked if borg does a explicit So, even if a user did not use Consequences? |
|
@enkore please review. |
| segment, size = self.io.write_delete(id) | ||
| self.compact[segment] += size | ||
| self.segments.setdefault(segment, 0) | ||
| self._delete(id, segment, offset) |
There was a problem hiding this comment.
It might be worth including a comment here indicating that the shadow_index will not get updated without performing an explicit delete here.
I'm not sure what to make of how borg's repository layer allows a PUT to shadow another PUT, but currently never does that (instead using DELETE + PUT). I'm not sure if PUT A, PUT A should be considered a sequence in need of repair.
There was a problem hiding this comment.
added a comment.
seems like put del put is also needed for other bookkeeping.
e.g. imagine borg recreate with recompress - data size will usually change here, but chunkid will stay same.
|
Would it be sufficient to add to |
4e5cf7b to
920319b
Compare
|
let's discuss consequences and borg check in #5661. |
The shadow_index should be in same state after both of these sequences (let's assume that A is not in repo yet for simplicity, but it does not matter): a) explicit delete: put(A), delete(A), put(A), resulting in: PUT A, DEL A, PUT A repo contents b) implicit delete: put(A), put(A), resulting in: PUT A, DEL A, PUT A repo contents
920319b to
f079a83
Compare
…5661 A) the compaction code needs the shadow index only for this case: segment A: PUT x, segment B: DEL x, with A < B (DEL shadows the PUT). B) for the following case, we have no shadowing DEL (or rather: it does not matter, because there is a PUT right after the DEL) and x is in the repo index, thus the shadow_index is not needed for the special case in the compaction code: segment A: PUT x, segment B: DEL x PUT x see also PR borgbackup#5636. reverts f079a83 and clarifies the code by more comments. we keep the code deduplication of 5f32b56 and just add a update_shadow_index param to make it not look like there was something accidentally forgotten, which was the whole reason for the reverted "fix".
…5661 A) the compaction code needs the shadow index only for this case: segment A: PUT x, segment B: DEL x, with A < B (DEL shadows the PUT). B) for the following case, we have no shadowing DEL (or rather: it does not matter, because there is a PUT right after the DEL) and x is in the repo index, thus the shadow_index is not needed for the special case in the compaction code: segment A: PUT x, segment B: DEL x PUT x see also PR borgbackup#5636. reverts f079a83 and clarifies the code by more comments. we keep the code deduplication of 5f32b56 and just add a update_shadow_index param to make it not look like there was something accidentally forgotten, which was the whole reason for the reverted "fix".
@timmc found:
"I notice that Repository.put doesn't add to shadow_index -- is that intentional? Not sure if there's an impact on this PR."
See commit comments.