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

Change: move GET iterator time conversion out of SQL #2112

Merged
merged 23 commits into from
Feb 28, 2024

Conversation

mattmundell
Copy link
Contributor

@mattmundell mattmundell commented Nov 27, 2023

What

Move the ISO time calculations in the "GET" iterators from SQL to C.

This is for the columns creation_time and modification_time.

Note that plain C iso_time works the same as SQL iso_time (creation_time, opts.user_zone) because the C does a tzset before the iso_time. To test this use GMP like:
time o m m '<get_reports report_id="82f3531c-43cd-4453-8300-990dc423d96b" delta_report_id="ea5d9e94-4f48-4a16-b0b5-a63b637176d2" details="1" filter="rows=2 timezone=UTC"/>'

Also, removal of opt.user_zone is safe from a filtering perspective, because for the creation_time/created filter term is converted to an epoch.

Why

Having the SQL do the time conversion is slower when there are many rows with a large offset, because Postgres still does the time conversion for every row.

time o m m '<get_info type="nvt" filter="sort-reverse=created rows=10 first=80000"/>' > /tmp/info
patch: 0.3, 0.3, 0.3
main: 3.2, 3.2, 3.3

This PR leads to faster SecInfo pages in GSA when paging to the end, CPEs in particular.

References

https://www.postgresql.org/docs/current/queries-limit.html 7.6 LIMIT and OFFSET:
The rows skipped by an OFFSET clause still have to be computed inside the server; therefore a large OFFSET might be inefficient.

Copy link

Conventional Commits Report

Type Number
Changed 1

🚀 Conventional commits found.

@mattmundell mattmundell marked this pull request as ready for review November 27, 2023 18:35
@mattmundell mattmundell requested a review from a team as a code owner November 27, 2023 18:35
Copy link
Member

@timopollmeier timopollmeier left a comment

Choose a reason for hiding this comment

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

This looks like a good optimization but there is one thing I'd do differently:

I think it would be better to make the allocation of new strings that need to be freed by the caller more explicit.

My suggestion would be to return an int64 from the iterator function and pass that to a new function that does both the ISO date-time conversion and the allocation.

@mattmundell
Copy link
Contributor Author

mattmundell commented Dec 12, 2023

I think it would be better to make the allocation of new strings that need to be freed by the caller more explicit.

My suggestion would be to return an int64 from the iterator function and pass that to a new function that does both the ISO date-time conversion and the allocation.

Good idea. I've started with 1e1e147. Will get back to it next week.

@mattmundell
Copy link
Contributor Author

I think the clang-format config should change to allow breaks after the return type of short definitions.

@mattmundell
Copy link
Contributor Author

I think the clang-format config should change to allow breaks after the return type of short definitions.

Hmm, .clang-format already has AlwaysBreakAfterReturnType All.

@mattmundell
Copy link
Contributor Author

Hmm, .clang-format already has AlwaysBreakAfterReturnType All.

Due to a clang-format limitation, see llvm/llvm-project#76206 (comment). Worked around in cb983df.

Copy link
Contributor

@a-h-abdelsalam a-h-abdelsalam left a comment

Choose a reason for hiding this comment

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

Looks good! You may want to update report_configs as well after updating the branch.

src/gmp.c Show resolved Hide resolved
src/gmp.c Show resolved Hide resolved
src/utils.c Outdated Show resolved Hide resolved
@mattmundell
Copy link
Contributor Author

Looks good! You may want to update report_configs as well after updating the branch.

Thanks for going through this big PR. Report configs part done in be66e2e.

@timopollmeier timopollmeier dismissed their stale review February 28, 2024 13:16

Requested changes have been made

@a-h-abdelsalam a-h-abdelsalam merged commit 75016ed into greenbone:main Feb 28, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants