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

Revert "Ensure CursorPagination respects nulls in the ordering field" #9381

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

max-muoto
Copy link
Contributor

@max-muoto max-muoto commented Apr 12, 2024

As @kylebebak has mentioned in #9359 attempting to accounting for nulls in the ordering field can lead to a serious performance regression in Postgres due to an index not being hit on the second query. This is blocking upgrades for some to 3.15, so putting out this PR to revert the changes. As he mentioned, it's a pretty uncommon use-case for the ordering field to be null, in the mean-time a reasonable work around would be using some sentinel value to ensure the items come last, or simply not using a nullable column.

Copy link
Contributor

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

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

lgtm.

it's a pretty uncommon use-case for the ordering field to be null

Not only that; the code also doesn't work properly in all edge cases. In particular, when there are several NULL rows (possible with nulls_distinct=True), the cursor position becomes ill-defined. The problem gets worse if those rows don't all fit on one page.

To prevent unpredictable behavior, the code would have to check that the schema doesn't allow this constellation. Alternatively, one could look into index types that work better with IS NULL conditions. However, I'm not convinced that doing all this within DRF is unwarranted. It's fair enough that people have what's documented ("requires that there is a unique, unchanging ordering"), and if someone has a special case, it shouldn't be a big deal to extend the class and adapt the behavior accordingly, within their app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants