Closed
Bug 1423820
Opened 7 years ago
Closed 7 years ago
Copy temp entities after reattaching databases to a cloned connection
Categories
(Core :: SQLite and Embedded Database Bindings, enhancement, P1)
Core
SQLite and Embedded Database Bindings
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(1 file)
We should copy over temp entities after reattaching databases, in case the connection sets up temp triggers on attached tables. Places does this for `moz_icons` in https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/toolkit/components/places/nsPlacesTriggers.h#297-302.
We should also probably close the cloned connection if copying temp entities fails, though I'm not sure if the `AsyncInitializeClone` machinery already handles that for us when it releases `mClone`.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.
https://reviewboard.mozilla.org/r/206134/#review211728
C/C++ static analysis found 4 defects in this patch.
You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
::: storage/mozStorageConnection.cpp:1536
(Diff revision 1)
> + });
> +
> + // Re-attach on-disk databases that were attached to the original connection.
> + {
> + nsCOMPtr<mozIStorageStatement> stmt;
> + rv = CreateStatement(NS_LITERAL_CSTRING("PRAGMA database_list"),
Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
::: storage/mozStorageConnection.cpp:1549
(Diff revision 1)
> + !name.EqualsLiteral("temp")) {
> + nsCString path;
> + rv = stmt->GetUTF8String(2, path);
> + if (NS_SUCCEEDED(rv) && !path.IsEmpty()) {
> + nsCOMPtr<mozIStorageStatement> attachStmt;
> + rv = aClone->CreateStatement(
Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
::: storage/mozStorageConnection.cpp:1553
(Diff revision 1)
> + nsCOMPtr<mozIStorageStatement> attachStmt;
> + rv = aClone->CreateStatement(
> + NS_LITERAL_CSTRING("ATTACH DATABASE :path AS ") + name,
> + getter_AddRefs(attachStmt));
> + MOZ_ASSERT(NS_SUCCEEDED(rv));
> + rv = attachStmt->BindUTF8StringByName(NS_LITERAL_CSTRING("path"),
Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
::: storage/mozStorageConnection.cpp:1556
(Diff revision 1)
> + getter_AddRefs(attachStmt));
> + MOZ_ASSERT(NS_SUCCEEDED(rv));
> + rv = attachStmt->BindUTF8StringByName(NS_LITERAL_CSTRING("path"),
> + path);
> + MOZ_ASSERT(NS_SUCCEEDED(rv));
> + rv = attachStmt->Execute();
Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Actually, we need to do this after setting `PRAGMA temp_store`, because https://sqlite.org/pragma.html says:
> When the temp_store setting is changed, all existing temporary tables, indices, triggers, and views are immediately deleted.
I also wrapped this up in a transaction, but I don't know if that's strictly necessary.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.
https://reviewboard.mozilla.org/r/206134/#review213100
Attachment #8935250 -
Flags: review?(mak77) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1f8af3283ae
Copy temp entities after reattaching databases to a cloned storage connection. r=mak
Updated•7 years ago
|
Priority: -- → P1
Comment 10•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•2 months ago
|
Product: Toolkit → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•