Cleanup Postgres get_all

[glasserc]

This is a more conservative approach towards addressing #1507. In particular, it doesn't introduce any race conditions, and probably reduces query times by much less.

The existing query's performance comes down to three factors:

1. It matches every record in the database that matches the queries, and outputs them in any random order.
2. It then sorts those records according to the order given in the sort order.
3. It does a distinct/join as part of paginating.

The performance characteristics of these steps varies depending on how many records the filters match and how many they don't match, as well as what proportion of the database is under the given parent_id.

1. If most of the database is under this parent, then doing a sequential scan is the fastest way to do this. Otherwise, you want to use an index scan to narrow it down as much as possible before applying filters.
2. Sorting is O(n log n), so if a lot of records match the filters, doing a sort afterwards is expensive. In those cases, you generally want to do an index scan to produce the records in the right order to begin with.
3. Doing a distinct/join is probably O(n log n), so it becomes more expensive the more records match.

Per our discussion in #1616, there are a couple of subtle bugs in the use of wildcard parent_id, which I used as the crowbar to try to pry open this query. First, introduce some tests to firm up the behavior we expect for wildcard parent_ids. It turns out that the third step above introduces a bug in pagination of results when doing a wildcard parent_id match, so just delete it.

Doing the sorting at the end seems wasteful to me; you need to sort anyhow in order to do the pagination, so why not generate records in the right order to begin with? I moved the 2nd step into the first step. This encourages Postgres to use indexes when called for. However, it seems that the benefit to the second step of using an index is outweighed by the cost to the first step of doing an index scan when very few records match, so in a Kinto instance with all records under one parent, doing a query with filters that match comparatively few records, this query is slower than the previous one.

I tested this by filling a Kinto instance with about 10m records, all in the same collection, each of the form {"id": "id-XXXXXX", "my_data": XXXXXX}, and then PATCHing one record to add a "foo": true field to it. I then measured the time of http GET 'localhost:8888/v1/buckets/a/collections/b/records?_limit=10', http GET 'localhost:8888/v1/buckets/a/collections/b/records?_limit=10&has_foo=true', and http GET 'localhost:8888/v1/buckets/a/collections/b/records?_limit=10&has_foo=false'. The first and last requests tend to perform the same, because both match the vast majority of the records in the database; on these, the new query takes about 60s (to the old query's ~90s, so about 33% faster). The second query is a little slower than the previous one (4.52s to 2.65s, or about 80% slower).

Perhaps it's possible to get the benefit of both queries by using heuristics to choose the sequential scan or the index scan, but then we're in the situation of trying to beat the Postgres query planner and I'm not sanguine about our chances. On the other hand, note that the Postgres query planner is hamstrung by the lack of indexing on the data column.

My conclusions are:

• It isn't really clear what the scalability characteristics of Kinto are or should be. Is Kinto meant to store 10m records? I would say probably not. What about 1m records, but all in one collection? I'm not certain.

• It isn't really clear (to me, at least) what queries Kinto should support as "first-class" and which are "gravy", i.e. added bonuses that we threw in because they were easy to implement but are not as well-loved, performant etc. as others. Naïvely I would say syncing collections is more important than supporting arbitrary filters, but then again, if that's what you want, CouchDB is available and may be a better fit for you.

• It isn't really clear to me what the relative priorities are in the Kinto ecosystem. As an example, in order to produce a Total-Records header, we have to add a COUNT to our queries, which means enumerating all the matching records rather than just the ones that we're going to render in the response. In other distributed systems, you might just paginate until you get an empty page. Is the ease-of-use/sanity checking provided by the Total-Records header worth the performance cost?

• Add documentation.

• Add tests.

• Add a changelog entry.

• (n/a) Add your name in the contributors file.

• (n/a) If you changed the HTTP API, update the API_VERSION constant and add an API changelog entry in the docs

• (n/a) If you added a new configuration setting, update the kinto.tpl file with it.

[leplatrem]

Some superficial comments...

[leplatrem]

nit: superfluous parenthesis

[leplatrem]

Maybe add a comment that what happens in reality:
https://github.com/Kinto/kinto/blob/master/kinto/core/resource/__init__.py#L1055-L1057

[leplatrem]

I know it makes no sense to redo that at each loop iteration, but I would have had less trouble reading all this if order_field and pagination_direction would have been define right along the pagination rules below :)

[leplatrem]

Please add a comment like Create records with different parent ids

[leplatrem]

index by memory address? I realize I don't understand why we don't just build a list :)

[glasserc]

Well, I just tried building a list and none of the tests failed. Maybe the person who wrote it originally was afraid that different pagination rules would re-match a record twice? In that case, the tests probably won't fail because we don't construct pagination rules in such a way where this could happen (but a pathological user could conceptually find a way if they were motivated). Since none of the tests fail, I'll take it out..

[leplatrem]

It isn't really clear what the scalability characteristics of Kinto are or should be. Is Kinto meant to store 10m records? I would say probably not. What about 1m records, but all in one collection? I'm not certain.

10m records accross several collections, I would say yes.

1m records in one collection, it should support it, but maybe not in optimal conditions. Since synchronization is going to be painful anyway (especially on the first time).

It isn't really clear (to me, at least) what queries Kinto should support as "first-class" and which are "gravy", i.e. added bonuses that we threw in because they were easy to implement but are not as well-loved, performant etc. as others. Naïvely I would say syncing collections is more important than supporting arbitrary filters, but then again, if that's what you want, CouchDB is available and may be a better fit for you.

I definitely agree that syncing is more important than arbitrary filters (for which we have the ElasticSearch plugin)

It isn't really clear to me what the relative priorities are in the Kinto ecosystem. As an example, in order to produce a Total-Records header, we have to add a COUNT to our queries, which means enumerating all the matching records rather than just the ones that we're going to render in the response. In other distributed systems, you might just paginate until you get an empty page. Is the ease-of-use/sanity checking provided by the Total-Records header worth the performance cost?

That was a naive choice at the beginning of the project. The Total-Records is not even exposed in our clients API...

On the server side, the total count is just used to figure out whether we have a next page. Which, as you say, could be managed differently (ie. paginate until empty page).

[glasserc]

One reason why Total-Records might be nice is to show pagination stuff on the client side. (e.g. if Total-Records is 50, we can show pages 1..5.) We do expose it, not as part of other requests, but as a getTotalRecords call in kinto-http.js. PostgREST handles this by exposing a Content-Range header, but that header doesn't contain a total count unless you add Prefer: count=exact header. They even recommend stripping this header in prod: http://postgrest.org/en/stable/admin.html?highlight=Content-Range#count-header-dos.

[glasserc]

We spoke about this online today. Removing the Total-Records header is kind of an API-breaking change, even though to our knowledge, our clients don't actually rely on this header.

[peterbe]

I'm not sure if it's worth it but we could skip the count part if the request is for the next page. E.g.

$ curl localhost:8888/v1/collection/records
< 200 OK
< Total-Records: 1,234,567
< Next-URL: http://localhost:8888/v1/paginate=somelonghashthing
$ curl http://localhost:8888/v1/paginate=somelonghashthing
< 200 OK
< Total-Records: 1,234,567
< Next-URL: http://localhost:8888/v1/paginate=anotherlonghash

In this case, the Total-Records number could come from the cache rather than having to do another query again.

Granted, I don't really know how the pagination works in Kinto but if the number really is only there for helping pagination then storing the first count in the cache would definitely make it possible to skip it for the next couple of pages.

Too complicated or every-little-helps?

[peterbe]

Another not-crazy idea is to keep a tally count of records per collection and parent. E.g.

if not filters:
    cur.execute("""
      select count from count_records where 
      collection_id=:collection_id and parent_id=:parent_id""", ...)
else:
    # Crap! Have to count them manually.
    cur.execute("""
      select count(*) from records where 
      collection_id=:collection_id and parent_id=:parent_id and
      {filters}
""", ...)
count, = cur.fetchone()  

# Now we have then count, now we just need to the the paginated records
cur.execute("""
  select id, last_modified, data from records 
  WHERE collection_id=:collection_id and parent_id=:parent_id and {filters}
""", ...)
records = cur.fetchmany()

A trigger function would increment and decrement the count_records table.

Obvious caveat (apart from trigger functions being annoyingly subtle) is that if the particular kinto database isn't used ever without complex filters this is a wasted record keeping.

[peterbe]

I extracted the query and compared.
In this database I have 183K records (almost) all in the same parent_id.

Best execution times...

1. The query in master: 3477.01ms
2. This PR: 1313.54ms
3. Two separate queries (count and select): 118.88ms + 1.69ms

With filtering on &has_foo=true which returns 2 rows

1. The query in master: 160.06ms
2. This PR: 356.93ms
3. Two separate queries (count and select): 104.29ms + 322.52ms

This concludes what @glasserc been trying to explain to me; Sometimes the count is expensive when it has to be separate. Sometimes it's cheap when it can count the (few) returned records.

This is also an emphasis about your point above about trying to outsmart the query planner in Postgres.

Not sure if this helps. Either way, we have choices of the best approach and they each depend and fit different environments/contexts.

[peterbe]

🎉🎊 \o/

Can't wait to get this into the Kinto that buildhub is using.

[Natim]

Let's prepare a release then 🌹

[leplatrem]

@glasserc could you add a CHANGELOG entry please?

[glasserc]

Oops, yes, this PR took so long I forgot about the unchecked boxes in the description. I've opened #1635 with a CHANGELOG entry.