Ordering by nullable column can lead to invalid cursor pagination

[benjie]

e.g.

create table p.foo (
  id int primary key,
  name text
);

insert into p.foo (id) values
  (1),
  (2),
  (3),
  (4);

{
  page1: allFoos(first: 2, orderBy: NAME_ASC) {
    nodes {
      id
    }
    pageInfo {
      endCursor
      hasNextPage
    }
    totalCount
  }
  page2: allFoos(
    first: 2
    orderBy: NAME_ASC
    after: "WyJuYW1lX2FzYyIsW251bGwsMl1d"
  ) {
    nodes {
      id
    }
    pageInfo {
      endCursor
      hasNextPage
    }
    totalCount
  }
}

{
  "data": {
    "page1": {
      "nodes": [
        {
          "id": 1
        },
        {
          "id": 2
        }
      ],
      "pageInfo": {
        "endCursor": "WyJuYW1lX2FzYyIsW251bGwsMl1d",
        "hasNextPage": true
      },
      "totalCount": 4
    },
    "page2": {
      "nodes": [],
      "pageInfo": {
        "endCursor": null,
        "hasNextPage": false
      },
      "totalCount": 4
    }
  }
}

base64Decode("WyJuYW1lX2FzYyIsW251bGwsMl1d") === '["name_asc",[null,2]]'

---

The null causes the cursor comparison (a = b OR a > b) to fail since b is null so it is null, so it gets excluded. Need to explicitly handle nulls here, e.g. ((a is null AND a is not distinct from b) OR a = b OR a > b) or something. Will have to think it through carefully, especially how it relates to NULLS FIRST and NULLS LAST.

Originally reported here: https://github.com/graphile-contrib/pg-order-by-related/issues/19

Reproduction thanks to @mattbretl

[benjie]

Pass an isNullable boolean as the third entry in the sort tuple for optimisation

[dargmuesli]

Hey Benjie, what's the current way to work around this? Fall back to offset-based pagination? Thanks for your work!

[benjie]

Yes :+1:

[benjie]

(Or just don't order by nullable fields :wink: )

[nbonavia]

I ran into this exact issue, and I can verify that the query on Postgresql performs an equality (=) operator, where for null values, this will not be a correct check (has to be is null rather than (=)).

[jcgsville]

Instead of passing an isNullable boolean as the third entry in the sort tuple for optimisation, couldn't it just generate a different SQL query if the value in the sort tuple is null? That way the performance hit would only be for pages that start with the null value?

We're running into this problem at Logixboard, and we're looking into forking the plugin to support this. Ordering by nullable fields is a requirement for us 🙂. Could you point us in the right direction where the logic for using the cursor lives? Found this but it doesn't seem to be the place where the sql is actually generated

[benjie]

Start here: https://github.com/graphile/graphile-engine/blob/1bc8cfefdab7a61fd7ad287bcdff66298352e308/packages/graphile-build-pg/src/queryFromResolveDataFactory.js#L122

Hopefully this leads you down the right path; it’s been years since I’ve touched that code and I remember it being Not Fun to get all the edge cases right. Fortunately there’s a lot of integration tests for it so you should know quickly if you break something 👍

[benjie]

This is probably the bit you are most interested in: https://github.com/graphile/graphile-engine/blob/1bc8cfefdab7a61fd7ad287bcdff66298352e308/packages/graphile-build-pg/src/queryFromResolveDataFactory.js#L387

[benjie]

I believe this is fixed in V5.

[benjie]

Basic testing indicates that this is fixed in V5.