Repo storage migration #443
Conversation
| if (!uint8arrays.equals(entry.bytes, otherBytes)) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
At first glance it struck me that this check wasn't needed. But I think I follow— is the idea that the block-map itself doesn't enforce how bytes map to cid, so its usage allows different bytes to be behind the same cid in two different block-maps (therefore we need to check the bytes in equals())?
There was a problem hiding this comment.
yeah that was my thinking.
Another line of thought is that we prevent this from happening in the first place (with an integrity check on set). But I kind've like BlockMap being a relatively dumb class. Almost didn't want to put add on it (which serializes some data & calculates a CID), but it made all the consuming code so much simpler that I did.
Relatedly, I've actually realized that we don't enforce integrity checks correctly on our blockstore (I guess I'd assumed that the car reader does this, but it doesn't). So have been putting some thinking today into ensuring that blockstores have correct CID mappings
| @@ -262,34 +261,15 @@ export class Repo { | |||
| latest: CID, | |||
| earliest: CID | null, | |||
| ): Promise<void> { | |||
| const commitPath = await this.storage.getCommitPath(latest, earliest) | |||
| if (commitPath === null) { | |||
| const commits = await this.storage.getCommits(latest, earliest) | |||
There was a problem hiding this comment.
👍 nice simplification in here.
There was a problem hiding this comment.
Having this test in here brings some nice peace of mind 💎 I am sure you had this in your mind while you worked on it, but the only lingering question for me is whether you feel the sqlite test brings good confidence running the same migration with postgres.
It seems to me like it does as long as the queries in the migration are all valid in postgres. In other words, I could imagine a world in which the migration totally fails on postgres for some reason and succeeds on sqlite. But it's hard for me to see a case in which the migration is incorrect on postgres but still runs without failing. So that seems pretty safe 👍
|
That was essentially my thought re: migrations, especially with relatively straightforward queries like these. I was interested to get your read on it. I bailed out of testing postgres, because to do so involved keeping around old code paths, which we can do if necessary, but it seemed like overkill. Actually, I guess one option would be to just run some repo operations, migrateDown & then migrate back up 🤔 |
Writing the migration & migration test for the repo storage changes.
The migration test is working off of a sqlite file that I generated using the old code.
Also snuck in a new method (that helped on the migration, but we want anyway):
getCommitswhich returns all data relevant to some commits in a range