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

Supabase default of nullsLast can cause performance issues with ascending:false. #239

Closed
GaryAustin1 opened this issue Jan 6, 2022 · 3 comments

Comments

@GaryAustin1
Copy link

Bug report

Describe the bug

I'm entering this as a bug as it is not expected default behavior, but it could be feature request or documentation enhancement.

There can be a performance issue for large tables with an index and using limit when ordered by descending because of default choice enforced by postgrest.js.

In Postgres NULLS FIRST is the default for ORDER BY DESC and NULLS LAST is default for the default ORDER BY ASC.

In PostgREST nullsfirst or nullslast is an optional parameter and would allow Postgres to sort by default order.

Supabase postgrest.js defaults to nullslast and can only be over ridden with a nullsFirst:true option.

In the case of using ascending:false option in Supabase.js (postgrest.js), nullslast is sent to postgREST by default and then Postgres will not use the default NULLS FIRST.

Many users will not even be thinking of nullsfirst or nullslast as default normally works fine.

To Reproduce

The issue is discussed and "resolved" in this thread on discord:

https://discord.com/channels/839993398554656828/928590123011031050

Expected behavior

Either it should be documented that it is recommended to also set nullsFirst:true when using ascending:false and why
OR better postgrest.js should take optional nullsFirst AND nullsLast parameter so default is not specifying and letting PostgREST and Postgres make their default choices.

Screenshots

image

System information

Lastest Supabase.

Additional context

Add any other context about the problem here.

@GaryAustin1 GaryAustin1 changed the title Jan 6, 2022
@kiwicopple
Copy link
Member

cc @steve-chavez - thoughts? I'm happy to document this one if you prefer

@steve-chavez steve-chavez transferred this issue from supabase/supabase Jan 6, 2022
@steve-chavez
Copy link
Member

steve-chavez commented Jan 6, 2022

@kiwicopple I think postgrest-js should not default to a NULLS LAST, otherwise the intuitive way of creating an index to speed up order won't work:

CREATE INDEX index_name ON TABLE table_name (date)
-- they'd have to create it as
-- CREATE INDEX index_name ON TABLE table_name (date DESC NULLS LAST)

better postgrest.js should take optional nullsFirst AND nullsLast parameter so default is not specifying and letting PostgREST and Postgres make their default choices.

Agree, this would be a breaking change in postgrest-js though. Something for supabase-js 2.0 supabase/supabase-js#170.

Some references:

Default NULLS LAST happens here:

order(
column: keyof T,
{
ascending = true,
nullsFirst = false,

soedirgo added a commit that referenced this issue May 31, 2022
BREAKING CHANGE: nullsFirst = true by default

We previously set `nullsFirst = false` by default, but it caused some
perf issues: #239
soedirgo added a commit that referenced this issue Jun 1, 2022
BREAKING CHANGE: nullsFirst = true by default

We previously set `nullsFirst = false` by default, but it caused some
perf issues: #239
soedirgo added a commit that referenced this issue Jun 7, 2022
BREAKING CHANGE: nullsFirst = true by default

We previously set `nullsFirst = false` by default, but it caused some
perf issues: #239
soedirgo added a commit that referenced this issue Aug 5, 2022
BREAKING CHANGE: nullsFirst = true by default

We previously set `nullsFirst = false` by default, but it caused some
perf issues: #239
@soedirgo
Copy link
Member

soedirgo commented Sep 27, 2022

Resolved in #283

leohku pushed a commit to leohku/postgrest-js that referenced this issue Jul 13, 2023
BREAKING CHANGE: nullsFirst = true by default

We previously set `nullsFirst = false` by default, but it caused some
perf issues: supabase#239
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

No branches or pull requests

4 participants