-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
perf: Cache dbTables FuzzySet per schema #4472
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
base: main
Are you sure you want to change the base?
Conversation
3149071 to
a1b92a6
Compare
|
I think I've managed to repro, with 125K tables: The server dies when trying to compute the hint: $ curl localhost:3000/xprojects
curl: (52) Empty reply from server
# correct table names do work
$ curl localhost:3000/projects
[{"id":1,"name":"Windows 7","client_id":1},
{"id":2,"name":"Windows 10","client_id":1},
{"id":3,"name":"IOS","client_id":2},
{"id":4,"name":"OSX","client_id":2},
{"id":5,"name":"Orphan","client_id":null}]$ |
Same result with this PR. I'll add a test case. |
Yeah... this PR won't help with the first hint computation (FuzzySet is created anyway). It would only help with subsequent hints (without this PR FuzzySet is created each time). Seems like FuzzySet implementation is memory hungry. Also - it depends on the number of schemas as with this PR there is a separate FuzzySet per schema created (doesn't help if all tables are in a single schema). |
|
@steve-chavez My suggestion would be to either:
Aside: OTOH I understand that if we already have the schema cache then let's use it - trade-offs everywhere :) |
+1 for this also from a dependency perspective: We have to pin an older version of the fuzzy-thing dependency, because a newer version causes even more regression. I'd like to get rid of that dependency, ideally. |
From my experience maintaining a postgREST-as-a-service, before the fuzzy search hint there were lots of support tickets that came from typos but users were quick to to blame postgREST and suspect a malfunction. It was a huge time sink. After the fuzzy search, those complaints just stopped. Nowadays with typescript clients and other typed clients we have in the ecosystem, maybe this is not such a big problem but I'd still argue have the fuzzy search is a net win for 99% use cases.
This looks like the viable short-term solution. Most use cases don't have that many tables too.
IIRC, #3869 was mostly for better error messages, but we have other features that might also reuse it. To not use the schema cache we'd have to implement resource embedding and other features in PostgreSQL itself (possible but would take a loot of time).
Could we instead vendor the dependency? 🤔 (see laserpants/fuzzyset-haskell#9) |
Besides the above, I'm also getting an "empty reply from server" on
Looking at the algorithms used as inspiration on laserpants/fuzzyset-haskell#2 (comment), I wonder if it's optimized for our use case. Maybe we can come up with a better library? (cc @taimoorzaeem) |
Makes sense. [...]
I think it makes sense to merge this PR regardless of these decisions as it is a quite obvious optimization (even though it does not fix the OOM issue). |
I don't think we should merge without a test that proves what's being improved (since it doesn't solve #4463). |
I will be happy to write a library 👍 . I would just need to do some feasibility, maybe a better library already exist on hackage? Needs some investigation. |
a1b92a6 to
20b391f
Compare
Done. See: https://github.com/PostgREST/postgrest/actions/runs/19477317041/attempts/1#summary-55740304508 |
As I mentioned earlier - I've done some research and I couldn't find any interesting alternatives. Fuzzy search algorithms are divided into two subgroups: online and offline. Online algorithms do not require additional memory but require scanning the whole list (so are Offline algorithms require creating an index. Best results are achieved using indexes based on n-grams - this is exactly what I am skeptical we can come up with a solution to 125k tables... |
20b391f to
2057eaa
Compare
@mkleczek We should not modify the mixed loadtest for this, that should stay the same across versions so we can see regressions. Additionally, that doesn't prove there's an enhancement here. Test should be like:
|
I am not sure I understand this. The old So I would say adding the "table not found" scenario to it is an important addition - it makes it more realistic and covering wider set of scenarios.
https://github.com/PostgREST/postgrest/actions/runs/19477317041/attempts/1#summary-55740304508 Am I missing something here?
Hmm... I thought load tests (not @steve-chavez could you advice on the way you would like it to be tested? |
The problem is that "mixed" is already conflating too many scenarios (see #4123), with this addition is even worse as we also conflate "successful traffic" with "error traffic" (note that now we have Another problem is that loadtests are slow, so we shouldn't add too many of them (IMO a dedicated one for errors doesn't seem worth it) if we can instead have an io test.
We do have perf related tests on: postgrest/test/io/test_big_schema.py Lines 10 to 36 in 91abcd4
It looks enough to have an upper bound of expected time for computing the hint to not have regressions.
I'd suggest adding the fixtures on |
ff6d3eb to
c2e1d94
Compare
@steve-chavez |
test/io/test_big_schema.py
Outdated
|
|
||
|
|
||
| def test_second_request_for_non_existent_table_should_be_quick(defaultenv): | ||
| "requesting a non-existent relationship the second time should be quick" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Why is the second time quicker? Does the fuzzy index get populated after hitting an error?
From the code, it looks like the fuzzy index is only populated when the schema cache is built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Why is the second time quicker? Does the fuzzy index get populated after hitting an error?
From the code, it looks like the fuzzy index is only populated when the schema cache is built.
That's because of Haskell laziness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then to avoid confusion I suggest:
| "requesting a non-existent relationship the second time should be quick" | |
| "requesting a non-existent relationship should be quick after the schema cache is loaded (2nd request)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, not sure if I understand this.
When we do a request with resource embedding the schema cache is already loaded and works on the first request. Why does this change for the fuzzy index and we need to make 2 requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe what we need is some comments about this optimization, I suggest adding those on the type definition #4472 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, not sure if I understand this.
When we do a request with resource embedding the schema cache is already loaded and works on the first request. Why does this change for the fuzzy index and we need to make 2 requests?
This is indeed tricky: schema cache is loaded lazily as well. But there are these two lines in AppState.retryingSchemaCacheLoad:
(t, _) <- timeItT $ observer $ SchemaCacheSummaryObs $ showSummary sCache
observer $ SchemaCacheLoadedObs t
which cause evaluation of SchemaCache fields.
I've decided not to add dbTablesFuzzyIndex to schema cache summary and leave its evaluation till first use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced the type alias, added comments to SchemaCache field and updated the test description.
c2e1d94 to
ee52b3f
Compare
f8d5cf5 to
6226416
Compare
6226416 to
ccd9664
Compare
Calculation of hint message when requested relation is not present in schema cache requires creation of a FuzzySet (to use fuzzy search to find candidate tables). For schemas with many tables it is costly. This patch introduces dbTablesFuzzyIndex in SchemaCache to memoize the FuzzySet creation.
ccd9664 to
465d076
Compare
| with run(env=env, wait_max_seconds=30) as postgrest: | ||
| response = postgrest.session.get("/unknown-table") | ||
| assert response.status_code == 404 | ||
| data = response.json() | ||
| assert data["code"] == "PGRST205" | ||
| first_duration = response.elapsed.total_seconds() | ||
| response = postgrest.session.get("/unknown-table") | ||
| assert response.elapsed.total_seconds() < first_duration / 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed something strange on my system. When I run the test with get endpoint changed to /table-with-a-weird-name, the test fails with an exception.
FAILED test/io/test_big_schema.py::test_second_request_for_non_existent_table_should_be_quick
- requests.exceptions.ConnectionError: ('Connection aborted.', RemoteDisconnected(
- 'Remote end closed connection without response'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taimoorzaeem I'm afraid the same thing happens when running this test against main.
This is expected as this PR only makes sure the FuzzySet is created once instead of every time hint is calculated. Hint calculation logic and the data structures stay the same.
IMHO it looks like the library we use for fuzzy search has reliability issues and we should look for other solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taimoorzaeem I found https://hackage-content.haskell.org/package/fuzzily-0.2.1.0/docs/Text-Fuzzily.html and https://hackage.haskell.org/package/fuzzyfind-3.0.2/docs/Text-FuzzyFind.html but they both implement online fuzzy search. For us means that hint calculation time is at least linear in the number of tables (which I don't think is a good idea as it will almost certainly fail with timeout for large schemas).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most scalable solution would be to implement the
SymSpellalgorithm. See: ref1, ref2, ref3.
The problem with SymSpell is that it is memory hungry (ie. for 100,000 words and edit distance of 2 it requires 1,500,000 entries in the dictionary). Building the index is also very costly.
You can minimize memory requirements with perfect hashing but it makes building the index even more costly.
In general, this is a tough problem. It is even tougher if the spelling dictionary is dynamic (relation names in the schema cache can be reloaded and require index rebuilding).
I think a haskell package for this would be great for the entire haskell community.
Undoubtedly. You can read about SOTA techniques for example here: https://towardsdatascience.com/spelling-correction-how-to-make-an-accurate-and-fast-corrector-dc6d0bcbba5f/
Implementing this in Haskell would be a very interesting task.
Having said that, when you think about the whole architecture from the high level, PostgREST does not seem to be the right place to implement in-memory text search engine. Especially that it is a component that is supposed to be scaled easily (ie. start new instances quickly). In such scenarios you want to externalize state, ie. have a separate component implementing text search algorithms. But you already have such a component: PostgreSQL itself!
IMHO these are possible paths for PostgREST:
- Stay with current architecture (ie. query building based on in-memory schema cache outside of the database) and implement limited but cheap spell checking.
- Move query building to the database itself and use database facilities to handle spell checking (somewhat not a viable solution as it would be a complete rewrite).
- Stay with current architecture and externalize spell checking
- by invoking external system when calculating hint (there are multiple questions here: should it be some specialized search engine - we don't want to have such a dependency. should it be PostgreSQL - then see below)
- by letting PostgreSQL handle misspelled relations (that in essence means reverting fix: handle queries on non-existing table gracefully #3869)
It just seems to me we've just hit the wall with our current architecture here and the only thing we can do is to admit it and live with deficiency in spell checking or re-architect and rewrite PostgREST.
One mitigation would be to work on fuzzyset library and try to optimize it as much as possible (I've taken a quick look at the source and found some minor optimization opportunities). The question is really: what schema sizes are "normal" for PostgREST and what sizes are outside of what PostgREST supports?
@taimoorzaeem @steve-chavez does the above make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great analysis Michal! Many options to go from here, but I think the first thing we need to do is to have reproducible production level testing setup, only then we can take a sure fire decision.
Calculation of hint message when requested relation is not present in schema cache requires creation of a FuzzySet (to use fuzzy search to find candidate tables). For schemas with many tables it is costly. This patch introduces dbTablesFuzzyIndex in SchemaCache to memoize the FuzzySet creation.
Related to #4463