Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduced useInMemoryDatabases configuration option #294

Closed

Conversation

ThomasGreiner
Copy link

This PR introduces a configuration option that stores all databases in memory. While it is already possible to use escapeDatabaseName to make it work for individual databases, sysdb will still be file-based. Our main use case for it is for running tests to avoid having to do unnecessary file cleanup afterwards.

I also noticed that #278 mentions that you intend to make it easier for Node users to use in-memory databases which this change might help with.

If it's a concern to you that this option would be exposed to non-Node environments, I could clarify this in the comments/README or implement it differently. For instance, extending escapeDatabaseName() to also apply to sysdb or adding a separate configuration option to escape the file name of sysdb.

@brettz9
Copy link
Collaborator

brettz9 commented Mar 24, 2017

This is a good beginning, thank you.

I'm not personally really concerned about the issue with non-Node environments, as it is not overloading a legitimate database name, but relying on config (and even if the user were deploying polyglot browser/Node code, they could detect the environment to turn on or off the config).

A few concerns I think we'd need to have resolved/addressed first, however:

  1. The openDatabase call within open() is the most critical to allow for ":memory:" (or ""), so I'd think the in-memory config ought to change the behavior there as well as with the sysdb without requiring separate config to override escapeDatabaseName. (Unless the same indexedDB instance should not be used to open other databases.)

  2. We need to add code (and docs) to avoid adding in-memory databases to dbVersions (or otherwise handle). That table is used by IDBFactory.prototype.webkitGetDatabaseNames for listing names of existing databases (and its records are deleted by deleteDatabase). Multiple concurrent in-memory databases are supported by node-sqlite3 but as they would all share the same name, this would cause problems (e.g., deleting one would delete a record of all) unless some additional column were added to the dbVersions table to indicate a unique ID. If the latter were attempted, this would, however, also necessitate removing the records from the dbVersions table when the in-memory databases were closed, as they are not intended to be persisted and there would be problems given that the versions table would otherwise preserve a record of them even though they no longer exist (this can actually already be a problem if a user deletes a database file from the file system without updating the dbVersions table, but that is pretty unavoidable). While I am not sure it would be worth the work to support them being added into this table (especially considering the table itself is part of a temporary database--which incidentally we might wish to auto-delete if no references continued inside of it), besides needing to avoid adding them, I do think it would be useful to expose some means of forcing these in-memory databases to close. (See the next item.)

  3. Since more than one ":memory:" is possible, we need to consider what happens if, when the config is set, "" or ":memory:" is sent to deleteDatabase. Even though I know deletion is of less concern given that it should be auto-cleaned, the user might be unit testing or using polyglot browser/Node code and wish to delete them manually, so I think it makes the most sense to have deleteDatabase close the database. However, we have to consider how a particular instance of ":memory:" is to be marked for deletion.

For any given non-shared in-memory WebSQL database, one can call nodeWebSQLDBInstance._db.close() to destroy the database entirely; shared ones will do this after all connections are closed. (Incidentally, we really need to be calling this for all databases under closing conditions, including for the sake of catching errors, but we can save that for another issue, noting that we might not want to actually call close for in-memory databases unless deleteDatabase is called.)

  1. We should have (Mocha) unit tests using these db types

Besides the above, I'd ideally like for the same PR to also handle the following related and thankfully pretty easy, situations:

  1. Besides ":memory:", I see now per https://sqlite.org/inmemorydb.html and https://sqlite.org/uri.html that file::memory:<optional query params><optional hash> is another possibility for SQLite (and presumably node-websql/node-sqlite3) for which it shouldn't be hard to add support at the same time (we'd need a regular expression /^file::memory:/ or such to check for it instead of just a string identity check). I'd like for us to add config (the same config renamed or another boolean) to support this.

  2. Similarly, I'd like us to offer empty string support to support temporary file creation in Node (though technically the so-called temporary file creation option may actually sometimes only work in memory). Currently escapeDatabaseName escapes empty strings to allow the user to use the empty string as a database name, but with config on, we ought to optionally support an unescaped empty string as with ":memory:". This could be implemented with escapeDatabaseName by returning the empty string as you did with ":memory:" when the right config is set.

3. While we're on the topic of Node file databases, it'd be nice to knock off the to-do to have Node optionally delete the database file when deleteDatabase is called (currently, it just empties the databases contents as the browser is confined to doing).

@brettz9 brettz9 mentioned this pull request Mar 24, 2017
@brettz9
Copy link
Collaborator

brettz9 commented Apr 9, 2017

Ok, in master, I've built on your work to add the following support in addition to :memory::

  1. Support file::memory:[optionalQueryParams][#optionalHash]
  2. Support the empty string for temporary file-based databases

In order to add this support, we look for the value of CFG.memoryDatabase which is set to one of the above.

The user can thus still supply a name (which gets made available to deleteDatabase and webkitGetDatabaseNames. For deleteDatabase, an in-memory database will be closed (destroyed).

And if one is lazy, one can set CFG.autoName to true and just supply the empty string to open calls (normally the empty string is an allowed value).

I expect the latter two features will work but I have not yet added unit tests for them. If you wanted to play around with them and add any such tests (and if necessary any PRs to fix), that would be most appreciated as I would like to focus on getting our remaining failing W3C-tests up to speed as well as debug in the browsers (esp. Safari).

FWIW, I have also added the following somewhat related features:

  1. cacheDatabaseInstances (this is automatically and by necessity leveraged for in-memory databases but it can also be used for regular database instances)
  2. Allow the .sqlite extension to be added conditionally including on __sysdb__.
  3. A default-on config escapeNFDForDatabaseNames to ensure database names preserve all Unicode information when saved to a file system that normalizes file names with normalization form NFD (the Mac).
  4. deleteDatabaseFiles on-by-default to delete the database files rather than emptying them upon deleteDatabase

@brettz9 brettz9 closed this Apr 9, 2017
@ThomasGreiner
Copy link
Author

Thanks for handling this and sorry about the lack of feedback. I'm still trying to wrap my head around how those things are related to each other and how to use them effectively so I doubt that I'd be of much help anytime soon, unfortunately.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 10, 2017

They're not really related to each other outside of the fact that they relate to Node and db config. The main concern I have is with CFG.memoryDatabase.

Just see if it works ok when set to ":memory:" (passing whatever name you want to indexedDB.open()).

Also if you could check CFG.autoName, just setting it to true and passing the empty string to indexedDB.open("") to confirm it picks a database name.

If not, I guess I'll just keep it listed as untested for now.

@hawkrives
Copy link

I'll try testing it today or tomorrow, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants