perf: shortcut for proc with no variadic arg #1636

steve-chavez · 2020-10-28T01:13:30Z

On #1603, logic for using VARIADIC functions was added. However that logic is also executed for every non-variadic function.

This PR changes that. By adding a cached pdHasVariadic to ProcDescription we avoid extra steps for non-variadic functions.

Besides that:

Isolates the logic for changing the RPC params /rpc/func?arg1=val1&arg2=val2 to json in a single function. This makes that part of the code more understandable IMO.
Includes some refactors done on Add pgrst.accept setting to RPC #1582.
Fixes appending an erroneous charset=uft-8 on binary output. Reported on gitter chat.

* move json ops tests to JsonOperatorSpec * move phfts operator tests to QuerySpec and RpcSpec * add custom spec for pre-request header guc tests

steve-chavez · 2020-10-28T01:16:38Z

test/Feature/RpcSpec.hs

+    it "returns last value for repeated params without VARIADIC" $
+      get "/rpc/sayhello?name=ignored&name=world"


@wolfgangwalther I've changed this behavior. It would be a breaking change otherwise.

Oh yes. It was my intention to not introduce a breaking change here - that's why I wrote the test. 👍

Also refactor how rpc params are converted to json

wolfgangwalther · 2020-10-28T09:18:29Z

src/PostgREST/ApiRequest.hs

+  where
+    mergeParams :: RpcParamValue -> RpcParamValue -> RpcParamValue
+    mergeParams (Variadic a) (Variadic b) = Variadic $ b ++ a
+    mergeParams _ v                       = v -- repeated params for non-variadic arguments are not merged


I think the order of this needs to be changed to v _. This is because you changed the test-case from first to last repetition for non-repeated args.

However, since you're change with hasVariadic this code path is not taken anymore. So we actually need another test case that has a function with regular argument + variadic argument, where the non-variadic one is repeated as well. And then this part kicks in and should show the same behaviour as the regular case.

@wolfgangwalther Hmm.. could you help me with that on another PR?

I mostly wanted to isolate the bits for non-variadic functions here. If the variadic part behaves differently it wouldn't be a breaking change for now since we haven't been supporting variadic.

Sure, see #1639.

Thanks a lot Wolfgang :)

wolfgangwalther · 2020-10-28T09:20:38Z

src/PostgREST/ApiRequest.hs

+-- | Convert rpc params `/rpc/func?a=val1&b=val2` to json `{"a": "val1", "b": "val2"}
+jsonRpcParams :: ProcDescription -> [(Text, Text)] -> PayloadJSON
+jsonRpcParams proc prms =
+  if not $ pdHasVariadic proc then -- if proc has no variadic arg, save steps and directly convert to json


I'm curious - did you test the performance difference of this? How many req/s does the variadic parsing take away?

Edit: There is a drop in performance. See below #1636 (comment)

You know what, the perf difference is actually negligible.

I did a load test before and I remember getting around 100 req/s less but I must have made a mistake. I was calling an add_them func with 10 args.

It's still a nice refactor I guess 😄

Btw, I get ~1990 req/s for an /rpc/add_them?a=1&b=2&c=3&d=4&e=5.

With the code you have in this PR, you could compare the following 3 functions/requests:

-- call with: GET /rpc/add_them1?a=1&b=2&c=3&d=4&e=5 -- will take the "if not pdHasVariadic" path CREATE FUNCTION add_them1(a int, b int, c int, d int, e int) RETURNS int LANGUAGE SQL AS $$ SELECT a+b+c+d+e $$; -- call with: GET /rpc/add_them2?a=1&b=2&c=3&d=4&e=5 -- will take the "else" path - but is actually not a variadic function (the variadic arg will be ignored, it has a default value) CREATE FUNCTION add_them2(a int, b int, c int, d int, e int, VARIADIC v int[] DEFAULT '{}') RETURNS int LANGUAGE SQL AS $$ SELECT a+b+c+d+e $$; -- call with: GET /rpc/add_them3?v=1&v=2&v=3&v=4&v=5 -- will take the "else" path - and is really variadic CREATE FUNCTION add_them3(VARIADIC v int[]) RETURNS int LANGUAGE SQL AS $$ -- no SUM or anything on purpose to be as much comparable to the other funcs as possible SELECT v[1]+v[2]+v[3]+v[4]+v[5] $$;

This should give us a really good idea whether the variadic branch is in fact slower or not.

It's still a nice refactor I guess smile

With most of the refactor I absolutely agree, yes! But if the variadic branch was not really slower, then this branching (and adding the hasVariadic stuff) would just increase the complexity of the code (one indication of that would be the need for #1639 to make things consistent again).

addthem2 is not working (need to add v=5). Should it work with #1639?

I tested this and I'm pretty sure we hit a postgres bug here.

Filed a bug report here: https://www.postgresql.org/message-id/[email protected]

Made new 7 load tests with the corrected addthem2:

addthem2

(1923 + 1908 + 1897 + 1892 + 1911 + 1915 + 1904)/7 = 1907

Seems to be the slowest code path(not sure why).

So the non-variadic path remains to be the fastest.

Filed a bug report here: https://www.postgresql.org/message-id/[email protected]

@wolfgangwalther Nice! I see you already got some replies.

So if it's a pg bug, would there be anything to add on this PR?

Seems to bit the slowest code path(not sure why).

This might be because it's now 6 params instead of 5 that have to be parsed. So you can't really compare it to the first addthem1 you ran, but only to the 2nd modified example I gave, that has 6 params as well.

Anyway, I think it's fine to keep the non-variadic path.

So if it's a pg bug, would there be anything to add on this PR?

Not sure whether they will agree to call it a bug or rather a "limitation". It's currently not possible to call the function in PG that way, so I think we are fine with where we are. If you want to use such a function you can always add an overload that does not have the variadic part - and then everything will work.

Nothing to do here for us right now.

Got it. Then I'll merge!

wolfgangwalther · 2020-10-29T06:52:40Z

src/PostgREST/Types.hs

@@ -45,6 +49,7 @@ toMime CTTextCSV         = "text/csv"
 toMime CTTextPlain       = "text/plain"
 toMime CTOpenAPI         = "application/openapi+json"
 toMime CTSingularJSON    = "application/vnd.pgrst.object+json"
+toMime CTUrlEncoded      = "application/x-www-form-urlencoded"


Code coverage drops because of this line. Is this ever going to be used? This would only be needed to return this mimetype, right? I doubt that will ever be the case.

BTW, that's the same case for the CTAny line below.

If our goal was to increase code coverage (not only technically but also for a nice number :D), this would be some simple things to do.

Hmm.. yeah, it's unused.

How would I go about that? Just delete the lines? Not sure if the compiler would like that..

Yeah, you're right. I didn't think that to the end. But for the code coverage stuff to be useful, we'll have to find a way to "remove" those false alarms, otherwise it's a pain to check for the important stuff.

It looks like the tool for the job are hpc "overlays". I will dig into this further once I find a bit of time and see what we can do.

steve-chavez added 2 commits October 26, 2020 18:40

refactor: reorder PgVersion(95/96)Spec tests

b080479

* move json ops tests to JsonOperatorSpec * move phfts operator tests to QuerySpec and RpcSpec * add custom spec for pre-request header guc tests

refactor: add Hasql arrayColumn decoder

039cdbe

steve-chavez commented Oct 28, 2020

View reviewed changes

steve-chavez added 2 commits October 27, 2020 20:21

Fix charset=utf-8 appending to binary output

4a7c36c

perf: shortcut for proc with no variadic arg

79207e2

Also refactor how rpc params are converted to json

steve-chavez force-pushed the refactor-fix-charset-utf8 branch from a9c0c84 to 79207e2 Compare October 28, 2020 01:22

steve-chavez mentioned this pull request Oct 28, 2020

Calling variadic functions PostgREST/postgrest-docs#357

Merged

wolfgangwalther reviewed Oct 28, 2020

View reviewed changes

wolfgangwalther mentioned this pull request Oct 28, 2020

Fix variadic mixed repeated args #1639

Merged

wolfgangwalther reviewed Oct 29, 2020

View reviewed changes

steve-chavez merged commit 8618ffa into PostgREST:master Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: shortcut for proc with no variadic arg #1636

perf: shortcut for proc with no variadic arg #1636

steve-chavez commented Oct 28, 2020

steve-chavez Oct 28, 2020

wolfgangwalther Oct 28, 2020

wolfgangwalther Oct 28, 2020

steve-chavez Oct 28, 2020

wolfgangwalther Oct 28, 2020

steve-chavez Oct 30, 2020

wolfgangwalther Oct 28, 2020

steve-chavez Oct 28, 2020 •

edited

Loading

steve-chavez Oct 28, 2020

wolfgangwalther Oct 28, 2020

wolfgangwalther Oct 28, 2020 •

edited

Loading

wolfgangwalther Oct 28, 2020 •

edited

Loading

steve-chavez Oct 29, 2020 •

edited

Loading

steve-chavez Oct 29, 2020 •

edited

Loading

wolfgangwalther Oct 29, 2020

steve-chavez Oct 29, 2020

wolfgangwalther Oct 29, 2020

steve-chavez Oct 29, 2020

wolfgangwalther Oct 29, 2020

		it "returns last value for repeated params without VARIADIC" $
		get "/rpc/sayhello?name=ignored&name=world"

perf: shortcut for proc with no variadic arg #1636

perf: shortcut for proc with no variadic arg #1636

Conversation

steve-chavez commented Oct 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-chavez Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfgangwalther Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

wolfgangwalther Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

steve-chavez Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

addthem2

steve-chavez Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-chavez Oct 28, 2020 •

edited

Loading

wolfgangwalther Oct 28, 2020 •

edited

Loading

wolfgangwalther Oct 28, 2020 •

edited

Loading

steve-chavez Oct 29, 2020 •

edited

Loading

steve-chavez Oct 29, 2020 •

edited

Loading