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

Add functional index to ipam.ip_address model/table #11519

Closed
tyler-8 opened this issue Jan 16, 2023 · 4 comments
Closed

Add functional index to ipam.ip_address model/table #11519

tyler-8 opened this issue Jan 16, 2023 · 4 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Milestone

Comments

@tyler-8
Copy link
Contributor

tyler-8 commented Jan 16, 2023

NetBox version

v3.4.2

Feature type

Change to existing functionality

Proposed functionality

Create a functional index on the ipam.ip_address table. Which indexes the output of CAST(HOST(ipam.ipaddress)) so it isn't called on every execution of the query - which can be computationally expensive.

Use case

I've detailed my findings in this discussion thread.

Queries/searches for IPAddress filtered by parent Prefixes can be CPU intensive to the database and, depending on the parent prefix and IP addresses present, can be very inefficient when searching through hundreds of thousands of IPs (~260K) and only returning a few. Query time itself isn't terrible, but there is a noticeable uptick in CPU & DB worker threads/processes generated by this type of query compared to others - and that can be of concern to larger/high-volume instances.

The culprit is mostly with calling CAST(HOST("ipam_ipaddress"."address") AS INET)

return 'CAST(HOST(%s) AS INET) <<= %s' % (lhs, rhs), params
and the resulting sequential scans it must perform. Indexing address with btree or inet_ops makes little difference in performance and query cost.

But the above functional index is 1/3 the query cost (2264 vs 6632) and the query time is <=2ms (vs ~70ms). I observed a ~5MB increase (+8%) in table disk size (260K IPs) with this index, and new data insertion time seemed un-impacted in any noticeable way (Using the REST API: (0.0185s without index, 0.0166s with index)).

Additional metrics

Querying the /ipam/ip-addresses/ endpoint ~2K times with different prefixes as parent and 3 separate client processes:

  • No index: 6m18s
  • With index: 4m37s

Database changes

New index on address field.

CREATE INDEX ipam_ipaddress_address_host_idx ON ipam_ipaddress (cast(host(address) as inet));

One possible way is in a manually-defined migrations file:

operations = [
        migrations.RunSQL(
            sql="CREATE INDEX ipam_ipaddress_address_host_idx ON ipam_ipaddress (cast(host(address) as inet))",
            reverse_sql='DROP INDEX ipam_ipaddress_address_host_idx ON ipam_ipaddress'
        )
    ]

or it might be possible on the model itself via defining an index expression.

External dependencies

N/A

And thanks & credit to @candlerb & @kkthxbye-code for setting me down the path

@tyler-8 tyler-8 added the type: feature Introduction of new functionality to the application label Jan 16, 2023
@tyler-8
Copy link
Contributor Author

tyler-8 commented Jan 17, 2023

Here are the CPU stats of the above 2K API queries against /ipam/ip-addresses/

Output of ps -eo user,pid,pcpu,cmd --sort=-pcpu | head -n 25 on the database server.

with index

user         pid      cpu    command
postgres     <pid>    2.0    postgres: <...> idle
postgres     <pid>    1.8    postgres: <...> idle
postgres     <pid>    1.5    postgres: <...> idle
postgres     <pid>    1.4    postgres: <...> idle
postgres     <pid>    1.3    postgres: <...> idle
postgres     <pid>    0.9    postgres: <...> idle
postgres     <pid>    0.9    postgres: <...> idle
postgres     <pid>    0.9    postgres: <...> idle

without index

user         pid      cpu     command
postgres     <pid>    18.1    postgres:    <...>    idle
postgres     <pid>    16.4    postgres:    <...>    SELECT
postgres     <pid>    14.1    postgres:    <...>    idle
postgres     <pid>    14.0    postgres:    <...>    idle
postgres     <pid>    12.5    postgres:    <...>    idle
postgres     <pid>    11.5    postgres:    <...>    idle
postgres     <pid>     9.4    postgres:    <...>    idle
postgres     <pid>     5.7    postgres:    <...>    idle

and of course this is just while the test activity is going on in a test environment, not accounting for other workflows and uses happening simultaneously in a production environment. This is just to show the elevated CPU impact.

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Jan 18, 2023
@jeremystretch
Copy link
Member

Does adding the index require modifying the NetHostContained lookup? Or will PostgreSQL automatically employ the index as-is?

@tyler-8
Copy link
Contributor Author

tyler-8 commented Feb 18, 2023

Does adding the index require modifying the NetHostContained lookup? Or will PostgreSQL automatically employ the index as-is?

In my testing, adding the index (and the subsequent performance benefits) was invisible to NetBox and didn't require any code changes (beyond creating the index itself).

@jeremystretch jeremystretch added status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels May 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Do not attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our contributing guide.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Aug 2, 2023
@jeremystretch jeremystretch self-assigned this Aug 2, 2023
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation pending closure Requires immediate attention to avoid being closed for inactivity labels Aug 2, 2023
@jeremystretch jeremystretch added this to the v3.6 milestone Aug 2, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

No branches or pull requests

2 participants