Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post view: move cursor pagination to separate library, add backward pagination to PostQuery #4320

Merged
merged 26 commits into from
Jan 24, 2024

Conversation

dullbananas
Copy link
Collaborator

@dullbananas dullbananas commented Dec 22, 2023

The library uses tuple comparison.

The new page_back option causes the page to be before the cursor instead of after. It's possible to get the posts at the end by enabling it without providing the cursor (without this, going to the previous page would be impossible when viewing the empty page that appears when all posts have been viewed).

Includes the change that was in #4300

@dullbananas dullbananas marked this pull request as ready for review December 27, 2023 05:05
Copy link
Collaborator

@phiresky phiresky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! The changed code here looks clean. The code in the library seems complex. A few concerns. Could you maybe post some example queries this generates? The code in the library is a bit hard to follow. Especially I don't understand the ORDER BY row_number() OVER () DESC" and the need for a subquery.

@@ -36,6 +36,7 @@ full = [
"tokio-postgres",
"tokio-postgres-rustls",
"rustls",
"i-love-jesus",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be renamed to something more descriptive? like diesel-tuple-pagination maybe. Also not sure what dessalines/nutomic think of depending on an external library like this, since it does seem fairly specific to lemmy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will not rename my library

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't like the extra complication cursor pagination requires in general, with all of these pre-fetching of bounds, macros, etc. It makes post_view.rs so much harder to read and understand even to me.

Every example I can find online of cursor pagination is where you are sorting based off a single id or time column, rather than multiple filter and sort columns like we have. Its unfortunate that limit and offset are such a performance burden, since they require none of this complication, but I spose there's no way around it.

Whether that complicated cursor logic lives in lemmy, or elsewhere where it can be used by other projects, doesn't matter too much to me, but the latter seems better if it helps other diesel projects besides ours. We're dependent on a lot of diesel libraries that aren't made by its maintainer.

As far as the name, it doesn't bother me personally, but I could see other people not wanting to use it because of the name. For limited-scope libraries it makes sense to name the thing what it does, and it seems like a lot of other diesel projects are named things like diesel-.... or ...-diesel .

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarification, the prefetching is unrelated to the cursor pagination - I just added it in the same PR because it was a convenient related change. It's a performance boost for the post query in general with the "subscribed" filter, even for the first page where the pagination method is irrelevant. For the page-based pagination the same method could in fact be used for both sides of the page so the db doesn't have to join everything while trying to find the page start and end.

@@ -0,0 +1,12 @@
CREATE FUNCTION reverse_timestamp_sort (t timestamp with time zone)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since timestamptz is a 8-byte column but numeric is a variable-size column it might be good if this also returned of 8-byte size. for example bigint ((-extract(epoch from t) * 1000)::bigint)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@dullbananas
Copy link
Collaborator Author

Amazing! The changed code here looks clean. The code in the library seems complex. A few concerns. Could you maybe post some example queries this generates? The code in the library is a bit hard to follow. Especially I don't understand the ORDER BY row_number() OVER () DESC" and the need for a subquery.

I just added better comments in the library, so it should be a little less confusing.

For normal forward pagination, it just sorts by the columns and adds a filter with tuple comparison. In this case, wrapping it in SELECT * FROM (...) does nothing and is only done because I only put the if statement around the part that adds the additional order-by clause.

The limit_and_offset_from_end option for backwards pagination is complicated because PostgreSQL has no built-in way to get the last rows instead of the first, so the library does this by reversing the inner query by replacing descending sorts with ascending sorts, then reversing the final result using SELECT * FROM (...) ORDER BY row_number() OVER () DESC. The 2nd reversal cancels out the 1st, but the limit is still backwards.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me, I'd want others to look at it tho too.

crates/db_views/src/post_view.rs Outdated Show resolved Hide resolved
@dessalines
Copy link
Member

@phiresky when you get a chance, look at this one too.

@dessalines dessalines enabled auto-merge (squash) January 24, 2024 15:27
@dessalines dessalines merged commit d8f9e8a into LemmyNet:main Jan 24, 2024
1 check passed
@Nutomic Nutomic mentioned this pull request Apr 8, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants