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

Refactor dbStructure.hs #1720

Merged
merged 7 commits into from
Jan 3, 2021

Conversation

wolfgangwalther
Copy link
Member

Related to #1719.

@wolfgangwalther wolfgangwalther force-pushed the cover-dbstructure branch 2 times, most recently from 68574b2 to 375390a Compare December 31, 2020 17:17
@wolfgangwalther
Copy link
Member Author

I moved the parseArgs part to SQL now. This does:

  • increase coverage, because it avoids some edge cases in the parsers

  • allow fool-proof parsing of argument names and types with spaces

  • have the following side-effect:
    For functions without arguments, the schema cache seemed to store a single unnamed / untyped argument before:

    - - qiName: welcome
        qiSchema: test
      - - pdArgs:
          - pgaName: ''
            pgaReq: true
            pgaType: ''
            pgaVar: false
          pdDescription: null
          pdHasVariadic: false
          pdName: welcome
          pdReturnType:
            contents:
              contents:
                qiName: text
                qiSchema: pg_catalog
              tag: Scalar
            tag: Single
          pdSchema: test
          pdVolatility: Volatile

    taken from https://github.com/PostgREST/postgrest/pull/1699/files#diff-153d54c3e6049f5c3ce9a68a92a10ccebd625e4ed7f13e2a51f908ae52aab1cbR60-R78

    Now, the schema cache has an empty pdArgs in that case. This might be related to some of the bugs we still have with overloading RPCs, although I didn't test that, yet. In any case the schema dump yaml files will become shorter ;).

@wolfgangwalther
Copy link
Member Author

memory tests have been failing in CI for this PR. I have no idea why - my local build is well short of the thresholds:

ok 1 - POST /rpc/leak?columns=blob: with a json key of 1M the memory usage(9,728,264 bytes) is less than 13M
ok 2 - POST /leak?columns=blob: with a json key of 1M the memory usage(9,798,432 bytes) is less than 13M
ok 3 - PATCH /leak?id=eq.1&columns=blob: with a json key of 1M the memory usage(9,701,448 bytes) is less than 13M
ok 4 - POST /rpc/leak?columns=blob: with a json key of 10M the memory usage(37,459,200 bytes) is less than 41M
ok 5 - POST /leak?columns=blob: with a json key of 10M the memory usage(37,537,168 bytes) is less than 41M
ok 6 - PATCH /leak?id=eq.1&columns=blob: with a json key of 10M the memory usage(37,439,024 bytes) is less than 41M
ok 7 - POST /rpc/leak?columns=blob: with a json key of 50M the memory usage(160,678,984 bytes) is less than 171M
ok 8 - POST /leak?columns=blob: with a json key of 50M the memory usage(160,754,392 bytes) is less than 171M
ok 9 - PATCH /leak?id=eq.1&columns=blob: with a json key of 50M the memory usage(160,663,608 bytes) is less than 171M
ok 10 - POST /perf_articles?columns=id,body: with a json payload of 32K that has 1000 array values the memory usage(6,828,480 bytes) is less than 11M
ok 11 - POST /perf_articles?columns=id,body: with a json payload of 329K that has 10000 array values the memory usage(7,721,816 bytes) is less than 11M
ok 12 - POST /perf_articles?columns=id,body: with a json payload of 3.4M that has 100000 array values the memory usage(17,046,840 bytes) is less than 21M

@wolfgangwalther
Copy link
Member Author

Memory seems to be fine in CI again.

A lot of the refactor is just about simplifying some pattern matches. Easier to read - less stale patterns that are not covered.

@wolfgangwalther wolfgangwalther marked this pull request as ready for review January 3, 2021 11:49
Copy link
Member

@steve-chavez steve-chavez left a comment

Choose a reason for hiding this comment

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

LGTM!

Really like the parseArgs in SQL and the new addM2MRels is short and sweet 🍬 .

@wolfgangwalther wolfgangwalther merged commit 5223082 into PostgREST:main Jan 3, 2021
@wolfgangwalther wolfgangwalther deleted the cover-dbstructure branch January 3, 2021 16:54
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.

2 participants