Improve SQL queries and performance to check for PTF packages (bsc#1225619) #9065

meaksh · 2024-07-17T13:11:50Z

What does this PR change?

This PR improves the performance to determine which packages are PTFs or part of a PTF, as in large environments with tons of packages, these SQL operations, and particularly the susePackageExcludingPartOfPtf SQL view can cause very huge delays in the context of Content Lifecycle Management (CLM) operations.

In order to achieve a better performance, this PR is doing the following changes:

Add new columns is_ptf and is_part_of_ptf to rhnPackage table.
These attributes are set once during package import, so no need to calculate them on each request.
The susePackageExcludingPartOfPtf view and other SQL queries have been adapted to use the new attributes.

GUI diff

No difference.

DONE

Documentation

No documentation needed: only internal and user invisible changes
DONE

Test coverage

No tests: already covered
DONE

Links

Issue(s): https://github.com/SUSE/spacewalk/issues/24426
Port(s): https://github.com/SUSE/spacewalk/pull/24882

DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests" (Test skipped, there are no changes to test)

Before you merge

Check How to branch and merge properly!

github-actions · 2024-07-17T13:12:03Z

👋 Hello! Thanks for contributing to our project.
Acceptance tests will take some time (aprox. 1h), please be patient ☕
You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/9065/checks
Once tests finish, if they fail, you can check 👀 the cucumber report. See the link at the output of the action.
You can also check the artifacts section, which contains the logs at https://github.com/uyuni-project/uyuni/pull/9065/checks.

If you are unsure the failing tests are related to your code, you can check the "reference jobs". These are jobs that run on a scheduled time with code from master. If they fail for the same reason as your build, it means the tests or the infrastructure are broken. If they do not fail, but yours do, it means it is related to your code.

Reference tests:

KNOWN ISSUES

Sometimes the build can fail when pulling new jar files from download.opensuse.org . This is a known limitation. Given this happens rarely, when it does, all you need to do is rerun the test. Sorry for the inconvenience.

For more tips on troubleshooting, see the troubleshooting guide.

Happy hacking!
⚠️ You should not merge if acceptance tests fail to pass. ⚠️

agraul

The change looks fine for me. Since the PR claims to improve performance, can you please provide data that backs the claim up? How much performance is gained?

python/spacewalk/server/importlib/packageImport.py

github-actions · 2024-07-18T10:27:17Z

Suggested tests to cover this Pull Request

sle_ssh_minion
min_salt_install_with_staging
min_salt_formulas
min_salt_install_package
srv_monitoring
min_salt_openscap_audit
min_salt_lock_packages
srv_rename_hostname
proxy_branch_network
minkvm_guests
min_monitoring
min_recurring_action
srv_check_channels_page
min_rhlike_openscap_audit
minssh_ansible_control_node
allcli_action_chain
srv_salt_download_endpoint
min_deblike_salt_install_package
min_salt_user_states
min_ssh_tunnel
allcli_sanity
min_action_chain
buildhost_docker_auth_registry
buildhost_osimage_build_image
min_ansible_control_node
min_bootstrap_script
min_activationkey
allcli_update_activationkeys
min_salt_pkgset_beacon
proxy_as_pod_basic_tests
buildhost_docker_build_image
srv_content_lifecycle
srv_add_rocky8_repositories
srv_check_sync_source_packages
min_cve_audit
sle_minion
min_salt_software_states
srv_push_package
minssh_action_chain
minssh_salt_install_package
buildhost_bootstrap
minssh_bootstrap_api
proxy_retail_pxeboot_and_mass_import
proxy_cobbler_pxeboot
min_retracted_patches
min_rhlike_salt_install_package_and_patch
min_deblike_salt_install_with_staging

schema/spacewalk/common/tables/rhnPackage.sql

meaksh · 2024-07-18T11:19:38Z

The change looks fine for me. Since the PR claims to improve performance, can you please provide data that backs the claim up? How much performance is gained?

Thanks for the review!

During the investigations on this L3, customer confirmed that due susePackageExcludingPartOfPtf view calculations was actually causing a bottleneck [1]. A TEST package was created to bypass susePackageExcludingPartOfPtf view and the performace increases significantly:

<<< before PTF >>> : 18816 | 01:32:12.56153  | admsmdb | INSERT INTO rhnServerNeededCache (server_id, package_id, channel_id) 

<<< with PTF  >>> : 18816 | 00:00:02.508874 | admsmdb | INSERT INTO rhnServerNeededCache (server_id, errata_id, package_id, channel_id)

There was later a discussion with Ricardo and it was agreed to go with this proposed solution here (moving calculation of ptf to import package time and store it in DB) [2]

[1] https://bugzilla.suse.com/show_bug.cgi?id=1225619#c26
[2] https://bugzilla.suse.com/show_bug.cgi?id=1225619#c27

cbbayburt

Thanks a lot for the fix, @meaksh. This saves us a huge sequential scan on rhnPackage on every operation involving packages. The performance gain is obvious.

cbbayburt · 2024-07-18T16:51:39Z

Thinking out loud here: Indexing on boolean fields where the values are roughly 50/50 is generally useless, but in our case, only a tiny fraction of the packages would be PTFs. In this case, we can further benefit from some partial indexing (WHERE NOT is_ptf) but at this point I have no idea how much real value this brings.

A possible solution could be to define partial indexes like below:

CREATE INDEX idx1 ON rhnPackage(id) WHERE NOT is_ptf;
CREATE INDEX idx2 ON rhnPackage(id) WHERE NOT is_part_of_ptf;

...-schema-5.0.8-to-susemanager-schema-5.0.9/300-replace-susePackageExcludingPartOfPtf-view.sql

java/code/src/com/redhat/rhn/common/db/datasource/xml/Package_queries.xml

java/code/src/com/redhat/rhn/domain/rhnpackage/Package.java

schema/spacewalk/common/tables/rhnPackage.sql

agraul

The Git history could use some cleanup, there are commits that just fix earlier commits. I recommend squashing them together (without keeping messages like "Fix Python code formatting issues raised by black")

rjmateus

LGTM

…25619) Do not export PTF flags as they are calculated at import time Make Hibernate aware of new attributes and fix junit tests Use boolean for storing PTF flags in the DB Add missing indexes to the rhnPackage schema definition Make 'package_retracted_and_ptf_details' query more efficient

meaksh · 2024-07-24T14:27:27Z

FYI the current failure for spacecmd_unittests is just noise here cause by the IT hickup we had yesterday. In fact, "spacecmd" tests make no sense for the changes in this PR.

meaksh requested review from a team as code owners July 17, 2024 13:11

meaksh requested review from cbbayburt and m-czernek and removed request for a team July 17, 2024 13:11

github-actions bot added java java_lint_checkstyle java_pgsql_tests schema_migration_test_pgsql database schema_migration_test_oracle labels Jul 17, 2024

agraul reviewed Jul 17, 2024

View reviewed changes

python/spacewalk/server/importlib/packageImport.py Outdated Show resolved Hide resolved

meaksh requested a review from a team as a code owner July 18, 2024 10:26

github-actions bot added API data-model xmlrpc old-ui labels Jul 18, 2024

meaksh force-pushed the master-improve-perforance-to-check-for-ptf-packages branch from 688c961 to 9db26c4 Compare July 18, 2024 10:39

aaannz reviewed Jul 18, 2024

View reviewed changes

schema/spacewalk/common/tables/rhnPackage.sql Outdated Show resolved Hide resolved

cbbayburt previously approved these changes Jul 18, 2024

View reviewed changes

cbbayburt reviewed Jul 18, 2024

View reviewed changes

...-schema-5.0.8-to-susemanager-schema-5.0.9/300-replace-susePackageExcludingPartOfPtf-view.sql Show resolved Hide resolved

rjmateus reviewed Jul 19, 2024

View reviewed changes

java/code/src/com/redhat/rhn/common/db/datasource/xml/Package_queries.xml Outdated Show resolved Hide resolved

java/code/src/com/redhat/rhn/domain/rhnpackage/Package.java Outdated Show resolved Hide resolved

schema/spacewalk/common/tables/rhnPackage.sql Outdated Show resolved Hide resolved

meaksh dismissed cbbayburt’s stale review via b4335f0 July 22, 2024 13:13

meaksh force-pushed the master-improve-perforance-to-check-for-ptf-packages branch from b4335f0 to 4b6e7cf Compare July 22, 2024 14:22

rjmateus previously approved these changes Jul 22, 2024

View reviewed changes

meaksh dismissed rjmateus’s stale review via a616b0d July 22, 2024 15:39

agraul approved these changes Jul 23, 2024

View reviewed changes

rjmateus approved these changes Jul 23, 2024

View reviewed changes

aaannz approved these changes Jul 23, 2024

View reviewed changes

meaksh force-pushed the master-improve-perforance-to-check-for-ptf-packages branch from a616b0d to 954d1a4 Compare July 23, 2024 14:38

meaksh added the merge-candidate Meaning it needs to be considered for merging when the master branch is frozen label Jul 23, 2024

cbbayburt approved these changes Jul 23, 2024

View reviewed changes

deneb-alpha merged commit 64c6afb into master Jul 24, 2024
37 of 38 checks passed

deneb-alpha deleted the master-improve-perforance-to-check-for-ptf-packages branch July 24, 2024 14:35

meaksh mentioned this pull request Jul 29, 2024

Move schema upgrade file to the right directory #9119

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve SQL queries and performance to check for PTF packages (bsc#1225619) #9065

Improve SQL queries and performance to check for PTF packages (bsc#1225619) #9065

meaksh commented Jul 17, 2024 •

edited by rjmateus

Loading

github-actions bot commented Jul 17, 2024

agraul left a comment

github-actions bot commented Jul 18, 2024

meaksh commented Jul 18, 2024

cbbayburt left a comment

cbbayburt commented Jul 18, 2024 •

edited

Loading

agraul left a comment

rjmateus left a comment

meaksh commented Jul 24, 2024

Improve SQL queries and performance to check for PTF packages (bsc#1225619) #9065

Improve SQL queries and performance to check for PTF packages (bsc#1225619) #9065

Conversation

meaksh commented Jul 17, 2024 • edited by rjmateus Loading

What does this PR change?

GUI diff

Documentation

Test coverage

Links

Changelogs

Re-run a test

Before you merge

github-actions bot commented Jul 17, 2024

agraul left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2024

meaksh commented Jul 18, 2024

cbbayburt left a comment

Choose a reason for hiding this comment

cbbayburt commented Jul 18, 2024 • edited Loading

agraul left a comment

Choose a reason for hiding this comment

rjmateus left a comment

Choose a reason for hiding this comment

meaksh commented Jul 24, 2024

meaksh commented Jul 17, 2024 •

edited by rjmateus

Loading

cbbayburt commented Jul 18, 2024 •

edited

Loading