fix: remove JSON cast when querying archived workflows #13777
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With PostgreSQL, the `argo_archived_workflows.workflow` column has been of type `json` ever since 8a1e611, which was released as v2.5.0. Therefore, the `::json` casts do nothing, and prevent users from improving performance by migrating to JSONB using the following query: ```sql alter table argo_archived_workflows alter column workflow set data type jsonb using workflow::jsonb ``` Without the changes in this PR, running the above will massively slow down the queries, because casting JSONB to JSON is expensive. With the changes in this PR, it improves performance by ~80%, which I determined by running the benchmarks added in argoproj#13767 against a DB with 100,000 randomly generated workflows generated by argoproj#13715: ``` $ benchstat postgres_before_10000_workflows.txt postgres_after_10000_workflows.txt goos: linux goarch: amd64 pkg: github.com/argoproj/argo-workflows/v3/test/e2e cpu: 12th Gen Intel(R) Core(TM) i5-12400 │ postgres_before_10000_workflows.txt │ postgres_after_10000_workflows.txt │ │ sec/op │ sec/op vs base │ WorkflowArchive/ListWorkflows-12 185.81m ± ∞ ¹ 25.06m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/ListWorkflows_with_label_selector-12 186.35m ± ∞ ¹ 25.99m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/CountWorkflows-12 42.39m ± ∞ ¹ 11.78m ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 113.6m 19.72m -82.64% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 ``` The only downside to migrating to JSONB is it can take a long time if you've got a ton of workflows (~72s on my local DB with 100,000 workflows). I'll enter a separate PR for the migration, but I'm entering this change separately so it can hopefully go out in 3.6.0. Signed-off-by: Mason Malone <[email protected]>
|
Here's the PR for the migration, which builds on top of this PR: #13779 |
agilgur5
changed the title
agilgur5
changed the title
jessesuen
approved these changes
Oct 18, 2024
@terrytangyuan Jiacheng is also apparently not in the "members" team a la #8790 (comment), so that seems to be why we can't directly "request a review" via GH |
|
Added @jiachengxu to members |
|
Thanks Terry 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
With PostgreSQL, the
argo_archived_workflows.workflowcolumn has been of typejsonever since 8a1e611, which was released as v2.5.0. Therefore, the::jsoncasts do nothing, and prevent users from improving performance by migrating to JSONB using the following query:Without the changes in this PR, running the above will massively slow down the queries, because casting JSONB to JSON is expensive. With the changes, I'm measuring a ~80% performance boost (see below). Other users have reported similar improvements.
The only downside to migrating to JSONB is it can take a long time if you've got a ton of workflows (~72s on my local DB with 100,000 workflows). I entered #13779 with the migration, but I'm entering this separately so it can hopefully go out in 3.6.0. That way, we can tell users to run the above query if they run into performance issues, without having to do a patch release with the migration.
Modifications
Replace
(workflow::json)with justworkflowVerification
See #13779