[persistent_cache]: Introduce schema versioning [chromium/src : main]
Greg Thompson (Gerrit)
Greg Thompson added 11 comments![]( "Open in Gerrit")
constexpr int32_t kLatestUserVersion = 1;q: `Current` since it's the one that's currently being used until the next commit that changes it? i dunno...
if (!db_->Open(database_path_)) {a counter-proposal in pseudocode:
```
read the fist hunk of the db file into a buffer before opening the databse
pull out the ints at offset 68[1] and offset 60[1] in the buffer
if these don't match the current version:
if not vfs_file_set_.read_only():
truncate all files -- the cache becomes a fresh cache file
else:
return error
```
i think this is less code, less overhead, and easier to reason about compared to checking for existence of the table. it also avoids the GPU case of needing to bounce back to the browser process to get a new file when there's a version change (the browser doesn't open the db today). wdyt?
1. [application id](https://sqlite.org/fileformat2.html#application_id)
2. [user version number](https://sqlite.org/fileformat2.html#user_version_number)
(i'm not sure if this is a great idea -- just tossing it out there as a possibility)
sql::Statement get_user_version_stm =
sql::Statement(db_->GetUniqueStatement("PRAGMA user_version"));nit: omit `= sql::Statement` -- just `sql::Statement get_user_version_stm(db_->GetUniqueStatement("PRAGMA user_version"));`
DCHECK(get_user_version_stm.is_valid());what are the failure modes? would we really want to crash a debug/DCHECK build for them?
std::optional<int32_t> detected_user_version;nit: move down after `Step()` and return
detected_user_version.emplace(get_user_version_stm.ColumnInt(0));we don't need `optional` on this int, do we?
{"PRAGMA user_version=", base::NumberToString(user_version_)}))) {even though this is in a transaction, shall we move this down after table creation? i think this is more natural, but it's your call.
const std::string create_table_sql = base::StringPrintf(`StringPrintf` is a horribly expensive beast. :-) wdyt about either putting this statement up at the top of the file next to the table name's definition so that they can easily be audited? or do some light preprocessor juju to mash things together? i'm not concerned about leaving "entries" in this string as it was. i think it will take a developer 5 seconds to realize their mistake if they're changing the name of the table and miss one of the places.
// Grab another pending backend to be able to re-open the database.
ASSERT_OK_AND_ASSIGN(
auto other_pending_backend,
backend_storage_->MakePendingBackend(db_path, false, false));one must use `ShareReadWriteConnection` or `ShareReadOnlyConnection` to get a new `PendingBackend` to connect to an existing backend. this is the only way to have the shared lock be available for all connections. as it is now in this test, each pending backend has its own shared lock over the same files, which is dangerous.
please refactor to either use `MakePendingBackend` a second time only after destroying the first `Backend` (so that all handles are closed), or `Bind` the first `PendingBackend` and then use `Share...Connection` to get a second `PendingBackend` to the same storage.
even though this is a test, and you might be tempted to argue "it's fine here 'cause we're not using the lock", there's still a risk of someone seeing this and trying to use this pattern in production.
kInvalidVersion);nit: instead of testing by changing what is considered the "current" version (which requires adding a data member to each instance), wdyt about modifying a file on-disk to change its version? could either do it via SQL statements or just by flipping bits at offsets 60-63.
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li added 10 comments![]( "Open in Gerrit")
q: `Current` since it's the one that's currently being used until the next commit that changes it? i dunno...
Sure current sounds good 😊
if (!db_->Open(database_path_)) {a counter-proposal in pseudocode:
```
read the fist hunk of the db file into a buffer before opening the databse
pull out the ints at offset 68[1] and offset 60[1] in the buffer
if these don't match the current version:
if not vfs_file_set_.read_only():
truncate all files -- the cache becomes a fresh cache file
else:
return error
```i think this is less code, less overhead, and easier to reason about compared to checking for existence of the table. it also avoids the GPU case of needing to bounce back to the browser process to get a new file when there's a version change (the browser doesn't open the db today). wdyt?
1. [application id](https://sqlite.org/fileformat2.html#application_id)
2. [user version number](https://sqlite.org/fileformat2.html#user_version_number)(i'm not sure if this is a great idea -- just tossing it out there as a possibility)
I think that reading the file number directly or truncating adds complications because it probably needs to happen under the lock so the file doesn't change in a non-thread-safe way? It's always going to be safe for exclusive connections but it feels maybe a bit clunky to have some ReadWrite backends feel confident to truncate and not others.
Local debugging shows me that we take the SHARED lock to execute the `get_user_version` statement. So I think that we could issue the truncate if we took the EXCLUSIVE lock first which can be done manually through SanboxedFile.
Alternatively we could also `DROP TABLE` which would implicitly take the lock on setups that require it without changing much in the case of exclusive connections? I quite like the simplicity of truncating the file though.
I've modified the current implementation of the CL to address some of the points raised [simplicity, speed] and once we figure out the lock situation I can have a go at adding the local handling of version mismatch.
sql::Statement get_user_version_stm =
sql::Statement(db_->GetUniqueStatement("PRAGMA user_version"));nit: omit `= sql::Statement` -- just `sql::Statement get_user_version_stm(db_->GetUniqueStatement("PRAGMA user_version"));`
Right! I've changed another instance of this in FindImpl() while I was there.
what are the failure modes? would we really want to crash a debug/DCHECK build for them?
To be completely honest I've been copy pasting that check from other parts of the code without thinking about it.
There's actually essentially zero chance for such a simple statement to spontaneously beome invalid and even then I'm not sure fully crashing is the way to go.
I've removed the DCHECK. It could be worth it to revesit the other instances of this check in the code.
nit: move down after `Step()` and return
Right. I'm not sure why I was creating the variable in advance at all.
detected_user_version.emplace(get_user_version_stm.ColumnInt(0));we don't need `optional` on this int, do we?
As commented elsewhere I'm not sure what I was thinking there. There's no reason to not just initalize with the value from the query.
{"PRAGMA user_version=", base::NumberToString(user_version_)}))) {even though this is in a transaction, shall we move this down after table creation? i think this is more natural, but it's your call.
SGTM.
const std::string create_table_sql = base::StringPrintf(`StringPrintf` is a horribly expensive beast. :-) wdyt about either putting this statement up at the top of the file next to the table name's definition so that they can easily be audited? or do some light preprocessor juju to mash things together? i'm not concerned about leaving "entries" in this string as it was. i think it will take a developer 5 seconds to realize their mistake if they're changing the name of the table and miss one of the places.
Yeah good points 0_o. Getting rid of the StringPrintf is actually the only thing I can change in this CL to materially affect the OpenClose perftest!
Since I've stopped relying on querying for the presence of the table this has just gone away naturally 😊
// Grab another pending backend to be able to re-open the database.
ASSERT_OK_AND_ASSIGN(
auto other_pending_backend,
backend_storage_->MakePendingBackend(db_path, false, false));one must use `ShareReadWriteConnection` or `ShareReadOnlyConnection` to get a new `PendingBackend` to connect to an existing backend. this is the only way to have the shared lock be available for all connections. as it is now in this test, each pending backend has its own shared lock over the same files, which is dangerous.
please refactor to either use `MakePendingBackend` a second time only after destroying the first `Backend` (so that all handles are closed), or `Bind` the first `PendingBackend` and then use `Share...Connection` to get a second `PendingBackend` to the same storage.
even though this is a test, and you might be tempted to argue "it's fine here 'cause we're not using the lock", there's still a risk of someone seeing this and trying to use this pattern in production.
That's certainly an oversight on my part and not something I wanted to do because it happens to pass.
I agree it should be done properly everywhere and I've added explicit scoping.
nit: instead of testing by changing what is considered the "current" version (which requires adding a data member to each instance), wdyt about modifying a file on-disk to change its version? could either do it via SQL statements or just by flipping bits at offsets 60-63.
I don't think it's a nit because I was quite annoyed at adding the new member so I'm happy to get rid of it!
I've changed the code to modify the files directly.
Since the test is not very fine grained (Initialize() fails or succeeds) the one thing I don't like about messing with the file is that if I did something very wrong and completely mangled them it would fail just the same as a bad version.
I think that adding a simetrical operation on the success case provides at least some protection.
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 13 comments![]( "Open in Gerrit")
static constexpr int32_t kCurrentUserVersion = 1;#include <stdint.h>
constexpr int32_t kDefaultUserVersion = 0;#include <stdint.h>
sql::Transaction transaction(&*db_);wdyt about avoiding the transaction if in read-only mode? it'll add overhead for no benefit -- we only read the value in that case.
if (!db_->Open(database_path_)) {Olivier Lia counter-proposal in pseudocode:
```
read the fist hunk of the db file into a buffer before opening the databse
pull out the ints at offset 68[1] and offset 60[1] in the buffer
if these don't match the current version:
if not vfs_file_set_.read_only():
truncate all files -- the cache becomes a fresh cache file
else:
return error
```i think this is less code, less overhead, and easier to reason about compared to checking for existence of the table. it also avoids the GPU case of needing to bounce back to the browser process to get a new file when there's a version change (the browser doesn't open the db today). wdyt?
1. [application id](https://sqlite.org/fileformat2.html#application_id)
2. [user version number](https://sqlite.org/fileformat2.html#user_version_number)(i'm not sure if this is a great idea -- just tossing it out there as a possibility)
I think that reading the file number directly or truncating adds complications because it probably needs to happen under the lock so the file doesn't change in a non-thread-safe way? It's always going to be safe for exclusive connections but it feels maybe a bit clunky to have some ReadWrite backends feel confident to truncate and not others.
Local debugging shows me that we take the SHARED lock to execute the `get_user_version` statement. So I think that we could issue the truncate if we took the EXCLUSIVE lock first which can be done manually through SanboxedFile.
Alternatively we could also `DROP TABLE` which would implicitly take the lock on setups that require it without changing much in the case of exclusive connections? I quite like the simplicity of truncating the file though.
I've modified the current implementation of the CL to address some of the points raised [simplicity, speed] and once we figure out the lock situation I can have a go at adding the local handling of version mismatch.
I think that reading the file number directly or truncating adds complications because it probably needs to happen under the lock so the file doesn't change in a non-thread-safe way? It's always going to be safe for exclusive connections but it feels maybe a bit clunky to have some ReadWrite backends feel confident to truncate and not others.
We need to worry about the shared lock if we expect to ever have multiple connections racing to open the same db for the first time. The API doesn't provide a way to share a connection that hasn't been initialized (`ShareRead{Only,Write}Connection` requires a `PersistentCache`), so the first writer will _always_ be the only one passing through `Initialize()`. In other words: if `Initialize()` is called on a r/w instance and the version doesn't match, we know that this is the first opener so it's safe to clear the db without worrying about extra locking.
Are there any use-cases today where we'd want to fail the first opener without deleting the files?
- GPU: the process has exclusive r/w access, so clearing the files and starting over is safe and easier than round-tripping back to the browser to get new files. (The browser never even bothers to open the DB.)
- CodeCache: `PersistentCacheCollection` in the browser always binds before sharing connections, so it is always the only party holding a db. It would be able to delete the files and retry without extra process hopping.
In neither case would we want to retain the out-of-date files. Since we've taken the philosophy of "it's a cache -- delete it at will" elsewhere, why not do so here, too?
Local debugging shows me that we take the SHARED lock to execute the `get_user_version` statement. So I think that we could issue the truncate if we took the EXCLUSIVE lock first which can be done manually through SanboxedFile.
I'm not sure we should manually fiddle with the shared lock. I'm concerned that we could accidentally create some state confusion between SQLite and our lock impl. A `BEGIN EXCLUSIVE` [statement](https://sqlite.org/lang_transaction.html#immediate) will take the write lock from the get-go, so we should do that if the need arises. (I don't think we need to worry about that here, though.)
Alternatively we could also `DROP TABLE` which would implicitly take the lock on setups that require it without changing much in the case of exclusive connections? I quite like the simplicity of truncating the file though.
Good point. Maybe `DROP TABLE` is the safest thing. Otherwise, I think if we were going to go the truncate route, we would want to close the db first and re-open afterwards to be sure that there's no state confusion.
I've modified the current implementation of the CL to address some of the points raised [simplicity, speed] and once we figure out the lock situation I can have a go at adding the local handling of version mismatch.
constexpr int64_t kUserVersionOffset = 60;iwyu
std::optional<BackendStorage> backend_storage_;iwyu
// bound with different connections with is incorrect.nit: "connections, which"
ASSERT_TRUE(backend);alternatively, remove this and change the line above to:
`ASSERT_NE(SqliteBackendImpl::Bind(std::move(pending_backend)), nullptr);`
you could also get rid of the scoping and just put a comment above it saying something like "// Initialize the database and close it."
auto other_pending_backend,could reuse `pending_backend` here
base::File db_file = other_pending_backend.sqlite_data.db_file.Duplicate();wdyt of changing things a little bit so that we `Read` directly from `db_file` here (without duplicating) to verify that the user version is set by the `Bind()` above?
then we can do the second bind to be sure that it succeeds. the final "Other tests rely on..." comment and code can go away.
base::File db_file = pending_backend.sqlite_data.db_file.Duplicate();instead of duplicating the file for use below, could we move the `Write` after getting the second `PendingBackend` and write directly to the file in it before binding it?
auto backend = SqliteBackendImpl::Bind(std::move(pending_backend));`ASSERT_NE(..., nullptr);` here, too
auto backend = SqliteBackendImpl::Bind(std::move(other_pending_backend));`ASSERT_EQ(..., nullptr);`
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li added 14 comments![]( "Open in Gerrit")
static constexpr int32_t kCurrentUserVersion = 1;Olivier Li#include <stdint.h>
Done
constexpr int32_t kDefaultUserVersion = 0;Olivier Li#include <stdint.h>
Done
wdyt about avoiding the transaction if in read-only mode? it'll add overhead for no benefit -- we only read the value in that case.
Sounds good.
I don't think there are any usecases where you can't get rid of the files no.
Regarding no two opens racing I follow your logic and I think that's right. Maybe I feel like the API would make that a tad more clear if `BackendStorage::MakePendingBackend` somehow returned an object that is not sendable through mojo but I guess we need it for the case of the GPU. In any case I think your explanation makes sense and it's fine to change.
Thank you for bringing up the exclusive transaction that sounds like something that would simplify digging into the file directly if we needed it.
I've had a go at dropping the table when the connection is ReadWrite and testing that the Bind now succeeds. The previous test that validates failure is still needed for read only connections so I've kept and renamed it.
if (!stm.is_valid()) {`DCHECK`s on statement validity removed because they were never correct.
The statement will be invalid on `SQLITE_BUSY` which is not a reason to crash any client.
This became apparent as a problem while trying this CL on CQ because bots started crashing on the line.
constexpr int64_t kUserVersionOffset = 60;Olivier Liiwyu
Done
std::optional<BackendStorage> backend_storage_;Olivier Liiwyu
Done
// bound with different connections with is incorrect.Olivier Linit: "connections, which"
Done
alternatively, remove this and change the line above to:
`ASSERT_NE(SqliteBackendImpl::Bind(std::move(pending_backend)), nullptr);`you could also get rid of the scoping and just put a comment above it saying something like "// Initialize the database and close it."
Sounds good!.
auto other_pending_backend,could reuse `pending_backend` here
How so if I moved it?
base::File db_file = other_pending_backend.sqlite_data.db_file.Duplicate();wdyt of changing things a little bit so that we `Read` directly from `db_file` here (without duplicating) to verify that the user version is set by the `Bind()` above?
then we can do the second bind to be sure that it succeeds. the final "Other tests rely on..." comment and code can go away.
SGTM.
base::File db_file = pending_backend.sqlite_data.db_file.Duplicate();instead of duplicating the file for use below, could we move the `Write` after getting the second `PendingBackend` and write directly to the file in it before binding it?
No longer possible now that this test validates a read_only connection and we can't write to the file handle of the second pending backend.
I've applied this recommendation to the readwrite test though.
auto backend = SqliteBackendImpl::Bind(std::move(pending_backend));Olivier Li`ASSERT_NE(..., nullptr);` here, too
Done
auto backend = SqliteBackendImpl::Bind(std::move(other_pending_backend));Olivier Li`ASSERT_EQ(..., nullptr);`
Done
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li added 1 comment![]( "Open in Gerrit")
if (!stm.is_valid()) {`DCHECK`s on statement validity removed because they were never correct.
The statement will be invalid on `SQLITE_BUSY` which is not a reason to crash any client.
This became apparent as a problem while trying this CL on CQ because bots started crashing on the line.
For ease of error tracking I think it's better to let the execution of the statement and have access to the errors in the the `sql::Database` metrics.
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li voted and added 2 comments![]( "Open in Gerrit")
Votes added by Olivier Li
| Commit-Queue | +1 |
2 comments
DCHECK(stm.is_valid());DCHECKs on statement validity removed because they were never correct.
The statement will be invalid on SQLITE_BUSY which is not a reason to crash any client.
This became apparent as a problem while trying this CL on CQ because bots started crashing on the line. For ease of error tracking I think it's better to let the execution of the statement fail and have access to the errors in the the sql::Database metrics.
if (!stm.is_valid()) {Olivier Li`DCHECK`s on statement validity removed because they were never correct.
The statement will be invalid on `SQLITE_BUSY` which is not a reason to crash any client.
This became apparent as a problem while trying this CL on CQ because bots started crashing on the line.
For ease of error tracking I think it's better to let the execution of the statement and have access to the errors in the the `sql::Database` metrics.
Acknowledged
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
it's going to take me a day or two to get to this. thanks for your patience.
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 6 comments![]( "Open in Gerrit")
partial review. sorry for the delay. :-(
if (!IsReadOnly()) {
transaction.emplace(&*db_);
if (!transaction->Begin()) {
return false;
}
}optional suggestion:
```
if (!IsReadOnly() && !transaction.emplace(&*db_).Begin()) {
return false;
}
```
I don't think there are any usecases where you can't get rid of the files no.
Regarding no two opens racing I follow your logic and I think that's right. Maybe I feel like the API would make that a tad more clear if `BackendStorage::MakePendingBackend` somehow returned an object that is not sendable through mojo but I guess we need it for the case of the GPU.
We do want callers to send the `PB` from `MakePendingBackend` directly to other processes -- having the browser open the database before sending it to the GPU would add overhead. I don't know why we'd need that.
A caller of `MakePendingBackend` can do one of two things with the instance it receives: 1) send it to another process, or 2) use `PersistentCache::Bind` to open it. It can't do both. If it wishes to have more than one connection to a cache, then it MUST use `PersistentCache::Bind` to open it and then use `BackendStorage::ShareRead{Only,Write}Backend` to get a new `PB` to it. We have a programming error if someone is calling `MakePendingBackend` twice for the same cache.
If you think we need better doc comments around this, please add them.
get_user_version_stm.reset();nit: drop use of `std::optional<>` around this and use `get_user_version_stm.Reset(/*clear_bound_vars=*/true);` here.
DCHECK(stm.is_valid());DCHECKs on statement validity removed because they were never correct.
The statement will be invalid on SQLITE_BUSY which is not a reason to crash any client.
This became apparent as a problem while trying this CL on CQ because bots started crashing on the line. For ease of error tracking I think it's better to let the execution of the statement fail and have access to the errors in the the sql::Database metrics.
Acknowledged
auto other_pending_backend,Olivier Licould reuse `pending_backend` here
How so if I moved it?
i mean remove `auto` so that the new instance is put into the existing local variable. (apologies for being overly brief!)
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li added 4 comments![]( "Open in Gerrit")
if (!IsReadOnly()) {
transaction.emplace(&*db_);
if (!transaction->Begin()) {
return false;
}
}optional suggestion:
```
if (!IsReadOnly() && !transaction.emplace(&*db_).Begin()) {
return false;
}
```
SGTM!
if (!db_->Open(database_path_)) {Agreed.
The case of the GPU makes sense to me. I didn't make that clear in my original comment but I indeed don't think we should rely on having had an Open take place all the time.
nit: drop use of `std::optional<>` around this and use `get_user_version_stm.Reset(/*clear_bound_vars=*/true);` here.
Ah right that's nicer thanks.
auto other_pending_backend,Olivier Licould reuse `pending_backend` here
Greg ThompsonHow so if I moved it?
i mean remove `auto` so that the new instance is put into the existing local variable. (apologies for being overly brief!)
Ah right! Yeah that sounds good I realize now it's not nice to have the old variable lying around.
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 8 comments![]( "Open in Gerrit")
// evolves in a non-backwards compatible way.consider documenting the impact of incrementing: all existing databases of earlier version(s) are cleared on `Bind`.
TRACE_EVENT_INSTANT1("persistent_cache", "open_failed",just curious: is removing these intentional? should we also remove the ones for find_failed and insert_failed? do they add value over the tracing done in `sql::Database::OnSqliteError`?
std::optional<sql::Transaction> transaction;could we check the version before starting the transaction so that we only pay that cost when the db needs to be mutuated?
sql::Statement get_user_version_stm(
db_->GetUniqueStatement("PRAGMA user_version"));
if (!get_user_version_stm.Step()) {
return false;
}
const int32_t detected_user_version = get_user_version_stm.ColumnInt(0);
// Reset statement so the database is not locked when we potentially
// try to drop it.
get_user_version_stm.Reset(/*clear_bound_vars=*/true);yet another way to do this so that we don't even need `Reset` and the accompanying comment: :-)
```
int32_t detected_user_version;
if (sql::Statement get_user_version_stm(
db_->GetUniqueStatement("PRAGMA user_version"));
get_user_version_stm.Step()) {
detected_user_version = get_user_version_stm.ColumnInt(0);
} else {
return false;
}
```
if (detected_user_version != kCurrentUserVersion) {could we return early if they match to reduce indentation?
if (!db_->Execute("CREATE TABLE entries(key TEXT PRIMARY KEY "should we switch to `BLOB` now, or is there a reason to do that separately? hmm. we could switch from `std::string_view` to `base::span<const uint8_t>` for the key at the same time. i've convinced myself that we should do that separately. :-)
"UNIQUE NOT "nit: could you adjust where the line breaks are so that this is a bit easier to read?
EXPECT_THAT(cache->Insert(kKey, base::byte_span_from_cstring("1")),
HasValue());i recently noticed that we have `EXPECT_OK(...);` for this case.
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li added 9 comments![]( "Open in Gerrit")
consider documenting the impact of incrementing: all existing databases of earlier version(s) are cleared on `Bind`.
That's a good point. Done.
TRACE_EVENT_INSTANT1("persistent_cache", "open_failed",just curious: is removing these intentional? should we also remove the ones for find_failed and insert_failed? do they add value over the tracing done in `sql::Database::OnSqliteError`?
Removing them was indeed intentional because it didn't make sense to keep them when they were so many new early returns.
I don't think the others add much and I've gone ahead and removed them.
could we check the version before starting the transaction so that we only pay that cost when the db needs to be mutuated?
Yes we can. I've moved it closer to the mutating calls. It's also no longer an optional because if that point is reached in the code then we know it's needed.
sql::Statement get_user_version_stm(
db_->GetUniqueStatement("PRAGMA user_version"));
if (!get_user_version_stm.Step()) {
return false;
}
const int32_t detected_user_version = get_user_version_stm.ColumnInt(0);
// Reset statement so the database is not locked when we potentially
// try to drop it.
get_user_version_stm.Reset(/*clear_bound_vars=*/true);yet another way to do this so that we don't even need `Reset` and the accompanying comment: :-)
```
int32_t detected_user_version;
if (sql::Statement get_user_version_stm(
db_->GetUniqueStatement("PRAGMA user_version"));
get_user_version_stm.Step()) {
detected_user_version = get_user_version_stm.ColumnInt(0);
} else {
return false;
}
```
Nice!
could we return early if they match to reduce indentation?
Yep!
if (!db_->Execute("CREATE TABLE entries(key TEXT PRIMARY KEY "should we switch to `BLOB` now, or is there a reason to do that separately? hmm. we could switch from `std::string_view` to `base::span<const uint8_t>` for the key at the same time. i've convinced myself that we should do that separately. :-)
I've put it up as a dependant CL so we can land the whole chain the save DEV release hopefully and avoid users clearing their caches too many times 😊
nit: could you adjust where the line breaks are so that this is a bit easier to read?
Done
if (!stm.is_valid()) {Note: Validity checks brought back because without them we sometimes trip this: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=98;drc=f8a495827fdd58be86e707d7396a4fad7014a117
The error code wouldn't have been logged from sql::Database anyways after all because of the early return here: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=118;drc=c8fb498259ce5c219f957dbe367da21a82a361c0
EXPECT_THAT(cache->Insert(kKey, base::byte_span_from_cstring("1")),
HasValue());i recently noticed that we have `EXPECT_OK(...);` for this case.
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 10 comments![]( "Open in Gerrit")
static constexpr int32_t kCurrentUserVersion = 1;`int` here and everywhere else a version is stored? (see related comment in .cc)
int32_t detected_user_version;`ColumnInt` returns an `int` -- is there a reason not to use that here?
int32_t detected_user_version;nit: a comment above this would help make it more clear why we're doing all of this. maybe something like:
```
// Check the user-version (https://sqlite.org/pragma.html#pragma_user_version)
// to see if there has been a schema change since the last time this database
// was modified.
```
// Begin an explicit transaction so that creating the table andanother comment suggestion above this point:
```
// This is either a new database (user-version has never been set) or was last
// written with an old schema. Clear it and create the current schema's table.
```
// If a table already exists drop it to start over fresh.this comment isn't needed if you take my suggestion above
if (!db_->Execute("DROP TABLE IF EXISTS entries")) {maybe it's obvious, but we'll need to adjust this if we ever have a schema with anything other than the one "entries" table. do you think it's worth being explicit about this so that future maintainers don't have to think as much? maybe a comment just above the `CREATE TABLE` statement below saying something like "IMPORTANT: Revise the `DROP TABLE` statement above if more than the one "entries" table is created here."?
if (!stm.is_valid()) {Note: Validity checks brought back because without them we sometimes trip this: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=98;drc=f8a495827fdd58be86e707d7396a4fad7014a117
The error code wouldn't have been logged from sql::Database anyways after all because of the early return here: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=118;drc=c8fb498259ce5c219f957dbe367da21a82a361c0
if my read is correct, we should check `stm` immediately after construction. it will be invalid if `sqlite3_prepare_v3` failed, in which case the error will go through `Database::OnSqliteError`. if it's valid at that point, it will stay valid through all `Bind+++` calls even if one of those fails -- a failure to bind doesn't close the statement or the db, does it?
are you seeing cases where we're failing to get the statement here, or where a statement becomes invalid after creation? if the former, is the failure due to something other than invalid SQL? where are you seeing that we're hitting that `LOG(FATAL) in `CheckValid`?
nit: rename to `components/persistent_cache/sqlite/sqlite_backend_impl_unittest.cc`
#include "components/persistent_cache/sqlite/sqlite_backend_impl.h"move above all other includes as per https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
const base::FilePath db_path = base::FilePath::FromASCII("Cache");nit: `db_basename` or somesuch -- this isn't a full path (see BackendStorage::MakePendingBackend)
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li added 9 comments![]( "Open in Gerrit")
`int` here and everywhere else a version is stored? (see related comment in .cc)
Done
`ColumnInt` returns an `int` -- is there a reason not to use that here?
There isn't really a reason no. Int is fine.
nit: a comment above this would help make it more clear why we're doing all of this. maybe something like:
```
// Check the user-version (https://sqlite.org/pragma.html#pragma_user_version)
// to see if there has been a schema change since the last time this database
// was modified.
```
I think that makes sense. I used your comment.
// Begin an explicit transaction so that creating the table andanother comment suggestion above this point:
```
// This is either a new database (user-version has never been set) or was last
// written with an old schema. Clear it and create the current schema's table.
```
Good point, I took it just slighty modified to represent the order of the operations.
// If a table already exists drop it to start over fresh.this comment isn't needed if you take my suggestion above
Done
maybe it's obvious, but we'll need to adjust this if we ever have a schema with anything other than the one "entries" table. do you think it's worth being explicit about this so that future maintainers don't have to think as much? maybe a comment just above the `CREATE TABLE` statement below saying something like "IMPORTANT: Revise the `DROP TABLE` statement above if more than the one "entries" table is created here."?
I think it's obvious while reading the code during active development but a comment doesn't hurt for somebody that would come in for a "quick fix" 😊
if (!stm.is_valid()) {Greg ThompsonNote: Validity checks brought back because without them we sometimes trip this: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=98;drc=f8a495827fdd58be86e707d7396a4fad7014a117
The error code wouldn't have been logged from sql::Database anyways after all because of the early return here: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=118;drc=c8fb498259ce5c219f957dbe367da21a82a361c0
if my read is correct, we should check `stm` immediately after construction. it will be invalid if `sqlite3_prepare_v3` failed, in which case the error will go through `Database::OnSqliteError`. if it's valid at that point, it will stay valid through all `Bind+++` calls even if one of those fails -- a failure to bind doesn't close the statement or the db, does it?
are you seeing cases where we're failing to get the statement here, or where a statement becomes invalid after creation? if the former, is the failure due to something other than invalid SQL? where are you seeing that we're hitting that `LOG(FATAL) in `CheckValid`?
I think it makes more sense to return before binding.
Am I seeing cases where we fail to get a valid statement here: Yes, on SQLITE_BUSY. (So indeed something other than invalid SQL).
Do I see the statement becoming invalid later: I haven't seen that spcecifically but haven't hunted for it either.
Where did I hit the fatal LOG? :
#include "components/persistent_cache/sqlite/sqlite_backend_impl.h"move above all other includes as per https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
Done
const base::FilePath db_path = base::FilePath::FromASCII("Cache");nit: `db_basename` or somesuch -- this isn't a full path (see BackendStorage::MakePendingBackend)
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li abandoned this change.
Olivier Li abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | +1 |
2 comments
if (!stm.is_valid()) {Greg ThompsonNote: Validity checks brought back because without them we sometimes trip this: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=98;drc=f8a495827fdd58be86e707d7396a4fad7014a117
The error code wouldn't have been logged from sql::Database anyways after all because of the early return here: https://source.chromium.org/chromium/chromium/src/+/main:sql/statement.cc;l=118;drc=c8fb498259ce5c219f957dbe367da21a82a361c0
Olivier Liif my read is correct, we should check `stm` immediately after construction. it will be invalid if `sqlite3_prepare_v3` failed, in which case the error will go through `Database::OnSqliteError`. if it's valid at that point, it will stay valid through all `Bind+++` calls even if one of those fails -- a failure to bind doesn't close the statement or the db, does it?
are you seeing cases where we're failing to get the statement here, or where a statement becomes invalid after creation? if the former, is the failure due to something other than invalid SQL? where are you seeing that we're hitting that `LOG(FATAL) in `CheckValid`?
I think it makes more sense to return before binding.
Am I seeing cases where we fail to get a valid statement here: Yes, on SQLITE_BUSY. (So indeed something other than invalid SQL).
Do I see the statement becoming invalid later: I haven't seen that spcecifically but haven't hunted for it either.
Where did I hit the fatal LOG? :
Acknowledged
#include "components/persistent_cache/sqlite/sqlite_backend_impl.h"Olivier Limove above all other includes as per https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
Done
did clang-format move it back down here? maybe renaming this test file to sqlite_backend_impl_unittest.cc will allow it to keep the include where it belongs?
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |