diff --git a/.sqlx/query-8af19a065237ef753ac64eb6e2487dfd3cdf65af898823dea092f80b8c47fbbb.json b/.sqlx/query-8af19a065237ef753ac64eb6e2487dfd3cdf65af898823dea092f80b8c47fbbb.json new file mode 100644 index 00000000..f60e3481 --- /dev/null +++ b/.sqlx/query-8af19a065237ef753ac64eb6e2487dfd3cdf65af898823dea092f80b8c47fbbb.json @@ -0,0 +1,29 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT DISTINCT image AS \"image!\" FROM oci_manifest\n WHERE repository_id = ANY($1) AND image = ANY($2)\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "image!", + "type_info": "Text", + "origin": { + "Table": { + "table": "oci_manifest", + "name": "image" + } + } + } + ], + "parameters": { + "Left": [ + "Int8Array", + "TextArray" + ] + }, + "nullable": [ + false + ] + }, + "hash": "8af19a065237ef753ac64eb6e2487dfd3cdf65af898823dea092f80b8c47fbbb" +} diff --git a/.sqlx/query-cf45729a40c2c11a545304532bb384e12a5d0744c38fa8ac0afc6506cef54623.json b/.sqlx/query-cf45729a40c2c11a545304532bb384e12a5d0744c38fa8ac0afc6506cef54623.json new file mode 100644 index 00000000..51d81443 --- /dev/null +++ b/.sqlx/query-cf45729a40c2c11a545304532bb384e12a5d0744c38fa8ac0afc6506cef54623.json @@ -0,0 +1,52 @@ +{ + "db_name": "PostgreSQL", + "query": "\n WITH names AS (\n SELECT DISTINCT image FROM oci_manifest\n WHERE repository_id = $1 AND image > $3 AND strpos(image, $2) > 0\n ORDER BY image\n LIMIT $4\n )\n SELECT n.image AS \"image!\",\n s.tag_count AS \"tag_count!\",\n s.manifest_count AS \"manifest_count!\",\n s.last_pushed_at AS \"last_pushed_at!\"\n FROM names n\n CROSS JOIN LATERAL (\n SELECT count(DISTINCT t.tag) AS tag_count,\n count(DISTINCT m.id) AS manifest_count,\n max(m.created_at) AS last_pushed_at\n FROM oci_manifest m\n LEFT JOIN oci_tag t ON t.manifest_id = m.id\n WHERE m.repository_id = $1 AND m.image = n.image\n ) s\n ORDER BY n.image\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "image!", + "type_info": "Text", + "origin": { + "Table": { + "table": "oci_manifest", + "name": "image" + } + } + }, + { + "ordinal": 1, + "name": "tag_count!", + "type_info": "Int8", + "origin": "Expression" + }, + { + "ordinal": 2, + "name": "manifest_count!", + "type_info": "Int8", + "origin": "Expression" + }, + { + "ordinal": 3, + "name": "last_pushed_at!", + "type_info": "Timestamptz", + "origin": "Expression" + } + ], + "parameters": { + "Left": [ + "Int8", + "Text", + "Text", + "Int8" + ] + }, + "nullable": [ + false, + null, + null, + null + ] + }, + "hash": "cf45729a40c2c11a545304532bb384e12a5d0744c38fa8ac0afc6506cef54623" +} diff --git a/crates/db/src/oci_dao.rs b/crates/db/src/oci_dao.rs index ed9334dc..809584eb 100644 --- a/crates/db/src/oci_dao.rs +++ b/crates/db/src/oci_dao.rs @@ -1329,6 +1329,75 @@ pub async fn list_images( .await } +/// The next `limit` images of a repository whose name contains `query` (all +/// when empty), by name, after the image `after` (from the first when +/// empty): a keyset scan over the `(repository_id, image, digest)` index, +/// then each image's counts from its own manifests (a lookup per image, so +/// that no plan reads the repository's other manifests). Reading a +/// repository's images batch by batch costs each batch alone, not the +/// batches before it as `OFFSET` does. +pub async fn list_images_after<'e, E>( + executor: E, + repository_id: i64, + query: &str, + after: &str, + limit: i64, +) -> sqlx::Result> +where + E: sqlx::PgExecutor<'e>, +{ + let query = query.to_ascii_lowercase(); + sqlx::query_as!( + ImageSummary, + r#" + WITH names AS ( + SELECT DISTINCT image FROM oci_manifest + WHERE repository_id = $1 AND image > $3 AND strpos(image, $2) > 0 + ORDER BY image + LIMIT $4 + ) + SELECT n.image AS "image!", + s.tag_count AS "tag_count!", + s.manifest_count AS "manifest_count!", + s.last_pushed_at AS "last_pushed_at!" + FROM names n + CROSS JOIN LATERAL ( + SELECT count(DISTINCT t.tag) AS tag_count, + count(DISTINCT m.id) AS manifest_count, + max(m.created_at) AS last_pushed_at + FROM oci_manifest m + LEFT JOIN oci_tag t ON t.manifest_id = m.id + WHERE m.repository_id = $1 AND m.image = n.image + ) s + ORDER BY n.image + "#, + repository_id, + query, + after, + limit, + ) + .fetch_all(executor) + .await +} + +/// Which of `images` one of the repositories `repository_ids` holds. +pub async fn images_held( + pool: &PgPool, + repository_ids: &[i64], + images: &[String], +) -> sqlx::Result> { + sqlx::query_scalar!( + r#" + SELECT DISTINCT image AS "image!" FROM oci_manifest + WHERE repository_id = ANY($1) AND image = ANY($2) + "#, + repository_ids, + images, + ) + .fetch_all(pool) + .await +} + /// How many images [`list_images`] would list in total. pub async fn count_images(pool: &PgPool, repository_id: i64, query: &str) -> sqlx::Result { let query = query.to_ascii_lowercase(); diff --git a/crates/db/tests/oci_images.rs b/crates/db/tests/oci_images.rs new file mode 100644 index 00000000..62c96058 --- /dev/null +++ b/crates/db/tests/oci_images.rs @@ -0,0 +1,98 @@ +//! `oci_dao`'s image listings against a real Postgres: reading every image +//! of a repository a batch at a time by name (#596). + +mod common; + +use forge_db::oci_dao; +use forge_db::repository_dao::{self, NewRepository}; + +use common::{pool, unique}; + +/// Index entries and table rows of `oci_manifest` this transaction read. +async fn rows_read(conn: &mut sqlx::PgConnection) -> i64 { + sqlx::query_scalar( + r#" + SELECT coalesce(sum(pg_stat_get_xact_tuples_returned(i.indexrelid)), 0)::bigint + + coalesce(pg_stat_get_xact_tuples_returned('oci_manifest'::regclass), 0) + FROM pg_index i + WHERE i.indrelid = 'oci_manifest'::regclass + "#, + ) + .fetch_one(conn) + .await + .unwrap() +} + +/// Reading a repository's images batch by batch with `list_images_after` +/// reads each manifest a bounded number of times, however many batches came +/// before: an `OFFSET` page reads every image before it again, so reading +/// them all costs the square of their number. +#[tokio::test] +async fn reading_every_image_by_name_reads_each_manifest_a_few_times() { + let pool = pool().await; + let repo = repository_dao::insert_repository( + &pool, + NewRepository::hosted(format!("images-{}", unique())), + ) + .await + .unwrap() + .id; + let sha = format!("{:064x}", (u128::from(unique()) << 64) | 0x596); + sqlx::query( + "INSERT INTO blob (sha256, size, s3_key, state) VALUES ($1, 1, $1, 'committed') \ + ON CONFLICT DO NOTHING", + ) + .bind(&sha) + .execute(&pool) + .await + .unwrap(); + const IMAGES: i64 = 10_000; + sqlx::query( + r#" + INSERT INTO oci_manifest (repository_id, image, digest, blob_sha256, media_type, size) + SELECT $1, 'team/app-' || lpad(n::text, 5, '0'), 'sha256:' || $2, $2, + 'application/vnd.oci.image.manifest.v1+json', 1 + FROM generate_series(0, $3 - 1) AS n + "#, + ) + .bind(repo) + .bind(&sha) + .bind(IMAGES) + .execute(&pool) + .await + .unwrap(); + sqlx::query("ANALYZE oci_manifest") + .execute(&pool) + .await + .unwrap(); + + let mut tx = pool.begin().await.unwrap(); + let before = rows_read(&mut tx).await; + let mut listed = Vec::new(); + let mut after = String::new(); + loop { + let batch = oci_dao::list_images_after(&mut *tx, repo, "app", &after, 1000) + .await + .unwrap(); + let full = batch.len() == 1000; + let Some(last) = batch.last() else { + break; + }; + after = last.image.clone(); + listed.extend(batch.into_iter().map(|image| image.image)); + if !full { + break; + } + } + let read = rows_read(&mut tx).await - before; + assert_eq!(listed.len() as i64, IMAGES); + assert!( + listed.windows(2).all(|w| w[0] < w[1]), + "in order, once each" + ); + assert_eq!(listed[0], "team/app-00000"); + assert!( + read < 3 * IMAGES, + "listing {IMAGES} images read {read} manifest rows" + ); +} diff --git a/crates/server/src/browse_images.rs b/crates/server/src/browse_images.rs index 72d0de90..5c7661e5 100644 --- a/crates/server/src/browse_images.rs +++ b/crates/server/src/browse_images.rs @@ -329,6 +329,12 @@ async fn image_page( /// group's content selector applies, and its guard decides per image which /// members serve it (the registry's `GroupMembers`). An image is listed /// with the counts of the highest priority member serving it. +/// +/// Each leaf's images are read a batch at a time by name (a keyset scan, +/// [`oci_dao::list_images_after`]), so a listing costs what the members +/// hold, once. Whether an owner holds an image is asked only of the images +/// a leaf that is no owner would list, a batch at a time: an owner's other +/// images are never read. async fn group_image_page( state: &AppState, principal: &Principal, @@ -345,31 +351,59 @@ async fn group_image_page( .group_members(principal, group, 0) .await .map_err(failure_response)?; - // The images an owner has, whatever the caller may read: with - // `prefer_hosted`, only owners serve them. - let mut owned: HashSet = HashSet::new(); - if members.asks_owners() { - for owner in members.owner_ids() { - for_each_image(state, owner, &query.q, |image| { - owned.insert(image.image); - }) - .await - .map_err(db_error)?; - } - } + let owners = members.owner_ids(); let mut merged: BTreeMap = BTreeMap::new(); for (position, leaf) in members.leaves() { - for_each_image(state, leaf.id, &query.q, |image| { - if merged.contains_key(&image.image) || !group_filter.allows(&image.image) { - return; + // An owner serves what it holds whoever else holds it. + let asks_owners = members.asks_owners() && !owners.contains(&leaf.id); + let mut after = String::new(); + loop { + let batch = oci_dao::list_images_after( + &state.pool, + leaf.id, + &query.q, + &after, + IMAGE_BATCH_SIZE, + ) + .await + .map_err(db_error)?; + let full = batch.len() as i64 == IMAGE_BATCH_SIZE; + let Some(last) = batch.last() else { + break; + }; + after = last.image.clone(); + let candidates: Vec = batch + .into_iter() + .filter(|image| { + !merged.contains_key(&image.image) && group_filter.allows(&image.image) + }) + .collect(); + // Owned or not makes a difference only where nothing else + // (a reserved prefix) keeps the image to the owners already. + let ask: Vec = candidates + .iter() + .filter(|image| asks_owners && !members.owners_only(&image.image, false)) + .map(|image| image.image.clone()) + .collect(); + let owned: HashSet = if ask.is_empty() { + HashSet::new() + } else { + oci_dao::images_held(&state.pool, &owners, &ask) + .await + .map_err(db_error)? + .into_iter() + .collect() + }; + for image in candidates { + let owners_only = members.owners_only(&image.image, owned.contains(&image.image)); + if members.serves(position, &image.image, owners_only) { + merged.insert(image.image.clone(), image); + } } - let owners_only = members.owners_only(&image.image, owned.contains(&image.image)); - if members.serves(position, &image.image, owners_only) { - merged.insert(image.image.clone(), image); + if !full { + break; } - }) - .await - .map_err(db_error)?; + } } let total = merged.len(); let images = merged.into_values().skip(query.skip).take(take).collect(); @@ -377,23 +411,27 @@ async fn group_image_page( } /// Call `each` with every image of repository `repository_id` matching -/// `q`, read [`IMAGE_BATCH_SIZE`] at a time. +/// `q`, read [`IMAGE_BATCH_SIZE`] at a time by name. async fn for_each_image( state: &AppState, repository_id: i64, q: &str, mut each: impl FnMut(oci_dao::ImageSummary), ) -> sqlx::Result<()> { - let mut offset = 0; + let mut after = String::new(); loop { let images = - oci_dao::list_images(&state.pool, repository_id, q, offset, IMAGE_BATCH_SIZE).await?; - let count = images.len() as i64; + oci_dao::list_images_after(&state.pool, repository_id, q, &after, IMAGE_BATCH_SIZE) + .await?; + let full = images.len() as i64 == IMAGE_BATCH_SIZE; + let Some(last) = images.last() else { + return Ok(()); + }; + after = last.image.clone(); images.into_iter().for_each(&mut each); - if count < IMAGE_BATCH_SIZE { + if !full { return Ok(()); } - offset += count; } } diff --git a/crates/server/tests/registry.rs b/crates/server/tests/registry.rs index 6e3c2d06..f5658247 100644 --- a/crates/server/tests/registry.rs +++ b/crates/server/tests/registry.rs @@ -1696,6 +1696,138 @@ async fn browse_images_selector_counts_and_pages_after_filtering() { } } +/// Copy the manifest of image `template` of repository `from` into +/// repository `to` as the images `team/app-` for `range`, without +/// registry uploads. +async fn seed_images( + srv: &TestServer, + from: i64, + template: &str, + to: i64, + range: std::ops::Range, +) { + sqlx::query( + r#" + INSERT INTO oci_manifest + (repository_id, image, digest, blob_sha256, media_type, size, summary) + SELECT $3, 'team/app-' || lpad(n::text, 5, '0'), + digest, blob_sha256, media_type, size, summary + FROM oci_manifest CROSS JOIN generate_series($4::bigint, $5::bigint - 1) AS n + WHERE repository_id = $1 AND image = $2 + ON CONFLICT DO NOTHING + "#, + ) + .bind(from) + .bind(template) + .bind(to) + .bind(range.start) + .bind(range.end) + .execute(&srv.pool) + .await + .unwrap(); +} + +/// A group's image list over a large proxy (#596): it reads each member's +/// images a batch at a time by name (what that costs is +/// `forge-db`'s `oci_images` test), lists them all, searches them, and +/// still keeps an image the hosted member owns off the proxy's list for a +/// caller who may not read that member, asking only of the proxy's images +/// whether the owner holds them. +#[tokio::test] +async fn a_group_lists_a_large_proxys_images_in_batches() { + let srv = common::start().await; + let (hosted, proxy, group) = (repo_name("own"), repo_name("big"), repo_name("grp")); + let hosted_id = create_repo( + &srv, + json!({"name": hosted, "kind": "hosted", "format": "docker"}), + ) + .await["id"] + .as_i64() + .unwrap(); + let proxy_id = create_repo( + &srv, + json!({"name": proxy, "kind": "proxy", "format": "docker", "remote_url": "http://127.0.0.1:9"}), + ) + .await["id"] + .as_i64() + .unwrap(); + create_repo( + &srv, + json!({"name": group, "kind": "group", "format": "docker"}), + ) + .await; + for (member, priority) in [(&hosted, 10), (&proxy, 5)] { + let resp = admin_post( + &srv, + &format!("/admin/repositories/{group}/members"), + json!({"member": member, "priority": priority}), + ) + .await; + assert_eq!(resp.status(), 201); + } + admin_oci(&srv) + .await + .push_image(&format!("{hosted}/team/app-02500"), "1", "amd64", b"owned") + .await; + // More than a batch of images, the owned one among them. + seed_images(&srv, hosted_id, "team/app-02500", proxy_id, 0..4_500).await; + let (user, token, uid) = user_with_token(&srv).await; + grant(&srv, uid, &group, "read").await; + grant(&srv, uid, &proxy, "read").await; + + let list = |query: String| { + let (srv, user, token, group) = (&srv, &user, &token, &group); + async move { + let resp = srv + .client() + .get(format!( + "{}/browse/images?repo={group}&{query}", + srv.base_url + )) + .basic_auth(user, Some(token)) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200); + let body: Value = resp.json().await.unwrap(); + let images: Vec = body["items"] + .as_array() + .unwrap() + .iter() + .map(|i| i["image"].as_str().unwrap().to_string()) + .collect(); + (body["total"].clone(), images) + } + }; + let (total, images) = list("take=2".into()).await; + assert_eq!(total, 4_499, "all but the owned image"); + assert_eq!(images, ["team/app-00000", "team/app-00001"]); + let (total, images) = list("skip=2497&take=4".into()).await; + assert_eq!(total, 4_499); + assert_eq!( + images, + [ + "team/app-02497", + "team/app-02498", + "team/app-02499", + "team/app-02501" + ] + ); + let (total, images) = list("q=app-0449&take=20".into()).await; + assert_eq!(total, 10); + assert_eq!(images.last().unwrap(), "team/app-04499"); + let (total, _) = list("q=app-0250&take=20".into()).await; + assert_eq!(total, 9, "the owned image stays off the search too"); + + // Who reads the owner lists its image, from the owner. + grant(&srv, uid, &hosted, "read").await; + let (total, images) = list("q=app-02500".into()).await; + assert_eq!( + (total, images), + (json!(1), vec!["team/app-02500".to_string()]) + ); +} + /// Helm charts pushed as artifacts: the image page lists them by their /// config type, and `/browse/chart` reads their metadata, README and values /// from the blobs Forge holds, through a group too. diff --git a/docs/changes/2.2.0/596.fixed.md b/docs/changes/2.2.0/596.fixed.md new file mode 100644 index 00000000..219d6171 --- /dev/null +++ b/docs/changes/2.2.0/596.fixed.md @@ -0,0 +1,9 @@ +The image list of a docker group in the web UI (`GET /browse/images` of a +group, run again on every keystroke of a search) read each member's images +in `OFFSET` pages of 1,000, each grouping every image before it again, so +its database work grew with the square of a large proxy's images, and it +also read every image of the group's owning hosted members. Members are now +read a batch at a time by name, each batch costing only its own images, +and whether an owner holds an image is asked only of the images a member +that is no owner would list. A hosted or proxy repository listed to a +caller with a content selector is read the same way.