sifive / wake

The SiFive wake build tool
Other
86 stars 28 forks source link

Open wake.db in read-only mode for commands that do not require writing to the database #501

Open richardxia opened 3 years ago

richardxia commented 3 years ago

Wake currently always opens the database in read-write mode, even when running a command like wake --failed or wake -o. This can be problematic in multiuser environments where one may want to inspect the database of workspace owned by a different user but only has read, not write access.

The current workaround I employ is copying the database file from the other user's workspace into a directory that I own so that I can run the commands listed above.

terpstra commented 3 years ago

So it turns out this is not possible: https://sqlite.org/wal.html

If you can't create the wake.db-wal, you cannot read the database, even with the newest version of sqlite3.

I'm leaving this issue open, because I do think it is an issue and it is not solved. Presumably moving to a different database might alleviate this.

richardxia commented 3 years ago

The page that you linked to includes the following section:

  1. Read-Only Databases Older versions of SQLite could not read a WAL-mode database that was read-only. In other words, write access was required in order to read a WAL-mode database. This constraint was relaxed beginning with SQLite version 3.22.0 (2018-01-22).

On newer versions of SQLite, a WAL-mode database on read-only media, or a WAL-mode database that lacks write permission, can still be read as long as one or more of the following conditions are met:

  1. The -shm and -wal files already exists and are readable
  2. There is write permission on the directory containing the database so that the -shm and -wal files can be created.
  3. The database connection is opened using the immutable query parameter.

Even though it is possible to open a read-only WAL-mode database, it is good practice to converted to PRAGMA journal_mode=DELETE prior to burning an SQLite database image onto read-only media.

Based on my interpretation of the documentation, you only need to satisfy one of the three criteria, and the third one looks like you can change the way that the path to the file is specified to use a file:// URI that contains a special query parameter for enabling immutable mode. Is this something that can be specified in the C bindings for opening the DB connection?

richardxia commented 3 years ago

Also, for convenience, here's the documentation on the immutable query parameter:

The immutable query parameter is a boolean that signals to SQLite that the underlying database file is held on read-only media and cannot be modified, even by another process with elevated privileges. SQLite always opens immutable database files read-only and it skips all file locking and change detection on immutable database files. If these query parameter (or the SQLITE_IOCAP_IMMUTABLE bit in xDeviceCharacteristics) asserts that a database file is immutable and that file changes anyhow, then SQLite might return incorrect query results and/or SQLITE_CORRUPT errors.

terpstra commented 3 years ago

Yes, I've read that. If you look closely, you'll also see that it will not work correctly if you claim the database is immutable and yet another process modifies it. You also need an sqlite version newer than we have on our machines. So, if we want this, we'd have to use advisory locks, which are broken for nfs.

I forgot we're using a statically linked wake on the gamma machines now, so the sqlite version is less of a problem and maybe we can get this to work.

richardxia commented 3 years ago

If you look closely, you'll also see that it will not work correctly if you claim the database is immutable and yet another process modifies it.

I saw that, but I think there are a number of cases where this is fine. For example, the scenario I described where the file and the directory are owned by another user, and where I have read but not write access, it would be helpful to be able to use wake --failed, since most of the time there isn't even another running wake process.

Even if there's some terrible, terrible atomicity issue with another running wake process modifying the DB, I think it's OK for this read-only wake process to sometimes return the wrong result or to even panic, since it's trivial enough for a user to just rerun the command.