-
-
Notifications
You must be signed in to change notification settings - Fork 569
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
Simple query causing massive memory usage (pushing over the half gigabyte level allowed by Heroku) #312
Comments
So I've downloaded the data from my staging environment and am running the dev version of PostGraphQL locally. I have 16GB of RAM whereas Heroku only gives 512MB so it's not a very fair comparison, and I have to monitor the memory very fast to see the issue. In order to do so, I've set up this loop (OSX, so using gdate from coreutils):
This has given me quite a high resolution file exposing the memory usage. I'm going to build a graph from it and upload... |
Setting
|
The important part of that seems to be:
The commit (last line) didn't fire until 2 seconds after the explain suggesting the memory allocation/slow processing is happening after postgres has returned the data whilst it is being processed by postgraphql |
So running the second query returns 72MB of data. $ echo 'select to_json(__local_0__) as value from "hookhaven_data"."sketch_integration_event_run" as __local_0__ where "id" is not null and true and true and ("sketch_id" = 11) order by "id" using > limit 100' | time psql bots_staging | wc -c
72609787
psql bots_staging 6.36s user 0.13s system 75% cpu 8.646 total
wc -c 0.29s user 0.04s system 3% cpu 8.645 total So it seems that the issue here could be fixed along with #265: basically we're requesting data that we don't need, and if that data is large then it can cause massive performance/memory allocation issues. |
This issue seems to match what you're hitting: brianc/node-postgres#1103 How many fields are you selecting? This may be fixable by not using |
You posted the comment before I did 😉 I'm going to look into fixing this. It's high priority so I'll see what I can do. |
I've been looking into #265 for a while; it seems this is a good starting point. Basically we want to remove the resolve(source, args, {fieldASTs}) {
// Look up SQL columns (and computed columns!) required by fieldASTs
// and add them to the select right here
} which instead of doing
I've not yet dug into |
@calebmer Why are we doing |
(Using |
We use The first fix I want to implement is just selecting the columns that are requested when there are no procedures. So for now at least a procedure column will cause a de-opt as we revert to the |
I use computed columns heavily, but fortunately not in this one case. (I do have performance issues related to computed columns, but they're not as problematic as this one is.) |
Ok. Computed columns are definitely something I really want to make fast as well given how great they are for the developer experience. Out of curiosity do you use a lot of connection computed columns? Those will be pretty tricky to optimize and may always require a de-opt. |
I'm going to keep taking notes here as I dig further into understanding the codebase (something I've been meaning to do for a while). Using the awesome |
Confirmed: in v0.8.0 fieldASTs was renamed to fieldNodes: |
Awesome 👍 I will note the approach I’m taking here. Because the GraphQL and Postgres parts of the codebase are kept separate (and intentionally so) I’m going to add a optimizations object which the GraphQL adapter can pass through the interface. The optimizations object will contain all of the fields selected in GraphQL. My current designs for an optimizations object depend on the changes in #281. So I’m rushing to finish that first. Out of curiosity, does |
Looks like resolve takes 4 arguments: |
Good question, I shall investigate... |
New query, with an inline and an external fragment and a relation lookup: {
sketchById(id: 11) {
sketchIntegrationEventRunsBySketchId(orderBy: ID_DESC, first: 100) {
edges {
node {
__id
... on SketchIntegrationEventRun {
createdAt
}
... F0
}
}
}
}
}
fragment F0 on SketchIntegrationEventRun {
id
sketchBySketchId {
...F1
}
}
fragment F1 on Sketch {
updatedAt
} It seems it does not lookup the fragments for you, you'll need to resolve them yourself: |
Looks like |
Unfortunately I timed out on this. I’m not sure when I’m going to get the chance to work on it again. In order to solve some of the major issues like this one I’m going to need more community support. Either fiscally or in code contributions. |
I'm guessing this is fixed in v4 so I'm going to tag it as so - if I accidentally close this without confirming please let me know! |
#nostalgia |
This is an issue that I'm digging into - I'm tracking my findings here so I can ask for help later if necessary.
The minimal query to trigger this is:
The text was updated successfully, but these errors were encountered: