BUGFIX: full table scan preventing looking up bgp configs by name #6477
Conversation
|
Why didn’t a test hit this? Not enough rows in the table to trigger it? |
|
@david-crespo The size of the table doesn't matter, but I think you need to run a query looking for a record by name. Any tests we have must be looking up by primary key. |
|
But speaking of, @internet-diglett it might be worth writing a regression test in this PR. |
|
@bnaecker yeah, I was looking into beefing up our tests across the board for networking APIs, but some of that work also overlaps with another PR I'm working on so I don't know which I'm going to put it in at the moment 😂 |
|
Please check if this will also fix #6619 Thank you! |
|
I don't think this PR will fix 6619. The error in that issue is but this PR is adding an index on the |
rcgoodfellow
force-pushed
the
bugfix-6471
branch
from
6fb119f
to
1774319
Compare
|
We weren't missing an index for looking up BGP configs by name, what we were missing was the I added a test that covers the API calls used in #6471 and #6619. Both of those issues are now addressed by this PR. The test I wrote also surfaced the fact that the diesel |
| async fn test_delete_bgp_config_delete_by_name() { | ||
| let logctx = dev::test_setup_log("test_delete_bgp_config_by_name"); |
There was a problem hiding this comment.
Should rename for clarity, since this deletes both bgp config and announce set.
| CREATE INDEX IF NOT EXISTS lookup_bgp_config_by_bgp_announce_set_id ON omicron.public.bgp_config ( | ||
| bgp_announce_set_id | ||
| ) WHERE | ||
| time_deleted IS NULL; |
There was a problem hiding this comment.
Double check if needed for line 2802
There was a problem hiding this comment.
Yes this is an index on a different field that is not already in the primary key so both the index and time deleted constraint are needed.
| CREATE INDEX IF NOT EXISTS lookup_bgp_config_by_bgp_announce_set_id ON omicron.public.bgp_config ( | ||
| bgp_announce_set_id | ||
| ) WHERE | ||
| time_deleted IS NULL; |
There was a problem hiding this comment.
Double check if this is needed for schema/crdb/lookup-bgp-config-indexes/up02.sql
There was a problem hiding this comment.
Same comment as above, yes this is needed.
| .count() | ||
| .execute_async(&conn) | ||
| .get_result_async(&conn) |
There was a problem hiding this comment.
You don't need to select all the rows if you just want to check whether there are any. Something like this should do it:
let configs_exist = diesel::dsl::select(diesel::dsl::exists(
sps_bgp_peer_config_dsl::switch_port_settings_bgp_peer_config
.filter(sps_bgp_peer_config::bgp_config_id.eq(id))
))
.get_result_async::<bool>(&conn)
.await?;Compare:
omicron/nexus/db-queries/src/db/datastore/ip_pool.rs
Lines 892 to 898 in d00b684
| .count() | ||
| .execute_async(&conn) | ||
| .get_result_async(&conn) |
There was a problem hiding this comment.
same deal here
let in_use = diesel::dsl::select(diesel::dsl::exists(
bgp_config_dsl::bgp_config
.filter(bgp_config::bgp_announce_set_id.eq(id))
.filter(bgp_config::time_deleted.is_null())
))
.get_result_async::<bool>(&conn)
.await?;
Related
resolves #6471