Correctly combine sorts in post view cursor-based pagination

[dullbananas]

Fixes #4239

[dessalines]

cc @phiresky

[phiresky]

this is a bit complicated so i have to admit i couldn't understand if it's right at first glance

[dessalines]

I'd say lets wait on this until after the release then... that post_view is already complicated enough without adding macros and that other complicated logic.

[dullbananas]

Keeping the incorrect implementation in the final release would be very bad because it could hide some posts from moderators

[dullbananas]

Also, in a comment on the post about using 0.19 on lemmy.ml, someone said "Ever since the upgrade, lemmy feels dead" which might be caused by this bug making recent posts unlikely to appear (because of the 3rd published time sort) unless either New sort type is being used or enough people using old pagination are discovering and upvoting them to make them more likely to appear with the incorrect cursor-based pagination

[dessalines]

I'm not against merging then, but I'd need to get another RC out to make sure this doesn't slow down anything, before we do the release. cc @phiresky

[SleeplessOne1917]

This would benefit from deriving Debug, PartialEq, and Eq as well.

[dullbananas]

Done

[phiresky]

It's a shame that diesel doesn't support tuple comparison. That would make this much simpler and easy to see that the performance is still fine. The discussion of that feature is here: diesel-rs/diesel#3750

So this approach is the workaround for not being able to simply do WHERE (a,b) < ('foo', 123).

So trying to understand this: Instead of doing (a,b,c)>=(cursora,cursorb,cursorc).

the resulting filter is: (a >= cursora) AND (b >= cursorb OR a != cursora) AND (c >= cursorc OR a != cursora OR b != cursorb).

Which is confusing but seems to make sense. Easier to understand for me would be
a >= cursora OR (a = cursora AND b >= cursorb) OR (a = cursora AND b = cursorb AND c >= cursorc).

Here's three posts about the approach implemented here:

• https://stackoverflow.com/questions/60433647/sql-query-for-pagination-with-multiple-columns-understand-or-operator
• https://medium.com/@gdr2409/pagination-with-postgresql-18e0b89e0b1c
• https://brunoscheufler.com/blog/2022-01-01-paginating-large-ordered-datasets-with-cursor-based-pagination

So it does seem like a fairly common solution.

I couldn't find any info about whether Postgresql is able to optimize this or not though which may make it unviable... tbh I doubt it can figure out all those ORs.

Another alternative would probably be to use diesel raw sql. That should look something like

query = query.filter(
   sql::<Bool>("(featured_local, hot_rank, published) >= (")
   .bind::<Bool,_>(cursor.featured_local)
   .sql(", ")
   .bind::<Double,_>(cursor.hot_rank)
   .sql(", ")
   .bind::<Timestamptz,_>(cursor.published)
   .sql(")")
)

It's disgusting but I think that might be better than this... and has pretty much guarantee that PG optimizes it correctly since it's 1:1 a slice of the corresponding index.
What do you think @dullbananas ?

[phiresky]

It would actually not be that different from this PR, just that loop would be replaced and the macros could be removed (i think)

[dullbananas]

I will try to make it work using tuple comparisons. DESC doesn't work for comparisons, so I guess I would need to negate the descending columns and do the same thing in each index.

I will do it in a separate PR so @dessalines can choose to either wait for it to be done or use the implementation in this PR for now.

[phiresky]

right, I didn't think about mixed asc and desc :(

[dessalines]

I spose since we're in a rush to get a release out, I'll merge this so we can test it in a release candidate, but ya do work on the alternate version.

[dessalines]

I've got this deployed into 0.19.0-rc.16, and on lemmy.ml, so we have some time to see how its working.

[Nutomic]

@dullbananas Even though this is merged, it would still be good if you can make a PR to change this to tuple comparisons instead of macros. That way it should be much easier to maintain.

[dullbananas]

I will try to make a library that would allow the post view to do order and pagination with very simple method calls on a struct that wraps the query. You're gonna love it.

Because of the need for the negation hack for mixed sorting directions, I will avoid using tuple comparison in the implementation unless it's necessary for a good query plan. And if tuple comparison is needed, then I will check whether or not indexes need to be changed. (Edit: it could also just use tuple comparison for parts of the sort that have the same sorting direction.)

I can probably also implement backwards pagination.