lib/db: Add index to track locally needed files #4958
Conversation
To optimize WithNeed, which is called for the local device whenever an index update is received. No tracking for remote devices to conserve db space, as WithNeed is only queried for completion.
|
Sounds awesome; will take some pondering to review properly as it's important, so bear with me. :) |
lib/db/leveldb_transactions.go
Outdated
| // the metadata if we replace it. | ||
| oldFile, hasOldFile = t.getFile(folder, fl.Versions[0].Device, name) | ||
| } | ||
| if insertedAt == 0 { |
There was a problem hiding this comment.
One thing to take into account here that I had to fix up in updateGlobal for receive only, and that I think can be an issue already without the receive only stuff...
If we update a local file to a lower version than what is currently in there, what is the head of the version list may change without the insert being at index zero. For receive only folders this happens when the user does a "revert", but I think it could happen already today when a file gets unignored as we then tweak the version. Whether the new version becomes the head of the list or not depends on conflict resolution, and if it does not win the resolution it'll be inserted somewhere in the middle.
So, basically, if the version list is {F1, F2, F3} where F1 is our local file and the need is nil (we have the latest version), an update after unignore (or the upcoming revert operation) can make it {F2, F1', F3} or {F2, F3, F1'}, where F1' is the version-tweaked unignored file.
We need to make F2 the needed version then, despite the insert happening somewhere further down the list.
After receive only folders it becomes worse, although that's not your problem at the moment. :) The head of the list can then be a version we need, which gets withdrawn due to a change happening on that receive only device, and potentially another version from another device then becomes promoted to the head of the list and becomes the new needed file.
There was a problem hiding this comment.
Definitely true, removedAt == 0 needs to be handled as well - thanks for catching that.
Doesn't that mean that global meta counting is broken already in this scenario?
There was a problem hiding this comment.
Yeah, I think so. I have it fixed but it's on top of a couple of other things, and of course it interferes with this PR.
There was a problem hiding this comment.
Lets not block fixing that on this PR: #4968
|
Again, db stuff magic, 3 or 4 years later I still don't understand it well, so I'll leave it for you guys. |
|
@calmh Should be clean now (commented tests are "generator" tests). I can post a followup-PR for the db-update refactor that you propose in your local flags PR, good idea? |
|
@imsodin 👍 please do |
calmh
changed the title
* master: lib/protocol: Test for IsEquivalent (syncthing#4996) gui: Add tabs in device editor (syncthing#4986) lib/config, lib/model: Handle shared with information in config (fixes syncthing#4870) (syncthing#4974) gui: Restrict shown decimals and restrict size of header columns (syncthing#4973) cmd/stfindignored: Default to current directory cmd/stfindignored: Make that 2018 cmd/stfindignored: Add utility gui: Hide allowed networks if unused (syncthing#4989) lib/protocol: Correct block size calculation on 32 bit archs (fixes syncthing#4990) (syncthing#4991) lib/upgrade: Tests should pass on darwin-386 gui, man: Update docs & translations gui: Don't remove the slash from path '/' (fixes syncthing#4983) (syncthing#4988) lib/fs: Resolve 8.3 filenames from watcher (ref syncthing#3800) (syncthing#4975) lib/scanner: Skip block size hysteresis test in -short mode lib/db: Add index to track locally needed files (syncthing#4958)
st-review
added
the
frozen-due-to-age
Purpose
It took me a while longer than it took Jakob to test it, but here it is.
The needKey->nil bucket tracks locally needed files and is update whenever files are updated. Thus traversing local needs does no longer need to go over the entire global bucket detecting what we actually need, it just walks this new bucket. For remote devices I don't do the same for less complexity and smaller db size, as lookups of remote needs are less frequent - that works just like before.
Some of the refactoring is due to an earlier version of the new test that worked without the jsons db and thus needed access to parts of
readWriteTransaction.updateGlobal. In my opinion the refactor is still worthwhile.The l -> fl/vl renaming is to not override the logger variable.
Testing
I added a test for the db migration from schema 0 to 3.
Edit:
I didn't the database schema change in the local flags PR. Just ignore that aspect in the review, depending on timeline I will adjust this PR accordingly or do a separate PR for that shuffling.