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

matcher: Use latest_vuln view in lieu of querying vuln table #947

Merged
merged 1 commit into from May 25, 2023

Conversation

crozzy
Copy link
Contributor

@crozzy crozzy commented May 23, 2023

This view has always existed but never been used (AFAICT). Lately it has been problematic to query all of the vuln table as some vulnerabilities associated with older update_operations we're deleted or duplicated in new update_operations.

TODO

  • Database performance analysis
  • Tests

@crozzy crozzy force-pushed the use-latest_vuln-view branch 2 times, most recently from 35df509 to 3b2317e Compare May 24, 2023 16:49
@crozzy
Copy link
Contributor Author

crozzy commented May 24, 2023

Typical explain pre-change:

Bitmap Heap Scan on vuln  (cost=158.99..163.02 rows=1 width=1027)
  Recheck Cond: (((package_name = 'libgcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text)) OR ((package_name = 'gcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text)))
  Filter: (((package_name = 'libgcc'::text) AND (package_kind = 'binary'::text)) OR ((package_name = 'gcc'::text) AND (package_kind = 'source'::text)))
  ->  BitmapOr  (cost=158.99..158.99 rows=1 width=0)
        ->  Bitmap Index Scan on vuln_lookup_idx  (cost=0.00..79.50 rows=1 width=0)
              Index Cond: ((package_name = 'libgcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text))
        ->  Bitmap Index Scan on vuln_lookup_idx  (cost=0.00..79.50 rows=1 width=0)
              Index Cond: ((package_name = 'gcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text))

Typical explain post-change:

  ->  Hash  (cost=185.94..185.94 rows=3 width=1035)
        ->  Nested Loop  (cost=159.43..185.94 rows=3 width=1035)
              ->  Bitmap Heap Scan on vuln v  (cost=158.99..163.02 rows=1 width=1027)
                    Recheck Cond: (((package_name = 'libgcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text)) OR ((package_name = 'gcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text)))
                    Filter: (((package_name = 'libgcc'::text) AND (package_kind = 'binary'::text)) OR ((package_name = 'gcc'::text) AND (package_kind = 'source'::text)))
                    ->  BitmapOr  (cost=158.99..158.99 rows=1 width=0)
                          ->  Bitmap Index Scan on vuln_lookup_idx  (cost=0.00..79.50 rows=1 width=0)
                                Index Cond: ((package_name = 'libgcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text))
                          ->  Bitmap Index Scan on vuln_lookup_idx  (cost=0.00..79.50 rows=1 width=0)
                                Index Cond: ((package_name = 'gcc'::text) AND (package_module = ''::text) AND (repo_name = 'cpe:/a:redhat:rhel:8.3::appstream'::text))
              ->  Index Scan using uo_vuln_vuln_idx on uo_vuln  (cost=0.44..22.87 rows=5 width=16)
                    Index Cond: (vuln = v.id)

@crozzy
Copy link
Contributor Author

crozzy commented May 24, 2023

The number of update_operations / updater should be bounded as long as GC is on. This has the potential to cause increased latency in vulnerability report generation for cases where GC is turned off.

@crozzy crozzy marked this pull request as ready for review May 24, 2023 22:02
@crozzy crozzy requested a review from a team as a code owner May 24, 2023 22:02
@crozzy crozzy requested review from RTann and removed request for a team May 24, 2023 22:02
RTann
RTann previously approved these changes May 24, 2023
Copy link
Contributor

@RTann RTann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for a sanity check, did you check that the unit test you added fails when we don't use latest_vulns?

@crozzy
Copy link
Contributor Author

crozzy commented May 24, 2023

Just for a sanity check, did you check that the unit test you added fails when we don't use latest_vulns?

yes

@crozzy crozzy requested a review from hdonnay May 25, 2023 15:24
Copy link
Member

@hdonnay hdonnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits that should be easy to knock out in a rebase.

LGTM after that.

datastore/postgres/querybuilder_test.go Outdated Show resolved Hide resolved
datastore/postgres/querybuilder_test.go Outdated Show resolved Hide resolved
This view has always existed but never been used (AFAICT). Lately it
has been problematic to query all of the vuln table as some
vulnerabilities associated with older update_operations we're deleted
or duplicated in new update_operations.

Signed-off-by: crozzy <joseph.crosland@gmail.com>
@crozzy crozzy requested a review from hdonnay May 25, 2023 17:28
@crozzy crozzy merged commit efec04b into quay:main May 25, 2023
7 checks passed
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.

None yet

3 participants