[tracing] Move payload to a different table in trace reports [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Related details
- Etienne Bergeron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Bergeron (Gerrit)
Etienne Bergeron added 9 comments![]( "Open in Gerrit")
if (!database_.DoesTableExist(kLocalTracesIndexTableName)) {You can combine these two if-statements.
if (!create_index_entry.Run()) {Since you have two Run statement, would it be better to have a transaction around it? This will ensure that if one statement fails, both fails.
```
sql::Transaction transaction(&database_);
if (!transaction.Begin()) return false;
[...]
stm1.Run();
stm2.Run();
return transaction.Commit();
```
database_.GetCachedStatement(SQL_FROM_HERE, R"sql(recommendation for the format is here:
https://chromium.googlesource.com/chromium/src/+/HEAD/sql/README.md#SQL-style
The current format have heading spaces which will be forbidden by the style guide soon.
create_payload.BindBlob(1, new_report.trace_content);If this is big (and performance matters), you should use the Blob streaming API.
If it's big and performance don't matters, leave it that way.
UPDATE local_traces_indexDitto for style
std::string received_value = get_content.ColumnString(0);Here again, potentially using streaming API.
See "Find" in the same file linked above.
DELETE FROM local_traces_payloadsditto for style (all of them)
return initialized_;return initialized_ && Transaction.Commit()
This will ensures that not half of a migration / cleanup is done.
it's the recommended practice.
Related details
- Etienne Pierre-Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 10 comments![]( "Open in Gerrit")
+anthonyvd@ since etienneb is OOO
if (!database_.DoesTableExist(kLocalTracesIndexTableName)) {You can combine these two if-statements.
Done
Since you have two Run statement, would it be better to have a transaction around it? This will ensure that if one statement fails, both fails.
```
sql::Transaction transaction(&database_);
if (!transaction.Begin()) return false;
[...]
stm1.Run();
stm2.Run();return transaction.Commit();
```
Done
recommendation for the format is here:
https://chromium.googlesource.com/chromium/src/+/HEAD/sql/README.md#SQL-styleThe current format have heading spaces which will be forbidden by the style guide soon.
Done
create_payload.BindBlob(1, new_report.trace_content);If this is big (and performance matters), you should use the Blob streaming API.
If it's big and performance don't matters, leave it that way.
I'll do this as a folow-up (since this isn't the bottleneck being fixed here)
UPDATE local_traces_indexEtienne Pierre-DorayDitto for style
Done
std::string received_value = get_content.ColumnString(0);Here again, potentially using streaming API.
See "Find" in the same file linked above.
I'll do this as a folow-up (since this isn't the bottleneck being fixed here)
ditto for style (all of them)
Done
return initialized_ && Transaction.Commit()
This will ensures that not half of a migration / cleanup is done.
it's the recommended practice.
- Anthony Vallée-Dubois
- Etienne Bergeron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthony Vallée-Dubois (Gerrit)
Anthony Vallée-Dubois added 6 comments![]( "Open in Gerrit")
Got a bug to assign here for tracking?
"uuid TEXT PRIMARY KEY NOT NULL,"Any reason why there's no FK constraint between this ID and the PK in the index table?
SQL_FROM_HERE, "DELETE FROM local_traces_payloads WHERE uuid=?"));An FK constraint on the uuid with a `ON DELETE CASCADE` clause would remove the need for this separate statement. If done this way, don't we need to wrap the 2 statements in a transaction too?
if (!database_.Raze()) {I remember Etienne mentioning Raze() being really problematic, but I'm not sure what the proper way to handle this is if it isn't Raze. Maybe jpgravel@ knows.
if (!database_.Execute("DROP TABLE local_traces IF EXISTS")) {Are we OK with losing previous data (not migrating it) during this upgrade?
"DELETE FROM local_traces_payloads "Same comment re: FK + ON DELETE CASCADE making this function redundant.
Related details
- Etienne Bergeron
- Etienne Pierre-Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 5 comments![]( "Open in Gerrit")
Any reason why there's no FK constraint between this ID and the PK in the index table?
Done
SQL_FROM_HERE, "DELETE FROM local_traces_payloads WHERE uuid=?"));An FK constraint on the uuid with a `ON DELETE CASCADE` clause would remove the need for this separate statement. If done this way, don't we need to wrap the 2 statements in a transaction too?
Done
if (!database_.Raze()) {I remember Etienne mentioning Raze() being really problematic, but I'm not sure what the proper way to handle this is if it isn't Raze. Maybe jpgravel@ knows.
+jpgravel@
if (!database_.Execute("DROP TABLE local_traces IF EXISTS")) {Are we OK with losing previous data (not migrating it) during this upgrade?
Yes, for simplicity I don't want to migrate databases.
Same comment re: FK + ON DELETE CASCADE making this function redundant.
- Anthony Vallée-Dubois
- Etienne Bergeron
- Jean-Philippe Gravel
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Got a bug to assign here for tracking?
Done
Related details
- Anthony Vallée-Dubois
- Etienne Bergeron
- Jean-Philippe Gravel
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. |
Anthony Vallée-Dubois (Gerrit)
Anthony Vallée-Dubois voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anthony Vallée-Dubois
| Code-Review | +1 |
2 comments
bool DeleteOrphanedPayloads();This doesn't exist in the cc anymore.
Related details
- Etienne Bergeron
- Etienne Pierre-Doray
- Jean-Philippe Gravel
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
This doesn't exist in the cc anymore.
Done
if (!database_.Raze()) {Etienne Pierre-DorayI remember Etienne mentioning Raze() being really problematic, but I'm not sure what the proper way to handle this is if it isn't Raze. Maybe jpgravel@ knows.
+jpgravel@
Discussed in chat, it sounds like there are issues with Raze, but those will be addressed in the impl of Raze, so nothing to do here.
Related details
- Anthony Vallée-Dubois
- Etienne Bergeron
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Anthony Vallée-Dubois (Gerrit)
Anthony Vallée-Dubois voted Code-Review+1![]( "Open in Gerrit")
- Etienne Bergeron
- Etienne Pierre-Doray
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[tracing] Move payload to a different table in trace reports
SQL queries are slow when checking `trace_content IS NOT NULL`.
The solution is to keep the payloads in a separate table, so that
1-most queries that don't need it aren't slow down
2-Checking if a table has a payload is a LEFT JOIN which is more efficient
- M content/browser/tracing/trace_report_database.cc
Code-Review: +1 by Anthony Vallée-Dubois
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |