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: use LATERAL instead of CTE for json body #2677

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

steve-chavez
Copy link
Member

@steve-chavez steve-chavez commented Feb 22, 2023

Increases transactions per second for RPC on 16% and for INSERTs on 6%.

It uses the fact that inlining can be done on function calls when:

none of the actual arguments contain volatile expressions or subselects

So basically it goes from:

WITH
pgrst_payload AS (SELECT '[{"id": 4, "name": "Vier"}, {"id": 5, "name": "Funf", "arr_data": null}, {"id": 6, "name": "Sechs", "arr_data": [1, 2, 3], "field-with_sep": 6}]'::json AS json_data),
pgrst_body AS ( SELECT CASE WHEN json_typeof(json_data) = 'array' THEN json_data ELSE json_build_array(json_data) END AS val FROM pgrst_payload)
INSERT INTO "test"."complex_items"("arr_data", "field-with_sep", "id", "name")
SELECT "arr_data", "field-with_sep", "id", "name"
FROM json_to_recordset ((SELECT val FROM pgrst_body)) AS _ ("arr_data" integer[], "field-with_sep" integer, "id" bigint, "name" text)
RETURNING "test"."complex_items".*

To:

INSERT INTO "test"."complex_items"("arr_data", "field-with_sep", "id", "name")
SELECT pgrst_body."arr_data", pgrst_body."field-with_sep", pgrst_body."id", pgrst_body."name"
FROM (
  SELECT '[{"id": 4, "name": "Vier"}, {"id": 5, "name": "Funf", "arr_data": null}, {"id": 6, "name": "Sechs", "arr_data": [1, 2, 3], "field-with_sep": 6}]'::json as json_data
) pgrst_payload,
LATERAL (
  SELECT CASE WHEN json_typeof(pgrst_payload.json_data) = 'array' THEN pgrst_payload.json_data ELSE json_build_array(pgrst_payload.json_data) END AS val
) pgrst_uniform_json,
LATERAL (
  SELECT * FROM json_to_recordset (pgrst_uniform_json.val) AS _ ("arr_data" integer[], "field-with_sep" integer, "id" bigint, "name" text)
) pgrst_body
RETURNING "test"."complex_items".*

Fixes #1652 and does optimization 2 mentioned on #2676.

@steve-chavez steve-chavez mentioned this pull request Feb 22, 2023
3 tasks
@steve-chavez steve-chavez force-pushed the insert-perf-1 branch 2 times, most recently from aac49cf to 5fb8ef0 Compare February 22, 2023 16:51
@steve-chavez
Copy link
Member Author

One thing to note is that the loadtest is not capturing this kind of slight improvements.

A first run shows an improvement in throughput: https://github.com/PostgREST/postgrest/pull/2677/checks?check_run_id=11510644266

While the second shows a decrease: https://github.com/PostgREST/postgrest/pull/2677/checks?check_run_id=11526098776

@steve-chavez
Copy link
Member Author

Will leave the pgbench test on the codebase. It doesn't run on CI but I think it's useful to have it handy(for copy/pasting for other benchs too).

@steve-chavez
Copy link
Member Author

Took at stab at #1652. Using the queries on cc9cfe2.

$ postgrest-with-postgresql-15 -f test/pgbench/fixtures.sql pgbench -n -T 10 -f test/pgbench/1652/old.sql 

pgbench (15.1)
transaction type: test/pgbench/1652/old.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 45046
number of failed transactions: 0 (0.000%)
latency average = 0.222 ms
initial connection time = 1.254 ms
tps = 4505.073033 (without initial connection time)

$ postgrest-with-postgresql-15 -f test/pgbench/fixtures.sql pgbench -n -T 10 -f test/pgbench/1652/new.sql
pgbench (15.1)
transaction type: test/pgbench/1652/new.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 52286
number of failed transactions: 0 (0.000%)
latency average = 0.191 ms
initial connection time = 1.274 ms
tps = 5229.205542 (without initial connection time)

So a 16% TPS increase. This is a case where the cost plan is misleading because the old query has a lower cost than the new one:

explain WITH pgrst_source AS (
  WITH
  pgrst_payload AS (SELECT '{"id": 4}'::json AS json_data),
  pgrst_body AS ( SELECT CASE WHEN json_typeof(json_data) = 'array' THEN json_data ELSE json_build_array(json_data) END AS val FROM pgrst_payload),
  pgrst_args AS ( SELECT * FROM json_to_recordset((SELECT val FROM pgrst_body)) AS _("id" integer) )
  SELECT "get_projects_below".*
  FROM "test"."get_projects_below"("id" := (SELECT "id" FROM pgrst_args LIMIT 1))
)
SELECT
  null::bigint AS total_result_set,
  pg_catalog.count(_postgrest_t) AS page_total,
  coalesce(json_agg(_postgrest_t), '[]')::character varying AS body,
  nullif(current_setting('response.headers', true), '') AS response_headers,
  nullif(current_setting('response.status', true), '') AS response_status
FROM (SELECT "projects".* FROM "pgrst_source" AS "projects") _postgrest_t;

                                          QUERY PLAN
----------------------------------------------------------------------------------------------
 Aggregate  (cost=25.28..25.30 rows=1 width=112)
   ->  Function Scan on get_projects_below  (cost=0.28..10.28 rows=1000 width=40)
         InitPlan 2 (returns $1)
           ->  Limit  (cost=0.02..0.03 rows=1 width=4)
                 InitPlan 1 (returns $0)
                   ->  Result  (cost=0.00..0.01 rows=1 width=32)
                 ->  Function Scan on json_to_recordset _  (cost=0.00..1.00 rows=100 width=4)
explain WITH pgrst_source AS (
  SELECT "get_projects_below".*
  FROM (
    SELECT '{"id": 4}'::json as json_data
  ) pgrst_payload,
  LATERAL (
    SELECT CASE WHEN json_typeof(pgrst_payload.json_data) = 'array' THEN pgrst_payload.json_data ELSE json_build_array(pgrst_payload.json_data) END AS val
  ) pgrst_uniform_json,
  LATERAL (
    SELECT * FROM json_to_recordset(pgrst_uniform_json.val) AS _("id" integer) LIMIT 1
  ) pgrst_args,
  LATERAL "test"."get_projects_below"("id" := pgrst_args.id)
)
SELECT
  null::bigint AS total_result_set,
  pg_catalog.count(_postgrest_t) AS page_total,
  coalesce(json_agg(_postgrest_t), '[]')::character varying AS body,
  nullif(current_setting('response.headers', true), '') AS response_headers,
  nullif(current_setting('response.status', true), '') AS response_status
FROM (SELECT "projects".* FROM "pgrst_source" AS "projects") _postgrest_t;

                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 Aggregate  (cost=28.28..28.30 rows=1 width=112)
   ->  Nested Loop  (cost=7.26..26.28 rows=400 width=40)
         ->  Limit  (cost=0.01..0.01 rows=1 width=4)
               ->  Function Scan on json_to_recordset _  (cost=0.01..1.00 rows=100 width=4)
         ->  Bitmap Heap Scan on projects  (cost=7.25..22.25 rows=400 width=40)
               Recheck Cond: (id < _.id)
               ->  Bitmap Index Scan on projects_pkey  (cost=0.00..7.15 rows=400 width=0)
                     Index Cond: (id < _.id)

I made the test.get_projects_below STABLE for the EXPLAINs above. As it can be seen the new query has inlining enabled since the Index Cond applies while the old query does a function scan. So I believe this fixes #1652.

@steve-chavez
Copy link
Member Author

This is failing for pg 11 and 12:

      when (actualPgVersion >= pgVersion110) $
        it "can embed if rpc returns domain of table type" $ do
          post "/rpc/getproject_domain?select=id,name,client:clients(id),tasks(id)"
              [json| { "id": 1} |]
            `shouldRespondWith`
              [json|[{"id":1,"name":"Windows 7","client":{"id":1},"tasks":[{"id":1},{"id":2}]}]|]
          get "/rpc/getproject_domain?id=1&select=id,name,client:clients(id),tasks(id)"
            `shouldRespondWith`
              [json|[{"id":1,"name":"Windows 7","client":{"id":1},"tasks":[{"id":1},{"id":2}]}]|]
  test/spec/Feature/Query/RpcSpec.hs:316:13: 
  1) Feature.Query.RpcSpec, remote procedure call, foreign entities embedding, can embed if rpc returns domain of table type
       status mismatch:
         expected: 200
         but got:  500
       body mismatch:
         expected: [{"client":{"id":1},"id":1,"name":"Windows 7","tasks":[{"id":1},{"id":2}]}]
         but got:  {"code":"XX000","details":null,"hint":null,"message":"could not find attribute 3 in subquery targetlist"}

It seems to be a bug in PostgreSQL itself, because of inlining:

WITH pgrst_source AS (
  SELECT "pgrst_call"."client_id", "pgrst_call"."id", "pgrst_call"."name"
  FROM (SELECT '{ "id": 1}'::json AS json_data) pgrst_payload,
  LATERAL (SELECT CASE WHEN json_typeof(pgrst_payload.json_data) = 'array' THEN pgrst_payload.json_data ELSE json_build_array(pgrst_payload.json_data) END AS val) pgrst_uniform_json,
  LATERAL (SELECT * FROM json_to_recordset(pgrst_uniform_json.val) AS _("id" integer) LIMIT 1) pgrst_body ,
  LATERAL "test"."getproject_domain"("id" := pgrst_body."id") pgrst_call
)
SELECT
  null::bigint AS total_result_set,
  pg_catalog.count(_postgrest_t) AS page_total,
  coalesce(json_agg(_postgrest_t), '[]')::character varying AS body,
  nullif(current_setting('response.headers', true), '') AS response_headers,
  nullif(current_setting('response.status', true), '') AS response_status
FROM (SELECT "projects"."id", "projects"."name", row_to_json("projects_client_1".*) AS "client", COALESCE( "projects_tasks_1"."projects_tasks_1", '[]') AS "tasks" FROM "pgrst_source" AS "proje
cts" LEFT JOIN LATERAL ( SELECT "clients_1"."id" FROM "test"."clients" AS "clients_1"  WHERE "clients_1"."id" = "projects"."client_id"   ) AS "projects_client_1" ON TRUE LEFT JOIN LATERAL ( SE
LECT json_agg("projects_tasks_1") AS "projects_tasks_1"FROM (SELECT "tasks_1"."id" FROM "test"."tasks" AS "tasks_1"  WHERE "tasks_1"."project_id" = "projects"."id"   ) AS "projects_tasks_1" )
AS "projects_tasks_1" ON TRUE   ) _postgrest_t;
ERROR:  type "projects_domain" does not exist
LINE 2:           SELECT projects::projects_domain FROM test.project...
                                   ^
QUERY:
          SELECT projects::projects_domain FROM test.projects WHERE id = $1;

CONTEXT:  SQL function "getproject_domain" during inlining

I'll just enable the test to run on pg >= 13.

@steve-chavez steve-chavez force-pushed the insert-perf-1 branch 2 times, most recently from 3e86be1 to 3285268 Compare February 25, 2023 07:04
@steve-chavez
Copy link
Member Author

To prevent regressions, I added tests to ensure inlining happens.

* add pgbench test for INSERT and RPC
* use LATERAL for RPC and INSERT
* add tests for inlining
@steve-chavez steve-chavez merged commit 0d5d209 into PostgREST:main Feb 25, 2023
@wolfgangwalther
Copy link
Member

This is failing for pg 11 and 12:
[...]
It seems to be a bug in PostgreSQL itself, because of inlining:

I don't think that's related to inlining, but rather to composite domains and how we call the function:

postgres=# SELECT * FROM test.getproject_domain(id := 1);
ERROR:  could not find attribute 2 in subquery targetlist

postgres=# SELECT test.getproject_domain(id := 1);
 getproject_domain
-------------------
 (1,"Windows 7",1)
(1 row)

postgres=# SELECT (test.getproject_domain(id := 1)).*;
 id |   name    | client_id
----+-----------+-----------
  1 | Windows 7 |         1
(1 row)

We could work around that by constructing the query as:

postgres=#   SELECT "pgrst_call"."client_id", "pgrst_call"."id", "pgrst_call"."name"
  FROM (SELECT '{ "id": 1}'::json AS json_data) pgrst_payload,
  LATERAL (SELECT CASE WHEN json_typeof(pgrst_payload.json_data) = 'array' THEN pgrst_payload.json_data ELSE json_build_array(pgrst_payload.json_data) END AS val) pgrst_uniform_json,
  LATERAL (SELECT * FROM json_to_recordset(pgrst_uniform_json.val) AS _("id" integer) LIMIT 1) pgrst_body ,
  LATERAL (SELECT ("test"."getproject_domain"("id" := pgrst_body."id")).*) pgrst_call;
 client_id | id |   name
-----------+----+-----------
         1 |  1 | Windows 7
(1 row)

The change is just in the last line:

-   LATERAL "test"."getproject_domain"("id" := pgrst_body."id") pgrst_call
+   LATERAL (SELECT ("test"."getproject_domain"("id" := pgrst_body."id")).*) pgrst_call;

@steve-chavez
Copy link
Member Author

I think those tests could be a bit less error-prone and easier to understand, though, if we'd just check that the plan does not have the function name in it. If the RPC function name is in there - it's not inlined.

Cool. I'll make the change.


The change is just in the last line:
LATERAL "test"."getproject_domain"("id" := pgrst_body."id") pgrst_call
LATERAL (SELECT ("test"."getproject_domain"("id" := pgrst_body."id")).*) pgrst_call;

I get 82 test failures when doing that, the errors are like:

{"code":"42809","details":null,"hint":null,"message":"type text is not composite"}
{"code":"42809","details":null,"hint":null,"message":"type integer is not composite"}

When changing the logic to:

    callIt :: SQL.Snippet -> SQL.Snippet
    callIt argument | returnsScalar = SQL.sql (fromQi qi) <> "(" <> argument <> ") pgrst_call"
                    | otherwise     = "(SELECT (" <> SQL.sql (fromQi qi) <> "(" <> argument <> ")).*) pgrst_call"

Now I get 10 failures of the type:

{"code":"42809","details":null,"hint":null,"message":"type void is not composite"}

I guess we'd have to consider void as a scalar.. would anything break because of that?

Not sure if worth it..

@steve-chavez
Copy link
Member Author

Right now we're considering void as `Nothing:

parseRetType :: Text -> Text -> Bool -> Bool -> Bool -> Maybe RetType
parseRetType schema name isSetOf isComposite isVoid
| isVoid = Nothing
| isSetOf = Just (SetOf pgType)
| otherwise = Just (Single pgType)
where
qi = QualifiedIdentifier schema name
pgType
| isComposite = Composite qi
| otherwise = Scalar

@wolfgangwalther
Copy link
Member

Not sure if worth it..

If I understood correctly, then RPCs returning a composite value are entirely broken on PG11 and PG12, right?

I don't think we can claim to support PG11 and PG12 and not fix it.

@wolfgangwalther
Copy link
Member

I guess we'd have to consider void as a scalar.. would anything break because of that?

I think this would actually be technically correct, because void behaves exactly like a scalar always returning NULL.

@steve-chavez
Copy link
Member Author

I don't think we can claim to support PG11 and PG12 and not fix it.

Yeah, you're right.

I think this would actually be technically correct, because void behaves exactly like a scalar always returning NULL.

Let me try and make this fix on #2723

@steve-chavez
Copy link
Member Author

I think this would actually be technically correct, because void behaves exactly like a scalar always returning NULL.

That did work and actually ProcDescription looks better by having a RetType instead of a Maybe RetType.

3 test failures now:

  test/spec/Feature/Query/RpcSpec.hs:627:11:
  1) Feature.Query.RpcSpec, remote procedure call, procs with OUT/INOUT params, returns an object result when there is a single OUT param
       status mismatch:
         expected: 200
         but got:  400
       body mismatch:
         expected: {"num_plus_one":6}
         but got:  {"code":"42809","details":null,"hint":null,"message":"type integer is not composite"}

  To rerun use: --match "/Feature.Query.RpcSpec/remote procedure call/procs with OUT/INOUT params/returns an object result when there is a single OUT param/"

  test/spec/Feature/Query/RpcSpec.hs:636:11:
  2) Feature.Query.RpcSpec, remote procedure call, procs with OUT/INOUT params, returns an object result when there is a single INOUT param
       status mismatch:
         expected: 200
         but got:  400
       body mismatch:
         expected: {"num":3}
         but got:  {"code":"42809","details":null,"hint":null,"message":"type integer is not composite"}

  To rerun use: --match "/Feature.Query.RpcSpec/remote procedure call/procs with OUT/INOUT params/returns an object result when there is a single INOUT param/"

  test/spec/Feature/Query/RpcSpec.hs:652:11:
  3) Feature.Query.RpcSpec, remote procedure call, procs with TABLE return, returns an object result when there is a single-column TABLE return type
       status mismatch:
         expected: 200
         but got:  400
       body mismatch:
         expected: [{"a":"A"}]
         but got:  {"code":"42809","details":null,"hint":null,"message":"type text is not composite"}

Somehow these are being taken as scalars:

create function test.single_out_param(num int, OUT num_plus_one int) AS $$
  select num + 1;
$$ language sql;

create function test.single_inout_param(INOUT num int) AS $$
  select num + 1;
$$ language sql;

create function test.single_column_table_return () returns table (a text) AS $$
  select 'A'::text;
$$ language sql;

There's a blocker in fixing the returns table (a text) one.

procReturnsScalar :: ProcDescription -> Bool
procReturnsScalar proc = case proc of
  ProcDescription{pdReturnType = Single (Scalar _)} -> True
  ProcDescription{pdReturnType = SetOf (Scalar _)}  -> True
  _                                                    -> False

The binary output on a single column logic is relying on the SetOf (Scalar _) -> True 😕

@steve-chavez
Copy link
Member Author

Pretty sure there is an assumption somewhere for why the first 2 functions are considered scalar too.

@steve-chavez
Copy link
Member Author

The binary output on a single column logic is relying on the SetOf (Scalar _) -> True confused

This would be easy to clear if I can break the current binary output feature(already did that on #2701) instead of relying on workarounds which are hard to understand in the codebase.

I think I'll continue with #1582 so I can break binary output and then fix this one.

@steve-chavez
Copy link
Member Author

If I understood correctly, then RPCs returning a composite value are entirely broken on PG11 and PG12, right?

@wolfgangwalther Not really, the following test is passing for example:

request methodHead "/rpc/getallprojects?select=*,clients!inner(*)&clients.id=eq.1"
[("Prefer", "count=exact")] ""
`shouldRespondWith` ""
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "0-1/2"]
}

Returning a domain on RPC is also passing:

when (actualPgVersion >= pgVersion110) $
it "returns domain of composite type" $
post "/rpc/ret_composite_domain"
[json|{}|]
`shouldRespondWith`
[json|{"x": 10, "y": 5}|]

The only thing that's broken is returning a domain alias for a table type:

-- domains on tables are only supported from pg 11 on
DO $do$BEGIN
IF (SELECT current_setting('server_version_num')::INT >= 110000) THEN
CREATE DOMAIN projects_domain AS projects;
CREATE FUNCTION getproject_domain(id int) RETURNS SETOF projects_domain
LANGUAGE sql
STABLE
AS $_$
SELECT projects::projects_domain FROM test.projects WHERE id = $1;
$_$;
END IF;
END$do$;

This seems to be fixed on pg here https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=a9ae99d0190960ce2d3dd3e5f10e7f4adc3cf203 but I don't think it has been backpatched(just tried 11.19, which should also come on #2718, it doesn't work)

Other references:

I don't think we can claim to support PG11 and PG12 and not fix it.

So that looks a bit extreme. At worst, I think the limitation on returning DOMAIN alias for table on RPC could be documented. And for those cases we can suggest to upgrade pg version.

  • LATERAL (SELECT ("test"."getproject_domain"("id" := pgrst_body."id")).*) pgrst_call;

Doing the above also inflates the plan cost tremendously.

@wolfgangwalther
Copy link
Member

I don't think we can claim to support PG11 and PG12 and not fix it.

So that looks a bit extreme. At worst, I think the limitation on returning DOMAIN alias for table on RPC could be documented. And for those cases we can suggest to upgrade pg version.

Ok.

steve-chavez added a commit to steve-chavez/postgrest that referenced this pull request Apr 5, 2023
The new LATERAL query used for calling the function, introduced on
PostgREST#2677, failed on functions
that returned a domain like `CREATE DOMAIN projects_domain AS projects`.

Work around that by changing the query conditionally, by
obtaining a bool that represents the composite alias on the SchemaCache
and only do this on pg 11 and 12.
@steve-chavez
Copy link
Member Author

The only thing that's broken is returning a domain alias for a table type

Actually that gave me a clean idea on how to solve this, followed up on #2737

steve-chavez added a commit that referenced this pull request Apr 5, 2023
The new LATERAL query used for calling the function, introduced on
#2677, failed on functions
that returned a domain like `CREATE DOMAIN projects_domain AS projects`.

Work around that by changing the query conditionally, by
obtaining a bool that represents the composite alias on the SchemaCache
and only do this on pg 11 and 12.
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.

limits don't seem to apply directly to the underlying query, causing poor performance
2 participants