go: store/nbs: reject a file manifest update that references a missing table file - #11430
go: store/nbs: reject a file manifest update that references a missing table file#11430reltuk wants to merge 2 commits into
Conversation
…g table file A bare local store takes no cross-process lock while a table file is landing, so between the moment a writer opens or renames a table file and the moment it commits a manifest naming it, another process can unlink it. The writer does not notice, since its own file descriptor keeps working, and it publishes a manifest that is broken for everyone else. Add a sanity check where we Stat the newly added files under the manifest LOCK. This allows other Dolt processes which want to safely unlink unreferenced files from the directory to do so under the same LOCK. While a write may transiently fail if an about-to-be-referenced file is deleted out from under it, it won't cause database corruption by writing a manifest which references a non-existant table file. This PR adds the add-files sanity check. It does not add the machinery to safely remove any files yet. Journaled stores are unaffected: ChunkJournal goes through journalManifest, not fileManifest.
|
This PR is being tested for SQL correctness. Please allow ~25 mins for this to complete. If this PR does not result in a SQL correctness regression, the |
|
@reltuk DOLT
|
| // into place, so a file that passes here cannot be removed by a lock-respecting | ||
| // process before the manifest naming it is committed. | ||
| // | ||
| // Nothing is expected to trip this: every path that adds a spec either renames |
There was a problem hiding this comment.
"Nothing is expected to trip this" seems inaccurate, and the rest of the paragraph makes no sense.
| // missing in between, which our own open file descriptor would not notice. | ||
| // | ||
| // Only specs |upstream| does not already carry are checked; re-verifying the | ||
| // whole set would make every manifest write a directory-sized stat storm. |
There was a problem hiding this comment.
"write a directory-sized stat storm" is too clever for its own good, please say it straight.
| } | ||
|
|
||
| // TestFileManifestUpdateAcceptsArchive asserts an archive satisfies a spec. | ||
| // A tableSpec records only the address, so both spellings must be tried. |
There was a problem hiding this comment.
"Spellings?" This docstring is vague and unclear.
| } | ||
|
|
||
| // TestFileManifestUpdateAcceptsAppendix asserts appendix specs are checked too; | ||
| // they name table files the same way the main spec list does. |
There was a problem hiding this comment.
This docstring should be written such that it doesn't require the previous test docstrings for context and avoid language like "they name table files the same way the main spec list does" which doesn't actually explain anything without additional context.
There was a problem hiding this comment.
This test is also almost identical to the previous one? Should it be a single parameterized test?
|
|
||
| // TestFileManifestUpdateSkipsExistingSpecs asserts only newly added specs are | ||
| // checked. Re-verifying the whole set on every write would cost a stat per | ||
| // table file, and those files were vouched for when they were added. |
There was a problem hiding this comment.
"vouched for" is an unusual word choice here.
| } | ||
|
|
||
| // TestFileManifestUpdateGCGenRejectsMissingTableFile asserts the GC generation | ||
| // update path is checked as well; it publishes a wholly new spec list. |
There was a problem hiding this comment.
Like the previous comment above, this docstring doesn't say much without the context of the previous docstrings. "it publishes a wholly new spec list" should use more precise language to say what's actually happening.
| touchTableFile(t, fm.dir, present) | ||
| contents := manifestContents{ | ||
| nbfVers: constants.FormatDoltString, | ||
| lock: computeAddr([]byte("lock 1")), |
There was a problem hiding this comment.
Can the test make clear why two different locks are used?
3 participants
A bare local store takes no cross-process lock while a table file is landing, so between the moment a writer opens or renames a table file and the moment it commits a manifest naming it, another process can unlink it. The writer does not notice, since its own file descriptor keeps working, and it publishes a manifest that is broken for everyone else.
Add a sanity check where we Stat the newly added files under the manifest LOCK. This allows other Dolt processes which want to safely unlink unreferenced files from the directory to do so under the same LOCK. While a write may transiently fail if an about-to-be-referenced file is deleted out from under it, it won't cause database corruption by writing a manifest which references a non-existant table file.
This PR adds the add-files sanity check. It does not add the machinery to safely remove any files yet.
Journaled stores are unaffected: ChunkJournal goes through journalManifest, not fileManifest.