Migrate SQLite backing store to LevelDB backing store for Indexeddb.

• Component: New Bugs
• Status: RESOLVED FIXED

[Greg Simon]

Migrate SQLite backing store to LevelDB backing store for Indexeddb.

[Greg Simon]

Created attachment 97405 [details]
Patch

[Hans Wennborg]

Comment on attachment 97405 [details]
Patch

It's great to see this uploaded!

View in context: https://bugs.webkit.org/attachment.cgi?id=97405&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:82
>  

Some of this patch overlaps with the patch in Bug 62382. Let's get that one landed first.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:131
> +    String toUniqueIdentifier = securityOrigin->databaseIdentifier() + "@" + name + String::format("@%d", LevelDBBackingStore);

We have code for creating these identifiers in a couple of places now. May be worth moving it out to a function or something.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:161
> +        return false;

Are we migrating the whole origin or just one database?

I think we should do the whole origin. That way we also don't need to involve an IDBDatabaseBackendImpl, we can just dig out all the database names from the IDBBackingStore.

In fact, I'd love to see methods like this..

IDBFactoryBackendImpl::migrate(SecurityOrigin, dataDir, ...) - this guy opens and/or creates backingstores in the right places

static migrateBackingStore(IDBBackingStore* from, IDBBackingStore *to) - this guy migrates an entire backing store from another (calling out to helper functions below)
static migrateDatabase (IDBBackingStore* from, IDBBackingStore *to, String databaseName) - this migrates a single database
(maybe a migrateObjectStore function too)

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:196
> +            RefPtr<IDBBackingStore::Cursor> cursor = fromBackingStore->openObjectStoreCursor(fromDatabaseBackend->id(), fromObjStoreIds[i], 0, IDBCursor::NEXT);

I would have used the ForEachObjectStoreRecord() method, but I'm not sure that matters.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:219
> +                if (!toBackingStore->createIndex(toDatabaseId, assignedObjectStoreId, name, keyPath, idxUnique[j], indexId))

createIndex doesn't populate the index (sorry, the name isn't that clear), so we'll need to do that too, and the layout test should test for it.
Maybe we can use openIndexKeyCursor(), or we can add a function similar to forEachObjectStoreRecord()

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1006
> +    return false;

If we just want to check for file existence, we should simplify this to return fileExists(path) :)

[Greg Simon]

(In reply to comment #2)
> (From update of attachment 97405 [details])
> It's great to see this uploaded!
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=97405&action=review
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:82
> >  
> 
> Some of this patch overlaps with the patch in Bug 62382. Let's get that one landed first.
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:131
> > +    String toUniqueIdentifier = securityOrigin->databaseIdentifier() + "@" + name + String::format("@%d", LevelDBBackingStore);
> 
> We have code for creating these identifiers in a couple of places now. May be worth moving it out to a function or something.
Yes -- I'll consolidate.

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:161
> > +        return false;
> 
> Are we migrating the whole origin or just one database?

Just single database on-demand (see comment below).

> I think we should do the whole origin. That way we also don't need to involve an IDBDatabaseBackendImpl, we can just dig out all the database names from the IDBBackingStore.
> 
> In fact, I'd love to see methods like this..
> 
> IDBFactoryBackendImpl::migrate(SecurityOrigin, dataDir, ...) - this guy opens and/or creates backingstores in the right places
> 
> static migrateBackingStore(IDBBackingStore* from, IDBBackingStore *to) - this guy migrates an entire backing store from another (calling out to helper functions below)
> static migrateDatabase (IDBBackingStore* from, IDBBackingStore *to, String databaseName) - this migrates a single database
> (maybe a migrateObjectStore function too)

This is an attempt to make the code as small as possible so it is an on-demand migration. I considered making it more generic but it seems the Indexeddb code suffers from this too much already and it is unlikely we'll be migrating again.



> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:196
> > +            RefPtr<IDBBackingStore::Cursor> cursor = fromBackingStore->openObjectStoreCursor(fromDatabaseBackend->id(), fromObjStoreIds[i], 0, IDBCursor::NEXT);
> 
> I would have used the ForEachObjectStoreRecord() method, but I'm not sure that matters.

I tried this first but the code was more verbose than the attached patch.


> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:219
> > +                if (!toBackingStore->createIndex(toDatabaseId, assignedObjectStoreId, name, keyPath, idxUnique[j], indexId))
> 
> createIndex doesn't populate the index (sorry, the name isn't that clear), so we'll need to do that too, and the layout test should test for it.
> Maybe we can use openIndexKeyCursor(), or we can add a function similar to forEachObjectStoreRecord()

Hmm... ok I'll dig into this then.

> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1006
> > +    return false;
> 
> If we just want to check for file existence, we should simplify this to return fileExists(path) :)
Yes! -- I'll change.




.. also, missing from this patch is SQLite deletion but I want to do that separately. When this patch lands migration won't automatically start happening yet until the DefaultBackingStore is changed.

[Eric Seidel (no email)]

Comment on attachment 97405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97405&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:127
>  bool IDBFactoryBackendImpl::migrate(const String& name, SecurityOrigin* securityOrigin, const String& dataDir, int64_t maximumSize)

Seems like we could have broken this gigantic function into smaller (static/inline) helpers to help readability.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:183
> +        if (!toObjStoreNames.contains(fromObjStoreNames[i])) {

Early continue?

[Greg Simon]

Created attachment 97872 [details]
Patch

[Greg Simon]

(In reply to comment #4)
> (From update of attachment 97405 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97405&action=review
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:127
> >  bool IDBFactoryBackendImpl::migrate(const String& name, SecurityOrigin* securityOrigin, const String& dataDir, int64_t maximumSize)
> 
> Seems like we could have broken this gigantic function into smaller (static/inline) helpers to help readability.

Correct -- I broke up the migration code itself into a static function.

> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:183
> > +        if (!toObjStoreNames.contains(fromObjStoreNames[i])) {
> 
> Early continue?
It is correct as written -- we are limiting our "merge" migration objectstores as a whole. If the object store exists at the target we move onto the next one.

[Hans Wennborg]

Comment on attachment 97872 [details]
Patch

Hi Greg,

Thanks again for working on this. It's a tricky area.

My main worry is that the patch confuses backing stores and databases, and fileIdentifiers and uniqueIdentifiers. See comments inline, and ping me if there's anything I can clarify.


View in context: https://bugs.webkit.org/attachment.cgi?id=97872&action=review

> LayoutTests/storage/indexeddb/migrate-basics.html:154
> +    shouldBeTrue("(window.store.index('ExampleIndex') != undefined)");

we also need to check that the index actually contains anything

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95
> +    IDBBackingStoreMap::iterator it2 = m_backingStoreMap.find(uniqueIdentifier);

This is a regression of what I fixed in Bug 62382. It is important that m_backingStoreMap is keyed *per origin* (fileIdentifier) rather than per database (uniqueIdentifier).

The m_backingStoreMap's purpose is to cache open backing stores, so that we only keep a single connection to each backing store. If a LevelDB database is opened more than once simultaneously, it will cause corruption.

This comment applies throughout the patch.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:104
> +            // LayoutTests: SQLite backing store may not exist on disk but may exist in cache.

Nice catch; I hadn't thought of this.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:117
> +            backingStore = IDBSQLiteBackingStore::open(securityOrigin.get(), dataDir, maximumSize, uniqueIdentifier, this);

Again, this seems to undo the patch from Bug 62382. Is this deliberate, or a merge thing?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:120
> +            backingStore = IDBLevelDBBackingStore::open(securityOrigin.get(), dataDir, maximumSize, uniqueIdentifier, this);

Ditto.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:148
> +        if (!toObjStoreNames.contains(fromObjStoreNames[i])) {

I think what Eric meant by his "early continue" comment is that you could turn this into

if (toObjStoreNames.contains(fromObjStoreNames[i]))
    continue;

// do migration here...

I.e. the functionality is the same, it's just a style thing, reducing the nesting of blocks.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:159
> +                keyPath = true;

Maybe "bool hasKeyPath = !fromKeyPaths[i].isEmpty();" is simpler. Also, I'm not sure what this is used for, see comment below.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:181
> +                ASSERT(keyPath);

If I understand correctly, this asserts that the object store has a key path. I don't see why that would hold? It's prefectly ok for an object store not to have a key path, and have indexes.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:184
> +                    break;

maybe return false?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:208
> +    toDatabaseExistedBefore = IDBLevelDBBackingStore::backingStoreExists(securityOrigin, dataDir);

this seems to check that the "to origin" exists, which isn't the same as that the "to database" exists in that origin.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:232
> +    // Open "To" database. First find out if it already exists -- this will determine if

the "First find out if it already exists" comment doesn't seem to apply here.. you're just opening the backing store?

[Greg Simon]

(In reply to comment #7)
> (From update of attachment 97872 [details])
> Hi Greg,
> 
> Thanks again for working on this. It's a tricky area.
> 
> My main worry is that the patch confuses backing stores and databases, and fileIdentifiers and uniqueIdentifiers. See comments inline, and ping me if there's anything I can clarify.

The separation of backing store and backends was present before this patch. I believe this is the correct way to do it because the two are related to each other (the backend has a pointer to the backingstore, which are both going to be different depending on leveldb or sqlite. Ideally there would not be a different between backing store and backend. 

> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=97872&action=review
> 
> > LayoutTests/storage/indexeddb/migrate-basics.html:154
> > +    shouldBeTrue("(window.store.index('ExampleIndex') != undefined)");
> 
> we also need to check that the index actually contains anything
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95
> > +    IDBBackingStoreMap::iterator it2 = m_backingStoreMap.find(uniqueIdentifier);
> 
> This is a regression of what I fixed in Bug 62382. It is important that m_backingStoreMap is keyed *per origin* (fileIdentifier) rather than per database (uniqueIdentifier).
> 
> The m_backingStoreMap's purpose is to cache open backing stores, so that we only keep a single connection to each backing store. If a LevelDB database is opened more than once simultaneously, it will cause corruption.

The backend keeps a pointer to a backing store so they are related even though they are separate C++ objects. This is an important fact in the migration code which is why I keep them together.

> 
> This comment applies throughout the patch.
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:104
> > +            // LayoutTests: SQLite backing store may not exist on disk but may exist in cache.
> 
> Nice catch; I hadn't thought of this.
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:117
> > +            backingStore = IDBSQLiteBackingStore::open(securityOrigin.get(), dataDir, maximumSize, uniqueIdentifier, this);
> 
> Again, this seems to undo the patch from Bug 62382. Is this deliberate, or a merge thing?
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:120
> > +            backingStore = IDBLevelDBBackingStore::open(securityOrigin.get(), dataDir, maximumSize, uniqueIdentifier, this);
> 
> Ditto.
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:148
> > +        if (!toObjStoreNames.contains(fromObjStoreNames[i])) {
> 
> I think what Eric meant by his "early continue" comment is that you could turn this into
> 
> if (toObjStoreNames.contains(fromObjStoreNames[i]))
>     continue;

Ah yes I missed that.

> 
> // do migration here...
> 
> I.e. the functionality is the same, it's just a style thing, reducing the nesting of blocks.
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:159
> > +                keyPath = true;
> 
> Maybe "bool hasKeyPath = !fromKeyPaths[i].isEmpty();" is simpler. Also, I'm not sure what this is used for, see comment below.
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:181
> > +                ASSERT(keyPath);
> 
> If I understand correctly, this asserts that the object store has a key path. I don't see why that would hold? It's prefectly ok for an object store not to have a key path, and have indexes.

I guess am misunderstanding the spec then because it mentions indexes using a keypath. I can remove the assertion.

> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:184
> > +                    break;
> 
> maybe return false?
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:208
> > +    toDatabaseExistedBefore = IDBLevelDBBackingStore::backingStoreExists(securityOrigin, dataDir);
> 
> this seems to check that the "to origin" exists, which isn't the same as that the "to database" exists in that origin.

Hmm. Yes, right -- the name needs to be in there too.

The purpose of this extra check is because if you call extractMetaData on an emptydatabase it seems to corrupt the store and you can't populate it anymore. OTOH, calling setDatabaseMetaData more than once increments the databaseId each time. THis was not a problem until we decided we would do partial/merge migrations. I suspect we should *not* do merge/partial migrations, which was the logic captured in patch 61000.
 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:232
> > +    // Open "To" database. First find out if it already exists -- this will determine if
> 
> the "First find out if it already exists" comment doesn't seem to apply here.. you're just opening the backing store?

See above comment about extractMetaData.

[Hans Wennborg]

(In reply to comment #8)
> > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95
> > > +    IDBBackingStoreMap::iterator it2 = m_backingStoreMap.find(uniqueIdentifier);
> > 
> > This is a regression of what I fixed in Bug 62382. It is important that m_backingStoreMap is keyed *per origin* (fileIdentifier) rather than per database (uniqueIdentifier).
> > 
> > The m_backingStoreMap's purpose is to cache open backing stores, so that we only keep a single connection to each backing store. If a LevelDB database is opened more than once simultaneously, it will cause corruption.
> 
> The backend keeps a pointer to a backing store so they are related even though they are separate C++ objects. This is an important fact in the migration code which is why I keep them together.

Yes, they are related, but it's not a 1-1 relationship, it's more like 1-n since there can be n databases for each backing store.

I see that it's important to keep backing stores with LevelDB vs SQLite apart, which we can do by making the store type part of the fileIdentifier.

It's essential that we keep track of which backing stores are open, and make sure we don't open the same one twice. That's what m_backingStoreMap, keyed with fileIdentifier, does.



> > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:159
> > > +                keyPath = true;
> > 
> > Maybe "bool hasKeyPath = !fromKeyPaths[i].isEmpty();" is simpler. Also, I'm not sure what this is used for, see comment below.
> > 
> > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:181
> > > +                ASSERT(keyPath);
> > 
> > If I understand correctly, this asserts that the object store has a key path. I don't see why that would hold? It's prefectly ok for an object store not to have a key path, and have indexes.
> 
> I guess am misunderstanding the spec then because it mentions indexes using a key ath. I can remove the assertion.

An index always has a key path. An object store may or may not have one. The assert seems to check for an object store key path.

[Greg Simon]

Created attachment 98073 [details]
Patch

[Hans Wennborg]

Comment on attachment 98073 [details]
Patch

This looks great, I only have nits left :)


View in context: https://bugs.webkit.org/attachment.cgi?id=98073&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:49
> +    return securityOrigin->databaseIdentifier() + "@" + name + String::format("@%d", type);

i think computeUniqueIdentifier could re-use computeFileIdentifier... something like

return computeFileIdentifier(...) + "@" + name

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:148
> +    // Migrate objectStores. Check to see if the object store already exists in the target

period.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:158
> +        int64_t assignedObjectStoreId=-1;

space around '=' operator

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:167
> +            keyPath = true;

I don't see keyPath actually being used anywhere.. probably a leftover from earlier version of the patch?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:176
> +                    break;

return false?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:191
> +                break;

return false?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:193
> +            IDBObjectStoreBackendImpl::populateIndex(*toBackingStore, toDatabaseId, assignedObjectStoreId, indexId, fromKeyPaths[i]);

populateIndex returns a bool on failure, so let's check it

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:252
> +    int64_t toDatabaseId=-1;

space around '='

[Greg Simon]

Created attachment 98749 [details]
Patch

[Hans Wennborg]

(In reply to comment #12)
> Created an attachment (id=98749) [details]
> Patch

LGTM

[Hans Wennborg]

Dimitri: I think Greg intended to ping you about this. Can you take a look?

[WebKit Review Bot]

Comment on attachment 98749 [details]
Patch

Clearing flags on attachment: 98749

Committed r89948: <http://trac.webkit.org/changeset/89948>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.