[Multi-Tab] Adding Schema Migration #485

schmidt-sebastian · 2018-02-02T21:56:01Z

This performs the IndexedDB schema migration required for MultiTab:

It adds a new IndexedDB store to store instance metadata.
It adds a new IndexedDB store to store a query changelog.
It adds a field to DbMutationQueue, which we can do silently without changing the schema.

The functionality still allows for creation of schema version 1, so that we can make testing easier.

It's likely that we will want to coordinate this migration with our Garbage Collection efforts.

mikelehen

I didn't review in detail but looked over this and flagged a few things that jumped out.

Have you looked at the SQLitePersistence runMigrations() code for Android to see if we can follow a similar pattern? In particular:

I like the switch / case (with fall-through) approach it uses.
In general we should try to make the upgrade mechanism as similar as possible across platforms.
It may make sense to copy the INDEXING_SUPPORT_ENABLED flag mechanism we used there. I think we should be careful not to migrate anybody's schema until we're fully ready to launch multi-tab.

Also, Greg is working on schema upgrade infrastructure for iOS right now too, so it may be worth peeking at what he's doing.

mikelehen · 2018-02-02T22:26:59Z

packages/firestore/src/local/indexeddb_migrations.ts

+  newVersion: number
+): void {
+  assert(
+    oldVersion >= 0 || oldVersion <= 1,


Fixed (combined the asserts into one)

mikelehen · 2018-02-02T22:27:09Z

packages/firestore/src/local/indexeddb_migrations.ts

+    'Unexpected upgrade from version ' + oldVersion
+  );
+  assert(
+    newVersion >= 1 || newVersion <= 2,


mikelehen · 2018-02-02T22:30:05Z

packages/firestore/src/local/indexeddb_migrations.ts

+  );
+
+  const createV1 = newVersion >= 1 && oldVersion <= 1;
+  const dropV1 = oldVersion >= 1;


Should this be <= ? FWIW, I'm not sure these intermediate variables are helping here. I keep looking back and forth between the if-checks...

mikelehen · 2018-02-02T22:36:56Z

packages/firestore/src/local/indexeddb_migrations.ts

+export function createOrUpgradeDb(
+  db: IDBDatabase,
+  oldVersion: number,
+  newVersion: number


Having newVersion be a parameter is super confusing to me. Are you intending to support downgrades (I think we should try hard to avoid that!)?

I am not at all planning on adding the ability to downgrade. I renamed the arguments to fromVersion and toVersion and added an assert that one must be lower than the other.

mikelehen · 2018-02-02T22:39:48Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -131,7 +87,14 @@ export class DbMutationQueue {
     * After sending this token, earlier tokens may not be used anymore so
     * only a single stream token is retained.
     */
-    public lastStreamToken: string
+    public lastStreamToken: string,
+    /**


I added newlines here and everywhere else where they were missing. I also tried to make this semi-valid JSDoc along with it (I still don't think it's valid as we would probably have to move the JSDoc comments to the function header).

Note that prettier stripped the newlines again.

Oh, sorry. I should have known this was prettier...

Thanks for your efforts to make things JSDoc friendly, though 1) I don't consider this a goal [we're never going to generate docs from our internal implementation files], and 2) I don't think that needed to go in this PR... In this case it's probably fine, but it could be distracting if somebody was revisiting this PR later (to track down a bug or port to another platform or whatever).

Even if it is not for external documentation, it could show up in code completion when done properly - whereas right not it just shows up as a warning due do the /** JSDoc */ marks.

I agree that this is mostly a non-issue though and happily pressed some buttons to revert this.

mikelehen · 2018-02-02T22:41:50Z

packages/firestore/src/local/indexeddb_schema.ts

+     * than or equal to this value are considered to have been acknowledged by
+     * the server.
+     */
+    public highestPendingBatchId: number


This only exists so that we can leave acknowledged mutations in the queue for the sake of other tabs being able to read them, right? I think it's worth calling that out here. Also, are we going to port this to other platforms? If not, please add a "// PORTING NOTE:" comment.

I updated the outdated comment. This is to reduce the number of scans we have to do in the mutation batch. Right now, to get the highest batch ID, we do an full scan at startup. I don't want to do this every time we add a mutation from a secondary tab.

mikelehen · 2018-02-02T22:46:47Z

packages/firestore/src/local/indexeddb_migrations.ts

+// https://github.com/Microsoft/TypeScript/issues/14322
+type KeyPath = any; // tslint:disable-line:no-any
+
+function createCache(db: IDBDatabase): void {


FWIW, "cache" is non-obvious, so I would at least add a comment.

// Create IndexedDB stores for our query / remote document caches.

I renamed this and split this up to match the names in Android: createQueryCache() and createRemoteDocumentCache()

mikelehen

couple more minor comments after a second look.

mikelehen · 2018-02-03T17:48:24Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -404,6 +367,7 @@ export class DbTargetDocument {
     * The targetId identifying a target.
     */
    public targetId: TargetId,
+    public snapshotVersion: number,


comment? (especially call out if we are doing any cheating with fake snapshot versions or whatever...)

Also I think we want to do some design review (with Gil at least) before moving forward with this (unless it's living in a non-master branch or something).

I talked to Gil and Greg about this yesterday. Gil favors a separate ChangeLog, as primary key updates are pretty expensive. I updated the Design Doc with this yesterday and added a new table to store this changelog. The nice thing about this is that now all changes in the schema are additive and we don't need to drop the Query & Remote Document Cache anymore.

mikelehen · 2018-02-03T17:50:15Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -455,6 +419,20 @@ export class DbTargetGlobal {
  ) {}
 }

+export type DbInstanceKey = [string, string];


comments?

Also, "Instance" feels too generic. Perhaps "ClientInstance" (and comments should explain the multi-tab scenario)

No longer used.

schmidt-sebastian · 2018-02-06T18:43:04Z

I didn't review in detail but looked over this and flagged a few things that jumped out.

Have you looked at the SQLitePersistence runMigrations() code for Android to see if we can follow a similar pattern? In particular:

I like the switch / case (with fall-through) approach it uses.
In general we should try to make the upgrade mechanism as similar as possible across platforms.
It may make sense to copy the INDEXING_SUPPORT_ENABLED flag mechanism we used there. I think we should be careful not to migrate anybody's schema until we're fully ready to launch multi-tab.
Also, Greg is working on schema upgrade infrastructure for iOS right now too, so it may be worth peeking at what he's doing.

This PR is now ready for review.

I looked at the iOS PR, which originally inspired the creation of a separate file to run the migrations. I like the Android way better to do this in place and merged my change into indexeddb_schema.

Both Android and iOS use the switch-statement with fallthrough. This doesn't allow us to specify a range of versions to update (from 0 to 1 for example) and I would like to use that for testing of the actual migration process.

I did not make this flag controlled as this PR is meant to be merged into the firestore-multi-tab branch. We should decide later on whether this feature will be optional and how we want to deal with optional features in the schema. Even if multi-tab is optional, it might make sense to add it to the schema but not write any data into the multi-tab specific stores.

schmidt-sebastian · 2018-02-06T22:22:27Z

BTW, I updated this PR to remove the "userId" from the InstanceMetadata store. The rest of the multi-tab work assumes that only one user is logged in at a time, and there is no reason to keep state around for users that are no longer active. I haven't updated the doc yet and wanted have this sanity checked first.

mikelehen

Thanks. I like this iteration a lot. Some feedback but nothing major. FWIW- My suggestion to add the flag was so that we could merge this to master before multi-tab is done / shipped. I am hoping we don't need to make multi-tab optional long-term.

In any case if you're intending to work in a branch, then we don't need the flag guard. Just be wary of conflicts...

mikelehen · 2018-02-06T20:33:14Z

packages/firestore/src/local/indexeddb_schema.ts

-    DbTargetDocument.documentTargetsKeyPath,
-    { unique: true }
+/**
+ * Performs database creation and schema migrations up to schema version 2.


I wouldn't bake the current schema version into this comment (it'll just get stale). And I think you should explain why toVersion is a parameter. Something like:

Performs database creation and schema migrations up to SCHEMA_VERSION. Note that in production, toVersion will always be SCHEMA_VERSION but for testing we allow "partial upgrades" to earlier versions (but toVersion must still be >= fromVersion).

I changed the comment as suggested. The schema version specific comment is not just above the assert, which has to be changed as we add new schema version. I thought about adding named constant for each schema version, but I am not sure how much value that adds.

mikelehen · 2018-02-06T20:44:29Z

packages/firestore/src/local/indexeddb_schema.ts

+    createMutationQueue(db);
+    createQueryCache(db);
+    createRemoteDocumentCache(db);
+  }


If you're not doing the switch / case pattern, I think you should still organize similarly (in particular put version 1 stuff before version 2), and I think we need to have a simple pattern that makes it obvious how to add code whenever we bump SCHEMA_VERSION... As-is, the toVersion === 2 check will need to be changed when we add version 3. In general, this method should be append-only as we add versions. We can't really change version migration stuff once it's shipped.

Perhaps:

if (fromVersion < 1 && toVersion >= 1) { // version 1 stuff... } if (fromVersion < 2 && toVersion >= 2) { // version 2 stuff... } ...

Or you could do a for loop:

for (let version = fromVersion+1; version <= toVersion; version++) { if (version === 1) { // version 1 stuff } else if (version === 2) { // version 2 stuff } }

I used code snippet version 1.

mikelehen · 2018-02-06T21:25:16Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -131,7 +87,14 @@ export class DbMutationQueue {
     * After sending this token, earlier tokens may not be used anymore so
     * only a single stream token is retained.
     */
-    public lastStreamToken: string
+    public lastStreamToken: string,
+    /**


Oh, sorry. I should have known this was prettier...

Thanks for your efforts to make things JSDoc friendly, though 1) I don't consider this a goal [we're never going to generate docs from our internal implementation files], and 2) I don't think that needed to go in this PR... In this case it's probably fine, but it could be distracting if somebody was revisiting this PR later (to track down a bug or port to another platform or whatever).

mikelehen · 2018-02-06T21:29:21Z

packages/firestore/src/local/indexeddb_schema.ts

-     * by the server. All MutationBatches in this queue with batchIds less
-     * than or equal to this value are considered to have been acknowledged by
-     * the server.
+     * @param lastAcknowledgedBatchId - An identifier for the highest numbered


FWIW, the reason we have these detailed comments here is more because these are fields on the class than because we like to comment all of our parameters. So while @param is probably correct, it may make it less obvious that these are also public fields on the class... So I'm ambivalent about actually using @param here. I don't care enough to ask you to undo it, but I think I'd prefer we not go migrate the whole codebase or anything...

It only took seconds to undo, so it's undone.

mikelehen · 2018-02-06T21:32:28Z

packages/firestore/src/local/indexeddb_schema.ts

+     * batch in the mutation queue. This allows for efficient insert of new
+     * batches without relying on in-memory state.
+     *
+     * PORTING NOTE: iOS and Android clients keep this value in-memory.


Don't they track it as nextBatchId? This is a clearer name I think. Could we do the same?

I changed this to nextBatchId (which is the internal name used in the clients), but then ultimately removed it as pointed out above.

mikelehen · 2018-02-06T22:34:23Z

packages/firestore/test/unit/local/schema_migration.test.ts

+    createOrUpgradeDb(db, event.oldVersion, targetVersion);
+  };
+
+  return deferred.promise;


Could you use SimpleDb.openOrCreate() ? [I'm not 100% sure]

SimpleDb doesn't expose the version and the list of object stores.

mikelehen · 2018-02-06T22:37:38Z

packages/firestore/test/unit/local/schema_migration.test.ts

+  return objectStores;
+}
+
+// Sorting these arrays directly should not affect the functionality of the SDK.


But it is ugly! Why not just do V1_STORES.slice().sort() where you need them sorted?

No longer used.

mikelehen · 2018-02-06T22:40:53Z

packages/firestore/test/unit/local/schema_migration.test.ts

+  it('can install schema version 1', () => {
+    return initDb(1).then(db => {
+      expect(db.version).to.be.equal(1);
+      expect(getAllObjectStores(db)).to.deep.equal(V1_STORES);


can you use to.have.members(...) and not need to ensure they're sorted the same?

mikelehen · 2018-02-06T22:44:58Z

packages/firestore/test/unit/local/schema_migration.test.ts

+V1_STORES.sort();
+ALL_STORES.sort();
+
+describe('SchemaMigration', () => {


What is SchemaMigration? :-) Maybe 'Schema Migration: createOrUpgradeDb()' (since createOrUpgradeDb is the method you're testing here).

mikelehen · 2018-02-06T22:46:21Z

packages/firestore/src/local/indexeddb_schema.ts

+  toVersion: number
+): void {
+  assert(
+    fromVersion < toVersion && fromVersion >= 0 && toVersion <= 2,


2 => SCHEMA_VERSION ?

I actually changed SCHEMA_VERSION to DEFAULT_SCHEMA_VERSION and reset the value back to 1. This way, I can merge this code to master, and Greg can build the GC migration on top of this.

googlebot · 2018-02-07T19:33:22Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

schmidt-sebastian · 2018-02-07T19:35:16Z

This is ready for review again. I have updated the base branch to 'master`, but used the SCHEMA_VERSION constant to make sure that we don't run this upgrade yet.

mikelehen

Basically LGTM, but I'm wary of checking in multi-tab schema work in master if we're doing the feature development in a different branch. It seems like this is at minimum confusing, and could potentially create headaches later (e.g. when you iterate on the schema or if we need to ship a non-multi-tab schema change before the multi-tab ones).

Have you considered just doing the multi-tab work in master? In that case I think this PR would add a global MULTITAB_ENABLED flag for tests to flip and then set SCHEMA_VERSION and ALL_STORES as appropriate based on it, and the rest of our code would similarly trigger off this flag (e.g. create a no-op SharedClientState object instead of the LocalStorage one). FWIW, this is the path we started down for indexing on Android.

I can probably be talked into approving this PR but I'd like to at least have the conversation.

The other option would be to separate the schema-migration part of this PR from the multi-tab part, but I think then we end up with no actual "schema migration" testing. :-)

mikelehen · 2018-02-08T23:38:33Z

packages/firestore/test/unit/local/schema_migration.test.ts

+  })
+    .then(db => {
+      fn(db);
+      return db;


I think you can just db.close() here and drop a then...

schmidt-sebastian · 2018-02-09T02:28:32Z

I want to keep the multi-tab work in a separate branch to avoid unnecessary churn on the master branch in case I need to revisit some of my design decisions. The reason I want(ed) to merge this into Master is that Greg is going to write schema migration code pretty soon and it would be nice if he could built it on top of this PR (which might bump multi-tab to version 3).

There are many other ways to guarantee that @gsoltis doesn't do duplicate work, and I'll just ask him to pull in this PR and merge it to Master with multi-tab removed and GC added.

TL/DR: Rebased to firestore-multi-tab and made Schema Version 2 the default in this branch.

mikelehen

One nit left...

mikelehen · 2018-02-09T17:29:02Z

packages/firestore/src/local/indexeddb_schema.ts

- * clients with the schema version 2.
- */
-export const ALL_STORES = [...V1_STORES, ...V2_STORES];
+export const DEFAULT_STORES = [...V1_STORES, ...V2_STORES];


Put back to ALL_STORES?

schmidt-sebastian added 3 commits February 2, 2018 12:00

Adding Schema Migration

eb44929

Pseudocode for Schema Migration

07e44e7

[AUTOMATED]: Prettier Code Styling

3e1053f

schmidt-sebastian requested review from mikelehen and wilhuff as code owners February 2, 2018 21:56

google-oss-bot added the needs-triage label Feb 2, 2018

mikelehen reviewed Feb 2, 2018

View reviewed changes

mikelehen reviewed Feb 3, 2018

View reviewed changes

IndexedDb Schema Migration

5e84782

schmidt-sebastian force-pushed the multitab-schemamigration branch from 4f47bf3 to 5e84782 Compare February 6, 2018 18:43

schmidt-sebastian changed the title ~~Multitab schemamigration~~ [Multi-Tab] Adding Schema Migration Feb 6, 2018

schmidt-sebastian assigned mikelehen Feb 6, 2018

schmidt-sebastian changed the base branch from master to firestore-multi-tab February 6, 2018 18:48

Lint cleanup

fd50301

schmidt-sebastian force-pushed the multitab-schemamigration branch from 648c0e6 to fd50301 Compare February 6, 2018 19:50

schmidt-sebastian added 3 commits February 6, 2018 13:08

Removing unused import

be3b463

Removing user ID from instance row

de237c5

[AUTOMATED]: Prettier Code Styling

53e56b5

mikelehen suggested changes Feb 6, 2018

View reviewed changes

schmidt-sebastian added 5 commits February 7, 2018 11:28

Review comments

7b3bedb

Merge branch 'master' into multitab-schemamigration

7b97c0a

Lint fixes

c176eff

Review

9154aad

[AUTOMATED]: Prettier Code Styling

6b072ff

schmidt-sebastian requested a review from jshcrowthe as a code owner February 7, 2018 19:33

schmidt-sebastian changed the base branch from firestore-multi-tab to master February 7, 2018 19:34

Fixing the tests

662365f

schmidt-sebastian force-pushed the multitab-schemamigration branch from 3537fe2 to 662365f Compare February 7, 2018 19:56

schmidt-sebastian added 4 commits February 7, 2018 17:51

Closing the Database in the Schema tests

dd21376

[AUTOMATED]: Prettier Code Styling

5e54b3a

Changing test helper to close the DB

cb81df8

[AUTOMATED]: Prettier Code Styling

7991970

mikelehen reviewed Feb 9, 2018

View reviewed changes

schmidt-sebastian added 2 commits February 8, 2018 18:19

Making v2 the default version

2c6d6e1

[AUTOMATED]: Prettier Code Styling

f16bb4f

schmidt-sebastian changed the base branch from master to firestore-multi-tab February 9, 2018 02:22

schmidt-sebastian added 2 commits February 8, 2018 18:24

Addressing comment

038c159

[AUTOMATED]: Prettier Code Styling

b70303c

mikelehen approved these changes Feb 9, 2018

View reviewed changes

Renamed to ALL_STORES

34ab25a

schmidt-sebastian merged commit 9c61e81 into firestore-multi-tab Feb 9, 2018

schmidt-sebastian deleted the multitab-schemamigration branch March 26, 2018 21:33

firebase locked and limited conversation to collaborators Oct 23, 2019

[Multi-Tab] Adding Schema Migration #485

[Multi-Tab] Adding Schema Migration #485

Uh oh!

Conversation

schmidt-sebastian commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian commented Feb 6, 2018

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

schmidt-sebastian commented Feb 2, 2018 •

edited

Loading

schmidt-sebastian commented Feb 6, 2018 •

edited

Loading