Skip to content

perf(eventstore): optimize commands to events function - #9092

Merged
muhlemmer merged 8 commits into
mainfrom
perf-commands-query
Jan 8, 2025
Merged

muhlemmer merged 8 commits into
mainfrom
perf-commands-query

Conversation

@muhlemmer

@muhlemmer muhlemmer commented Dec 19, 2024 •

Copy link
Copy Markdown
Collaborator

Which Problems Are Solved

We were seeing high query costs in a the lateral join executed in the
commands_to_events procedural function in the database. The high cost
resulted in incremental CPU usage as a load test continued and less
req/sec handled, sarting at 836 and ending at 130 req/sec.

How the Problems Are Solved

  1. Set PARALLEL SAFE. I noticed that this option defaults to UNSAFE.
    But it's actually safe if the function doesn't INSERT
  2. Set the returned ROWS 10 parameter.
  3. Function is re-written in Pl/PgSQL so that we eliminate expensive joins.
  4. Introduced an intermediate state that does SELECT DISTINCT for the
    aggregate so that we don't have to do an expensive lateral join.

Additional Changes

Use a COALESCE to get the owner from the last event, instead of a
CASE switch.

Additional Context

@vercel

vercel Bot commented Dec 19, 2024 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2025 11:59am

@github-actions

github-actions Bot commented Dec 19, 2024 •

Copy link
Copy Markdown

Thanks for your contribution @muhlemmer! 🎉

Please make sure you tick the following checkboxes before marking this Pull Request (PR) as ready for review:

  • I am happy with the code
  • Documentations and examples are up-to-date
  • Logical behavior changes are tested automatically
  • No debug or dead code
  • My code has no repetitions
  • The PR title adheres to the conventional commit format
  • The example texts in the PR description are replaced.
  • If there are any open TODOs or follow-ups, they are described in issues and link to this PR
  • If there are deviations from a user stories acceptance criteria or design, they are agreed upon with the PO and documented.

@muhlemmer

This comment was marked as outdated.

@muhlemmer muhlemmer added performance Performance investigation or improvement area/storage Database, eventstore, projections, migrations labels Dec 19, 2024
@codecov

codecov Bot commented Dec 19, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@a54bb29). Learn more about missing BASE report.
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9092   +/-   ##
=======================================
  Coverage        ?   62.27%           
=======================================
  Files           ?     1571           
  Lines           ?   147879           
  Branches        ?        0           
=======================================
  Hits            ?    92094           
  Misses          ?    51268           
  Partials        ?     4517           
Flag Coverage Δ
core-integration-tests-postgres 35.94% <100.00%> (?)
core-unit-tests 46.55% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adlerhurst

adlerhurst commented Dec 26, 2024 •

Copy link
Copy Markdown
Member

I propose to use PLPGSQL functions for postgres, this will not work for cockroachdb free users so i would use an sql function there, we might can benefit from begin atomic there.

The PLPGSQL function i got best results so far is the following:

CREATE OR REPLACE FUNCTION eventstore.latest_aggregate_state(
    instance_id TEXT
    , aggregate_type TEXT
    , aggregate_id TEXT
    
    , sequence OUT BIGINT
    , owner OUT TEXT
)
    LANGUAGE 'plpgsql'
    STABLE PARALLEL SAFE
AS $BODY$
    BEGIN
        SELECT
            COALESCE(e.sequence, 0) AS sequence
            , e.owner
        INTO
            sequence
            , owner
        FROM
            eventstore.events2 e
        WHERE
            e.instance_id = $1
            AND e.aggregate_type = $2
            AND e.aggregate_id = $3
        ORDER BY 
            e.sequence DESC
        LIMIT 1;

        RETURN;
    END;
$BODY$;

CREATE OR REPLACE FUNCTION eventstore.commands_to_events(commands eventstore.command[])
    RETURNS SETOF eventstore.events2 
    LANGUAGE 'plpgsql'
    STABLE PARALLEL SAFE
    ROWS 10
AS $BODY$
DECLARE
    "aggregate" RECORD;
    current_sequence BIGINT;
    current_owner TEXT;
BEGIN
    FOR "aggregate" IN 
        SELECT DISTINCT
            instance_id
            , aggregate_type
            , aggregate_id
        FROM UNNEST(commands)
    LOOP
        SELECT 
            * 
        INTO
            current_sequence
            , current_owner 
        FROM eventstore.latest_aggregate_state(
            "aggregate".instance_id
            , "aggregate".aggregate_type
            , "aggregate".aggregate_id
        );

        RETURN QUERY
        SELECT
            c.instance_id
            , c.aggregate_type
            , c.aggregate_id
            , c.command_type -- AS event_type
            , COALESCE(current_sequence, 0) + ROW_NUMBER() -- AS sequence
            , c.revision
            , NOW() -- AS created_at
            , c.payload
            , c.creator
            , COALESCE(current_owner, c.owner) -- AS owner
            , EXTRACT(EPOCH FROM NOW()) -- AS position
            , c.ordinality::INT -- AS in_tx_order
        FROM
            UNNEST(commands) WITH ORDINALITY AS c
        WHERE
            c.instance_id = aggregate.instance_id
            AND c.aggregate_type = aggregate.aggregate_type
            AND c.aggregate_id = aggregate.aggregate_id;
    END LOOP;
    RETURN;
END;
$BODY$;

@muhlemmer muhlemmer assigned muhlemmer and unassigned adlerhurst Jan 3, 2025
@muhlemmer

Copy link
Copy Markdown
Collaborator Author

This PR is as done as it gets. However previously discussed with @adlerhurst:

Keep using SQL as function language for Cockroach compatibility.

We didn't think about the fact CTEs are not yet supported by Cockroach either. So Cockroach can't be modified in the same way.

As it appears we need to maintain 2 versions of the eventstore.commands_to_events function anyway, we might as well got straight for Pl/PGSQL for PostgreSQL and port the function to Cockroach as soon as they release the needed features.

What is your opinion, @adlerhurst?

@muhlemmer
muhlemmer marked this pull request as ready for review January 3, 2025 14:19
@muhlemmer
muhlemmer requested a review from adlerhurst January 3, 2025 14:19
@adlerhurst

Copy link
Copy Markdown
Member

This PR is as done as it gets. However previously discussed with @adlerhurst:

Keep using SQL as function language for Cockroach compatibility.

We didn't think about the fact CTEs are not yet supported by Cockroach either. So Cockroach can't be modified in the same way.

As it appears we need to maintain 2 versions of the eventstore.commands_to_events function anyway, we might as well got straight for Pl/PGSQL for PostgreSQL and port the function to Cockroach as soon as they release the needed features.

What is your opinion, @adlerhurst?

Sounds like a good idea. I created an issue to track if crdb is compatible: #9131

@muhlemmer

Copy link
Copy Markdown
Collaborator Author

@adlerhurst I've modified the PR to use the proposed plpgsql function. There was a syntax error which I fixed as well:

ERROR: window function row_number requires an OVER clause (SQLSTATE 42809)

adlerhurst
adlerhurst previously approved these changes Jan 7, 2025
auto-merge was automatically disabled January 8, 2025 09:37

Pull request was converted to draft

@muhlemmer

This comment was marked as outdated.

@muhlemmer
muhlemmer marked this pull request as ready for review January 8, 2025 11:06
@muhlemmer
muhlemmer merged commit df2c6f1 into main Jan 8, 2025
@muhlemmer
muhlemmer deleted the perf-commands-query branch January 8, 2025 11:59
stebenz pushed a commit that referenced this pull request Jan 13, 2025
# Which Problems Are Solved

We were seeing high query costs in a the lateral join executed in the
commands_to_events procedural function in the database. The high cost
resulted in incremental CPU usage as a load test continued and less
req/sec handled, sarting at 836 and ending at 130 req/sec.

# How the Problems Are Solved

1. Set `PARALLEL SAFE`. I noticed that this option defaults to `UNSAFE`.
But it's actually safe if the function doesn't `INSERT`
2. Set the returned `ROWS 10` parameter.
3. Function is re-written in Pl/PgSQL so that we eliminate expensive
joins.
4. Introduced an intermediate state that does `SELECT DISTINCT` for the
aggregate so that we don't have to do an expensive lateral join.

# Additional Changes

Use a `COALESCE` to get the owner from the last event, instead of a
`CASE` switch.

# Additional Context

- Function was introduced in
#8816
- Closes #8352

---------

Co-authored-by: Silvan <27845747+adlerhurst@users.noreply.github.com>
adlerhurst added a commit that referenced this pull request Jan 13, 2025
# Which Problems Are Solved

We were seeing high query costs in a the lateral join executed in the
commands_to_events procedural function in the database. The high cost
resulted in incremental CPU usage as a load test continued and less
req/sec handled, sarting at 836 and ending at 130 req/sec.

# How the Problems Are Solved

1. Set `PARALLEL SAFE`. I noticed that this option defaults to `UNSAFE`.
But it's actually safe if the function doesn't `INSERT`
2. Set the returned `ROWS 10` parameter.
3. Function is re-written in Pl/PgSQL so that we eliminate expensive
joins.
4. Introduced an intermediate state that does `SELECT DISTINCT` for the
aggregate so that we don't have to do an expensive lateral join.

# Additional Changes

Use a `COALESCE` to get the owner from the last event, instead of a
`CASE` switch.

# Additional Context

- Function was introduced in
#8816
- Closes #8352

---------

Co-authored-by: Silvan <27845747+adlerhurst@users.noreply.github.com>
adlerhurst added a commit that referenced this pull request Jan 15, 2025
# Which Problems Are Solved

The performance of the initial push function can further be increased

# How the Problems Are Solved

`eventstore.push`- and `eventstore.commands_to_events`-functions were
rewritten

# Additional Changes

none

# Additional Context

same optimizations as for postgres:
#9092
livio-a pushed a commit that referenced this pull request Jan 17, 2025
# Which Problems Are Solved

The performance of the initial push function can further be increased

# How the Problems Are Solved

`eventstore.push`- and `eventstore.commands_to_events`-functions were
rewritten

# Additional Changes

none

# Additional Context

same optimizations as for postgres:
#9092

(cherry picked from commit 690147b)
livio-a pushed a commit that referenced this pull request Feb 13, 2025
# Which Problems Are Solved

The performance of the initial push function can further be increased

# How the Problems Are Solved

`eventstore.push`- and `eventstore.commands_to_events`-functions were
rewritten

# Additional Changes

none

# Additional Context

same optimizations as for postgres:
#9092

(cherry picked from commit 690147b)
fo-ofc pushed a commit to O-F-C/zitadel that referenced this pull request Jun 10, 2026
# Which Problems Are Solved

We were seeing high query costs in a the lateral join executed in the
commands_to_events procedural function in the database. The high cost
resulted in incremental CPU usage as a load test continued and less
req/sec handled, sarting at 836 and ending at 130 req/sec.

# How the Problems Are Solved

1. Set `PARALLEL SAFE`. I noticed that this option defaults to `UNSAFE`.
But it's actually safe if the function doesn't `INSERT`
2. Set the returned `ROWS 10` parameter.
3. Function is re-written in Pl/PgSQL so that we eliminate expensive
joins.
4. Introduced an intermediate state that does `SELECT DISTINCT` for the
aggregate so that we don't have to do an expensive lateral join.

# Additional Changes

Use a `COALESCE` to get the owner from the last event, instead of a
`CASE` switch.

# Additional Context

- Function was introduced in
zitadel#8816
- Closes zitadel#8352

---------

Co-authored-by: Silvan <27845747+adlerhurst@users.noreply.github.com>
fo-ofc pushed a commit to O-F-C/zitadel that referenced this pull request Jun 10, 2026
# Which Problems Are Solved

The performance of the initial push function can further be increased

# How the Problems Are Solved

`eventstore.push`- and `eventstore.commands_to_events`-functions were
rewritten

# Additional Changes

none

# Additional Context

same optimizations as for postgres:
zitadel#9092

(cherry picked from commit f2a96e8)

This branch was successfully deployed

1 active deployment
Preview — dfc64815 Deployed Jan 8, 2025 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage Database, eventstore, projections, migrations performance Performance investigation or improvement

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Machine account authentications performance

2 participants