Skip to content
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

perf: fix space leaks by downgrading fuzzyset to v0.2.4 #3339

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

laurenceisla
Copy link
Member

Closes #3329.

@laurenceisla laurenceisla marked this pull request as ready for review March 19, 2024 00:33
@steve-chavez
Copy link
Member

@laurenceisla How about a test for this? We now have the test/io/test_big_schema.py. You should be able to reproduce the stack overflow there, I think.

@wolfgangwalther
Copy link
Member

What is a "space leak"? A "memory leak"? Not sure if that's really what happens here.

Anyway, I don't understand why this doesn't fail any builds. I remember I tried downgrading fuzzyset, when I worked on allowing GHC 9.8 - and back then it caused other dependencies to need a downgrade, too etc.

@laurenceisla
Copy link
Member Author

laurenceisla commented Mar 19, 2024

What is a "space leak"? A "memory leak"? Not sure if that's really what happens here.

Don't know the details, but it's similar in that it takes more space in memory than it's necessary, but it's freed later.

Anyway, I don't understand why this doesn't fail any builds. I remember I tried downgrading fuzzyset, when I worked on allowing GHC 9.8 - and back then it caused other dependencies to need a downgrade, too etc.

We had fuzzyset 0.2.3 before, which had text < 1.3 as a dependency. The fuzzyset 0.2.4 version has text < 2.1, so that must be why it doesn't fail.



# See: https://github.com/PostgREST/postgrest/issues/3329
def test_should_not_fail_with_stack_overflow(defaultenv):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails on the main branch with "stack overflow".

@steve-chavez
Copy link
Member

Don't know the details, but it's similar in that it takes more space in memory than it's necessary, but it's freed later.

Just to clarify. It's not a memory leak, is just that we limit the stack size during development to 1K with:

-with-rtsopts=-K1K

Which forces a stack overflow error.

More details at https://neilmitchell.blogspot.com/2015/09/detecting-space-leaks.html?m=1


Don't know fuzzyset details and why it consumes noticeable memory now. It'd be better to open an issue upstream to see if it can be fixed.

@steve-chavez
Copy link
Member

What is a "space leak"?

Actually the term was correctly used:

A space leak occurs when a computer program uses more memory than necessary. In contrast to memory leaks, where the leaked memory is never released, the memory consumed by a space leak is released, but later than expected.

References:

@wolfgangwalther
Copy link
Member

Space leak it is, then. Thanks for the explanation / links.

We had fuzzyset 0.2.3 before, which had text < 1.3 as a dependency. The fuzzyset 0.2.4 version has text < 2.1, so that must be why it doesn't fail.

Hm, maybe. I thought I had tried 0.2.4, but maybe I hadn't. There is one problem with just downgrading: This will block us eventually, because certainly the old version won't be updated anymore.

Don't know fuzzyset details and why it consumes noticeable memory now. It'd be better to open an issue upstream to see if it can be fixed.

Yes, agreed.

I'm a bit worried about how well maintained fuzzyset is, though. There seems to be some recent activity, but for example I can't even find a git tag with 0.2.4 anywhere. There are a lot more releases on hackage than tagged in the git repo. I looked for alternatives... but those that I found seemed even less mature.

@wolfgangwalther
Copy link
Member

One more problem with downgrading: This will make it harder / impossible to build postgrest directly in nixpkgs. My goal is to make pkgsStatic.postgrest work directly in nixpkgs eventually.

But maybe we can / should downgrade temporarily while still reporting the problem upstream.

@laurenceisla
Copy link
Member Author

But maybe we can / should downgrade temporarily while still reporting the problem upstream.

Agree, opened an issue there: laserpants/fuzzyset-haskell#9

@laurenceisla laurenceisla merged commit 4602595 into PostgREST:main Apr 11, 2024
27 of 28 checks passed
@laurenceisla laurenceisla deleted the fix-stackoverflow branch April 11, 2024 19:13
@taimoorzaeem
Copy link
Collaborator

@laurenceisla With this downgrade, postgrest-test-doctests fails with the following:

src/PostgREST/Error.hs:294:26: error:
    Not in scope: ‘Fuzzy.fromList’
    NB: the module ‘Data.FuzzySet’ does not export ‘fromList’.

src/PostgREST/Error.hs:295:26: error:
    Not in scope: ‘Fuzzy.fromList’
    NB: the module ‘Data.FuzzySet’ does not export ‘fromList’.

src/PostgREST/Error.hs:296:21: error:
    Not in scope: ‘Fuzzy.getOne’
    NB: the module ‘Data.FuzzySet’ does not export ‘getOne’.

src/PostgREST/Error.hs:298:43: error:
    Not in scope: ‘Fuzzy.get’
    NB: the module ‘Data.FuzzySet’ does not export ‘get’.
.
.
.

@wolfgangwalther
Copy link
Member

Strange. Why did this not fail in CI. I think this was the latest run here: https://github.com/PostgREST/postgrest/actions/runs/8349585526/job/22854079725. Doctests seem fine here.

@taimoorzaeem
Copy link
Collaborator

Strange. Why did this not fail in CI. I think this was the latest run here: https://github.com/PostgREST/postgrest/actions/runs/8349585526/job/22854079725. Doctests seem fine here.

On more thought, I think it is some local issue on my side. My bad.

@steve-chavez
Copy link
Member

I'm getting the same errors. @laurenceisla Can you reproduce?

$ postgrest-test-doctests 

Warning: Both /home/stevechavez/.cabal and
/home/stevechavez/.config/cabal/config exist - ignoring the former.
It is advisable to remove one of them. In that case, we will use the remaining
one by default (unless '$CABAL_DIR' is explicitly set).

src/PostgREST/Error.hs:294:26: error:
    Not in scope: ‘Fuzzy.fromList’
    NB: the module ‘Data.FuzzySet’ does not export ‘fromList’.

src/PostgREST/Error.hs:295:26: error:
    Not in scope: ‘Fuzzy.fromList’
    NB: the module ‘Data.FuzzySet’ does not export ‘fromList’.

src/PostgREST/Error.hs:296:21: error:
    Not in scope: ‘Fuzzy.getOne’
    NB: the module ‘Data.FuzzySet’ does not export ‘getOne’.

src/PostgREST/Error.hs:298:43: error:
    Not in scope: ‘Fuzzy.get’
    NB: the module ‘Data.FuzzySet’ does not export ‘get’.

src/PostgREST/Error.hs:341:24: error:
    Not in scope: ‘Fuzzy.fromList’
    NB: the module ‘Data.FuzzySet’ does not export ‘fromList’.

src/PostgREST/Error.hs:342:24: error:
    Not in scope: ‘Fuzzy.fromList’
    NB: the module ‘Data.FuzzySet’ does not export ‘fromList’.

src/PostgREST/Error.hs:348:32: error:
    Not in scope: ‘Fuzzy.getOne’
    NB: the module ‘Data.FuzzySet’ does not export ‘getOne’.

src/PostgREST/Error.hs:349:50: error:
    Not in scope: ‘Fuzzy.getOne’
    NB: the module ‘Data.FuzzySet’ does not export ‘getOne’.

src/PostgREST/Version.hs:12:1: error:
    Could not find module ‘Paths_postgrest’
    Perhaps you meant Paths_doctest
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
src/PostgREST/Plan.hs:785: failure in expression `:{
 -- this represents the `projects(*)` part on `/clients?select=*,projects(*)`
 let
 subForestPlan =
   [
     Node {
       rootLabel = ReadPlan {
         select = [], -- there will be fields at this stage but we just omit them for brevity
         from = QualifiedIdentifier {qiSchema = "test", qiName = "projects"},
         fromAlias = Just "projects_1", where_ = [], order = [], range_ = fullRange,
         relName = "projects",
         relToParent = Nothing,
         relJoinConds = [],
         relAlias = Nothing, relAggAlias = "clients_projects_1", relHint = Nothing, relJoinType = Nothing, relIsSpread = False, depth = 1,
         relSelect = []
       },
       subForest = []
     }
   ]
:}'
expected: 
 but got: 
          ^
          <interactive>:46:6: error: Not in scope: data constructor ‘Node’
          
          <interactive>:47:20: error:
              Not in scope: data constructor ‘ReadPlan’

src/PostgREST/Config.hs:491: failure in expression `addFallbackAppName ver "postgres://user:pass@host:5432/postgres"'
expected: "postgres://user:pass@host:5432/postgres?fallback_application_name=PostgREST%2011.1.0%20%285a04ec7%29"
 but got: 
          ^
          <interactive>:284:1: error:
              Variable not in scope: addFallbackAppName :: ByteString -> t0 -> t

src/PostgREST/Error.hs:263: failure in expression `let rel ft = Relationship{relForeignTable = qi ft}'
expected: 
 but got: 
          ^
          <interactive>:294:14: error:
              Not in scope: data constructor ‘Relationship’

src/PostgREST/Error.hs:307: failure in expression `noRpcHint "api" "testt" ["val", "param", "name"] procs []'
expected: Just "Perhaps you meant to call the function api.test"
 but got: 
          ^
          <interactive>:304:1: error:
              Variable not in scope:
                noRpcHint :: t0 -> t1 -> [a0] -> [QualifiedIdentifier] -> [a1] -> t

Examples: 153  Tried: 127  Errors: 0  Failures: 4

@laurenceisla
Copy link
Member Author

@steve-chavez Strange, I'm not getting those errors:

[nix-shell:~/Projects/postgrest]$ postgrest-test-doctests
Warning: Both /home/laurence/.cabal and /home/laurence/.config/cabal/config
exist - ignoring the former.
It is advisable to remove one of them. In that case, we will use the remaining
one by default (unless '$CABAL_DIR' is explicitly set).

src/PostgREST/Version.hs:12:1: error:
    Could not find module ‘Paths_postgrest’
    Perhaps you meant Paths_doctest
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
Examples: 153  Tried: 153  Errors: 0  Failures: 0

Maybe your nix-shell is caching something? postgrest-clean could help.

@steve-chavez
Copy link
Member

Ah, right. Just reentering nix-shell made those errors go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Stack Overflow error in dev enviroment when table and embed resource do not exist
4 participants