[ip-pool] simplify range overlap-check query 🤖

[jam-mad]

Simplify the IP Pool / Subnet Pool overlap-check query

Fixes #9283.

Summary

FilterOverlappingIpRanges (the query that stops two IP Pool ranges or
Subnet Pool members from overlapping) checked each table with four
separate NOT EXISTS subqueries instead of one. This collapses it to one
subquery per table, 2 total instead of 8, with no change in behavior.

Measured with EXPLAIN ANALYZE against a seeded CockroachDB instance at
100k and 1M rows per table: the collapsed version is 7-18x faster
whenever a full table scan is needed, and never slower. Pure query
rewrite, no schema or API impact.

---

The problem

Every time Nexus adds a new IP Pool range or Subnet Pool member, it needs
to check that the new range doesn't overlap anything that already
exists. Both tables share one address space, so the check has to look at
both. That check lives in
nexus/db-queries/src/db/queries/ip_pool.rs.

The existing code wrote that check as eight separate NOT EXISTS
subqueries, four per table, each built to hit one of ip_pool_range's
two single-column indexes
(lookup_pool_range_by_first_address, lookup_pool_range_by_last_address).
The reasoning in the doc comment: one OR condition would stop the
planner from using those indexes and force a full scan, so the check got
split into index-friendly pieces instead.

The linked issue proposed collapsing this down to the standard
interval-overlap test. Two intervals [a, b] and [c, d] overlap
exactly when a <= d AND c <= b, applied once per table:

-- Before (4 of these per table, 8 total):
WHERE NOT EXISTS (
  SELECT 1 FROM ip_pool_range
  WHERE first_address >= $candidate_first AND last_address <= $candidate_last
    AND time_deleted IS NULL LIMIT 1
)
-- ...and three more like it, then the same four again for subnet_pool_member

-- After (1 per table, 2 total):
WHERE NOT EXISTS (
  SELECT 1 FROM ip_pool_range
  WHERE first_address <= $candidate_last AND last_address >= $candidate_first
    AND time_deleted IS NULL LIMIT 1
)
AND NOT EXISTS (
  SELECT 1 FROM subnet_pool_member
  WHERE first_address <= $candidate_last AND last_address >= $candidate_first
    AND time_deleted IS NULL LIMIT 1
)

A comment on the issue benchmarked both versions and found the collapsed
one only a bit faster, with both falling back to full scans around 1M
rows. I re-benchmarked it on a machine less powerful than most dev
setups.

Also, the code has grown since the issue was filed. It originally only
checked ip_pool_range. A later change added subnet_pool_member to
the same check, using the same 4-subqueries-per-table pattern, which
brought the total to 8 clauses. subnet_pool_member doesn't have the
pair of single-column indexes the original reasoning depends on. Its
first_address and last_address are virtual (computed) columns
derived from a subnet CIDR, backed by only one composite index
(lookup_subnet_pool_member_by_first_and_last_address). So the "4
subqueries hit 4 indexes" argument never applied to half of what this
query checks.

---

Investigation

I set up a CockroachDB instance running the real schema from schema/crdb/dbinit.sql.
I seeded non-overlapping rows into ip_pool_range and subnet_pool_member, ran
ANALYZE on both, then ran EXPLAIN ANALYZE on both versions at 100k and 1M rows
per table (1M to match the scale in the issue's benchmark comment).

Ran this directly on a mini computer: an Intel NUC, i5-8365U (4 cores
/ 8 threads @ 1.6GHz), 7.4GB RAM. My poor NUC didn't have enough swap by default
to survive the build and test runs, so I bumped the swapfile to 24GB partway
through. Treat the absolute times as a data point, not a lab result. Both ran
on the same machine under the same load, so the relative gap is what matters:

Scenario	Rows	Old (8-clause) version	New (2-clause) version	Speedup
Candidate past all existing data (append-only case)	100k	index scan, ~0 rows, ~12ms	index scan, ~0 rows, ~1ms	~12x
Candidate past all existing data (append-only case)	1M	index scan, 0 rows, 6ms	index scan, 0 rows, 1ms	6x
Candidate among existing data (normal case), ip_pool_range	100k	full scan x4, 251k rows read, 231ms	full scan x1, 25.6k rows read, 33ms	~7x
Candidate among existing data (normal case), ip_pool_range	1M	full scan x4, 2.23M rows read, 1.4s	full scan x1, 114.7k rows read, 79ms	~18x
Candidate among existing data (normal case), subnet_pool_member	100k	full scan x4, up to 400k rows read, 265-323ms	full scan x1, up to 100k rows read, 36-96ms	~4x
Candidate among existing data (normal case), subnet_pool_member	1M	full scan x4, 2.92M rows read, 2.8s	full scan x1, 457.7k rows read, 391ms	~7x

The gap grows with table size. The results differ from what the earlier
comment on the issue found. Reason: candidate placement, not table size.
Cheap for both versions near the end of the address space. Expensive,
and growing, once placed among existing data.

Two things you can't tell from the SQL alone:

1. CockroachDB runs every NOT EXISTS subquery in the AND chain, even
   after an earlier one already proves overlap. CockroachDB's own docs
   confirm this: subqueries are always evaluated eagerly, not skipped
   like a normal AND/OR can be. Old version: pays for four scans.
   New version: pays for one.
2. The split-by-index design only helps when the candidate lands past
   all existing addresses. Any candidate among already-used addresses
   (normal, not an edge case) forces a full scan either way. The old
   version just pays for that scan four times over.

The collapsed version was never worse in anything tested, and was
consistently faster whenever a full scan was needed.

---

The change

• nexus/db-queries/src/db/queries/ip_pool.rs: collapsed the six helper
  functions building the eight subqueries down to one shared
  push_overlap_subquery plus two thin per-table wrappers. walk_ast
  now builds two NOT EXISTS blocks instead of eight. Doc comment
  rewritten to state the interval-overlap formula and the measured
  reasoning above, instead of the now-outdated "split for index use"
  justification.
• nexus/db-queries/tests/output/filter_overlapping_ip_ranges.sql:
  regenerated golden SQL fixture to match the new 2-clause version.
• schema/crdb/dbinit.sql: fixed a stale path in a comment
  (nexus/src/db/queries/ip_pool.rs -> nexus/db-queries/src/db/queries/ip_pool.rs),
  noticed while working in this area, unrelated to the fix itself.

No schema change, no index change, no API/OpenAPI change. Confirmed with
cargo xtask openapi check (122 documents checked, 0 stale).

---

Testing

Existing coverage exercised (no regressions)

• cargo nextest run -p nexus-db-queries: all 42 tests in
  datastore::ip_pool pass, including the pre-existing
  cannot_insert_ip_pool_range_which_overlaps_subnet_pool_member.
• cargo nextest run -p omicron-nexus -E 'test(test_ip_pool_range)': the
  pre-existing test_ip_pool_range_overlapping_ranges_fails (5 overlap
  cases: exact duplicate, overlap below, overlap above, candidate
  contains existing, existing contains candidate) still passes unchanged.

New tests added

The existing suite only tested that overlapping ranges get rejected,
never that a truly separate, non-overlapping neighbor gets accepted. An
off-by-one in the new check's too-strict direction (e.g. </> where
it needs <=/>=) wouldn't be caught by any existing test.

• test_ip_pool_range_adjacent_ranges_succeed
  (nexus/tests/integration_tests/ip_pools.rs): HTTP test. Posts a range
  right below and right above an existing one. Both must succeed.
• can_insert_ip_pool_range_adjacent_to_subnet_pool_member
  (nexus/db-queries/src/db/datastore/ip_pool.rs): same idea,
  cross-table. An ip_pool_range candidate one address past an existing
  subnet_pool_member subnet must succeed.
• cannot_insert_ip_pool_range_which_contains_subnet_pool_member (same
  file): the reverse case, a candidate that contains an existing subnet
  instead of landing inside one. Covers both halves of the collapsed
  check (first_address <= X and last_address >= Y).
• can_explain_filter_overlapping_ip_ranges_query (same file): smoke
  test matching this file's can_explain_* convention. Confirms the
  query is valid SQL against the real schema.

How to re-run

# SQL-text regression test (no live DB needed):
cargo test -p nexus-db-queries --lib queries::ip_pool

# Datastore-level correctness + explain-plan smoke test (needs cargo-nextest):
cargo nextest run -p nexus-db-queries -E 'test(datastore::ip_pool)'

# Full HTTP-level integration test:
cargo nextest run -p omicron-nexus -E 'test(test_ip_pool_range)'

To reproduce the EXPLAIN ANALYZE comparison end to end:

# Point this at your omicron checkout's `out/` directory (the cockroach
# binary lives here once `cargo xtask download` or a normal build has run).
export OMICRON_OUT_PATH=/path/to/omicron/out
CRDB="$OMICRON_OUT_PATH/cockroachdb/bin/cockroach"
SCHEMA="$OMICRON_OUT_PATH/../schema/crdb/dbinit.sql"

# 1. Start a throwaway single-node CockroachDB:
mkdir -p /tmp/crdb-repro && cd /tmp/crdb-repro
"$CRDB" start-single-node \
  --insecure --store=./data \
  --listen-addr=127.0.0.1:26299 --http-addr=127.0.0.1:8099 \
  --background

# 2. Load the real schema:
"$CRDB" sql --insecure --host=127.0.0.1:26299 -f "$SCHEMA"

# 3. Seed 1M non-overlapping rows into each table (10 batches of 100k
#    keeps each transaction a reasonable size; drop to 1-2 batches if you
#    just want the 100k-row numbers):
for i in $(seq 0 9); do
  start=$((i * 100000)); end=$((start + 99999))
  "$CRDB" sql --insecure --host=127.0.0.1:26299 -e "
    INSERT INTO omicron.public.ip_pool_range
      (id, time_created, time_modified, time_deleted, first_address, last_address, ip_pool_id, rcgen)
    SELECT gen_random_uuid(), now(), now(), NULL,
           ('10.0.0.0'::INET + (s*2)), ('10.0.0.0'::INET + (s*2)),
           gen_random_uuid(), 1
    FROM generate_series($start, $end) AS s;"
done
for i in $(seq 0 9); do
  start=$((i * 100000)); end=$((start + 99999))
  "$CRDB" sql --insecure --host=127.0.0.1:26299 -e "
    INSERT INTO omicron.public.subnet_pool_member
      (id, time_created, time_modified, time_deleted, subnet_pool_id, subnet, min_prefix_length, max_prefix_length, rcgen)
    SELECT gen_random_uuid(), now(), now(), NULL, gen_random_uuid(),
           (host(('10.0.0.0'::INET + (s*2))) || '/32')::INET, 32, 32, 1
    FROM generate_series($start, $end) AS s;"
done

# 4. Update table statistics so the planner has real numbers to work with:
"$CRDB" sql --insecure --host=127.0.0.1:26299 -e "
  ANALYZE omicron.public.ip_pool_range;
  ANALYZE omicron.public.subnet_pool_member;"

# 5. Run both versions against a candidate placed among existing
#    data (offset 500001-500003: odd, so it never lands on a seeded
#    address, but sits in the densest part of the range):
MID_FIRST="'10.0.0.0'::INET + 500001"
MID_LAST="'10.0.0.0'::INET + 500003"

# OLD (8-clause) version, ip_pool_range half:
"$CRDB" sql --insecure --host=127.0.0.1:26299 -e "
EXPLAIN ANALYZE
SELECT 1 WHERE
NOT EXISTS (SELECT 1 FROM omicron.public.ip_pool_range WHERE first_address >= ($MID_FIRST) AND last_address <= ($MID_LAST) AND time_deleted IS NULL LIMIT 1)
AND NOT EXISTS (SELECT 1 FROM omicron.public.ip_pool_range WHERE first_address >= ($MID_FIRST) AND last_address <= ($MID_LAST) AND time_deleted IS NULL LIMIT 1)
AND NOT EXISTS (SELECT 1 FROM omicron.public.ip_pool_range WHERE ($MID_FIRST) >= first_address AND ($MID_FIRST) <= last_address AND time_deleted IS NULL LIMIT 1)
AND NOT EXISTS (SELECT 1 FROM omicron.public.ip_pool_range WHERE ($MID_LAST) >= first_address AND ($MID_LAST) <= last_address AND time_deleted IS NULL LIMIT 1);"

# NEW (2-clause) version, ip_pool_range half:
"$CRDB" sql --insecure --host=127.0.0.1:26299 -e "
EXPLAIN ANALYZE
SELECT 1 WHERE
NOT EXISTS (SELECT 1 FROM omicron.public.ip_pool_range WHERE time_deleted IS NULL
            AND first_address <= ($MID_LAST) AND last_address >= ($MID_FIRST) LIMIT 1);"

# Swap `ip_pool_range` for `subnet_pool_member` in both queries above to
# get the second table's numbers.

# 6. Clean up:
pkill -f "cockroach start-single-node"
rm -rf /tmp/crdb-repro

Look for spans: FULL SCAN versus an actual index name in each plan's
table: line, and compare the KV rows read: counts, to see the same
result shown in the table above.

---

Risk / non-goals

• No behavior change. Same logic as the split version, checked by hand.
  Performance change only.
• No new index proposed. Both tables' existing indexes go unused by
  either version once a candidate isn't at the tail of the address
  space. A new index could help if full scans become a real bottleneck
  in production. Out of scope here.
• Tested at 100k and 1M rows per table, matching the issue's earlier
  benchmark. Holds at both sizes. Bigger gap at 1M (~18x and ~7x), not
  smaller.