Count items in database as much as possible #344

trinity-1686a · 2018-12-09T19:26:27Z

elegaanz commented

2018-12-09 19:26:27 +00:00

(Migrated from github.com)

No description provided.

👍 1

codecov[bot] commented

2018-12-09 20:30:47 +00:00

(Migrated from github.com)

Codecov Report

Merging #344 into master will increase coverage by 0.14%.
The diff coverage is 21.62%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   28.56%   28.71%   +0.14%     
==========================================
  Files          62       62              
  Lines        5601     6165     +564     
==========================================
+ Hits         1600     1770     +170     
- Misses       4001     4395     +394

Impacted Files	Coverage Δ
src/routes/user.rs	`0% <0%> (ø)`	⬆️
plume-models/src/posts.rs	`12.01% <0%> (-0.13%)`	⬇️
plume-models/src/notifications.rs	`0% <0%> (ø)`	⬆️
src/routes/blogs.rs	`0% <0%> (ø)`	⬆️
plume-models/src/db_conn.rs	`0% <0%> (ø)`	⬆️
src/main.rs	`0% <0%> (ø)`	⬆️
plume-models/src/comments.rs	`0% <0%> (ø)`	⬆️
src/routes/posts.rs	`0% <0%> (ø)`	⬆️
src/routes/comments.rs	`0% <0%> (ø)`	⬆️
src/routes/instance.rs	`0% <0%> (ø)`	⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61b6cee...d418093. Read the comment docs.

# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/344?src=pr&el=h1) Report > Merging [#344](https://codecov.io/gh/Plume-org/Plume/pull/344?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/61b6ceed92ef838a085da6eb66f87ab1de65a520?src=pr&el=desc) will **increase** coverage by `0.14%`. > The diff coverage is `21.62%`. [![Impacted file tree graph](https://codecov.io/gh/Plume-org/Plume/pull/344/graphs/tree.svg?width=650&token=sHtxmDuZ06&height=150&src=pr)](https://codecov.io/gh/Plume-org/Plume/pull/344?src=pr&el=tree) ```diff @@ Coverage Diff @@ ## master #344 +/- ## ========================================== + Coverage 28.56% 28.71% +0.14% ========================================== Files 62 62 Lines 5601 6165 +564 ========================================== + Hits 1600 1770 +170 - Misses 4001 4395 +394 ``` | [Impacted Files](https://codecov.io/gh/Plume-org/Plume/pull/344?src=pr&el=tree) | Coverage Δ | | |---|---|---| | [src/routes/user.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcy91c2VyLnJz) | `0% <0%> (ø)` | :arrow_up: | | [plume-models/src/posts.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-cGx1bWUtbW9kZWxzL3NyYy9wb3N0cy5ycw==) | `12.01% <0%> (-0.13%)` | :arrow_down: | | [plume-models/src/notifications.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-cGx1bWUtbW9kZWxzL3NyYy9ub3RpZmljYXRpb25zLnJz) | `0% <0%> (ø)` | :arrow_up: | | [src/routes/blogs.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcy9ibG9ncy5ycw==) | `0% <0%> (ø)` | :arrow_up: | | [plume-models/src/db\_conn.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-cGx1bWUtbW9kZWxzL3NyYy9kYl9jb25uLnJz) | `0% <0%> (ø)` | :arrow_up: | | [src/main.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-c3JjL21haW4ucnM=) | `0% <0%> (ø)` | :arrow_up: | | [plume-models/src/comments.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-cGx1bWUtbW9kZWxzL3NyYy9jb21tZW50cy5ycw==) | `0% <0%> (ø)` | :arrow_up: | | [src/routes/posts.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcy9wb3N0cy5ycw==) | `0% <0%> (ø)` | :arrow_up: | | [src/routes/comments.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcy9jb21tZW50cy5ycw==) | `0% <0%> (ø)` | :arrow_up: | | [src/routes/instance.rs](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcy9pbnN0YW5jZS5ycw==) | `0% <0%> (ø)` | :arrow_up: | | ... and [18 more](https://codecov.io/gh/Plume-org/Plume/pull/344/diff?src=pr&el=tree-more) | | ------ [Continue to review full report at Codecov](https://codecov.io/gh/Plume-org/Plume/pull/344?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/344?src=pr&el=footer). Last update [61b6cee...d418093](https://codecov.io/gh/Plume-org/Plume/pull/344?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

trinity-1686a approved these changes 2018-12-13 21:05:05 +00:00

trinity-1686a left a comment

Things I commented are further optimizations possible, however they're not strictly needed to be implemented for this to be merged. Actually I don't even know if it would give better performance as optimizations I proposed can be proved equivalent to current implementation with what DBMSs know about our model, so they might do the optimization themselves

plume-models/src/blogs.rs

trinity-1686a commented

2018-12-13 20:32:26 +00:00

I wonder if the second query is useful, I think count()ing on the first one is sufficient, except in case where :

multiple users have the same id, which is guaranteed to never happen
an author_id exist in blog_authors but not in users, which is guaranteed to never happen too
more than one couple (blog,author) with the same blog&author exist (but different entry id), which is guaranteed to never happen since the constraint blog_author_unique was added (only apply if .select(blog_authors::author_id) is removed too, if it is not then multiple (id_X,blog_1,author_1) would be mapped to only one (author_1) anyway. Hope I'm clear enough)

I wonder if the second query is useful, I think `count()`ing on the first one is sufficient, except in case where : - multiple users have the same id, which is guaranteed to never happen - an author_id exist in blog_authors but not in users, which is guaranteed to never happen too - more than one couple (blog,author) with the same blog&author exist (but different entry id), which is guaranteed to never happen since the constraint blog_author_unique was added (only apply if `.select(blog_authors::author_id)` is removed too, if it is not then multiple (id_X,blog_1,author_1) would be mapped to only one (author_1) anyway. Hope I'm clear enough)

plume-models/src/notifications.rs

trinity-1686a commented

2018-12-13 20:34:59 +00:00

ordering then counting is useless, I guess PostgreSQL would see it and optimize that away, but I also think SQLite is too dumb to do the same, so it might be shorter and faster to remove that

elegaanz (Migrated from github.com) reviewed 2018-12-13 21:15:59 +00:00

plume-models/src/notifications.rs

elegaanz (Migrated from github.com) commented

2018-12-13 21:15:58 +00:00

Yes, I just copy/pasted another function. I'll change that.

elegaanz (Migrated from github.com) reviewed 2018-12-13 21:17:50 +00:00

plume-models/src/blogs.rs

elegaanz (Migrated from github.com) commented

2018-12-13 21:17:50 +00:00

OK, I'll change it (and yes you were clear enough 🙂)

OK, I'll change it (and yes you were clear enough :slightly_smiling_face:)

trinity-1686a approved these changes 2018-12-13 21:23:32 +00:00

trinity-1686a commented

2018-12-14 08:23:19 +00:00

Love it when an issue is only reproducible on one db backend. It might be because sqlite don't enforce some constraints by default, we could enabled PRAGMA foreign_keys = ON;, but I wonder what changes to make this test fail

Love it when an issue is only reproducible on one db backend. It might be because sqlite don't enforce some constraints by default, we could enabled `PRAGMA foreign_keys = ON;`, but I wonder what changes to make this test fail

igalic commented

2018-12-14 10:15:19 +00:00

(Migrated from github.com)

It might be because sqlite don't enforce some constraints by default, we could enabled PRAGMA foreign_keys = ON;

we have that enabled — on the users table — not sure if/how we enable it by default.

, but I wonder what changes to make this test fail

maybe we're finally using that constraint? 😅

> It might be because sqlite don't enforce some constraints by default, we could enabled `PRAGMA foreign_keys = ON;` we have that enabled — on the `users` table — not sure if/how we enable it by default. > , but I wonder what changes to make this test fail maybe we're finally using that constraint? 😅

trinity-1686a commented

2018-12-14 10:22:54 +00:00

@igalic I think we only set it in a migration, this property is not stored so we need to set it somehow at each Plume launch, and this is a global var, so it was enabled for all tables (but only until the migration end)

igalic commented

2018-12-14 13:15:20 +00:00

(Migrated from github.com)

this property is not stored

unlike (some) other pragma instructions, it's not storable… it's, as you say, per connection or per transaction.

So perhaps the diesel sqlite driver has something we can turn on by default

i checked, it can't
but maybe we can wrap the query functions with a begin transaction; pragma foreign_keys = on;

> this property is not stored unlike (some) other `pragma` instructions, it's not storable… it's, as you say, per connection or per transaction. So perhaps the diesel sqlite driver has something we can turn on by default ------ i checked, it can't but maybe ***we*** can wrap the query functions with a `begin transaction; pragma foreign_keys = on;`

trinity-1686a reviewed 2018-12-14 19:06:32 +00:00

plume-models/src/db_conn.rs

					
				@ -41,0 +48,4 @@

				        sql_query("PRAGMA foreign_keys = on;").execute(conn)

				            .map(|_| ())

				            .map_err(|_| ConnError::ConnectionError(ConnectionError::BadConnection(String::from("PRAGMA foreign_keys = on failed"))))

				    }

trinity-1686a commented

2018-12-14 19:06:32 +00:00

oh, I was searching for exactly that but did not find it

elegaanz (Migrated from github.com) reviewed 2018-12-14 19:23:28 +00:00

plume-models/src/db_conn.rs

					
				@ -41,0 +48,4 @@

				        sql_query("PRAGMA foreign_keys = on;").execute(conn)

				            .map(|_| ())

				            .map_err(|_| ConnError::ConnectionError(ConnectionError::BadConnection(String::from("PRAGMA foreign_keys = on failed"))))

				    }

elegaanz (Migrated from github.com) commented

2018-12-14 19:23:27 +00:00

~~Looks like it is not working at all~~ 🙃 nvm I think I found the issue

~~Looks like it is not working at all~~ :upside_down_face: nvm I think I found the issue

trinity-1686a reviewed 2018-12-14 19:38:37 +00:00

plume-models/src/db_conn.rs

					
				@ -41,0 +48,4 @@

				        sql_query("PRAGMA foreign_keys = on;").execute(conn)

				            .map(|_| ())

				            .map_err(|_| ConnError::ConnectionError(ConnectionError::BadConnection(String::from("PRAGMA foreign_keys = on failed"))))

				    }

trinity-1686a commented

2018-12-14 19:38:37 +00:00

I think your changes are only affecting the web interface, which is at the same time the most important part, and the one not tested by travis 🙃
I've tested doing similar changes in plume_model::lib:tests::db (adding sql_query("PRAGMA foreign_keys = on;").execute(&conn).unwrap();) which seems to work fine

I think your changes are only affecting the web interface, which is at the same time the most important part, and the one not tested by travis :upside_down_face: I've tested doing similar changes in plume_model::lib:tests::db (adding `sql_query("PRAGMA foreign_keys = on;").execute(&conn).unwrap();`) which seems to work fine

trinity-1686a reviewed 2018-12-14 22:11:02 +00:00

plume-models/src/blogs.rs

trinity-1686a commented

2018-12-14 22:11:02 +00:00

"bfore", "after bfore", "after after", then is it "after after bfore" or "after after after"?
(joking about the comments, but they should be removed actually, I guess you forgot to)

"bfore", "after bfore", "after after", then is it "after after bfore" or "after after after"? (joking about the comments, but they should be removed actually, I guess you forgot to)

elegaanz (Migrated from github.com) reviewed 2018-12-14 22:15:21 +00:00

plume-models/src/blogs.rs

elegaanz (Migrated from github.com) commented

2018-12-14 22:15:20 +00:00

Oops 😬

Oops :grimacing:

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Plume/Plume#344

Rows
Columns