postgrest-js icon indicating copy to clipboard operation
postgrest-js copied to clipboard

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

Open GaryAustin1 opened this issue 3 years ago • 2 comments

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 avatar Jan 06 '22 14:01 GaryAustin1

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

kiwicopple avatar Jan 06 '22 15:01 kiwicopple

@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 https://github.com/supabase/supabase-js/issues/170.

Some references:

  • https://stackoverflow.com/questions/45719419/how-to-create-an-index-properly-for-desc-nulls-last-sorting
  • https://dba.stackexchange.com/questions/254731/why-does-order-by-nulls-last-affect-the-query-plan-on-a-primary-key

Default NULLS LAST happens here:

https://github.com/supabase/postgrest-js/blob/ff790caf45cf3a3d8fe84e743600393988fc7122/src/lib/PostgrestTransformBuilder.ts#L40-L44

steve-chavez avatar Jan 06 '22 18:01 steve-chavez

Resolved in https://github.com/supabase/postgrest-js/pull/283

soedirgo avatar Sep 27 '22 07:09 soedirgo