From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id EAF8E1FF15C for ; Fri, 3 Oct 2025 12:51:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A7E0E1921; Fri, 3 Oct 2025 12:51:50 +0200 (CEST) Message-ID: Date: Fri, 3 Oct 2025 12:51:47 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20251003085045.1346864-1-d.csapak@proxmox.com> <20251003085045.1346864-7-d.csapak@proxmox.com> <5304f093-fb23-42ad-ab0c-bca839572d73@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <5304f093-fb23-42ad-ab0c-bca839572d73@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759488683219 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.026 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 5/6] api: admin: datastore: optimize `groups` api call X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On 10/3/25 12:18 PM, Thomas Lamprecht wrote: > Am 03.10.25 um 10:50 schrieb Dominik Csapak: >> Currently we always touch all files for each snapshot in a group when >> listing them, even though we don't need all that info. >> >> We're only interested in getting either the last finished snapshot >> information, or the last unfinished one (which must the only one in >> normal use, we can't have multiple unfinished snapshots usually) >> >> Instead of getting all the information upgfront, use the snapshot > > s/upgfront/upfront/ > >> iterator of the group to get only the id, sort them by time, and >> use the first we're interested in, getting the snapshot specific info >> only for those we want to check. >> >> In my (admittedly extreme) setup with ~600 groups with ~1000 snapshots >> each, this changes the time this api call needs from ~40s to <1s. >> (on a relatively fast disk). > > nice! In the UI we mainly (only?) uses this for gathering the notes of > a group, FWICT? > yes currently we only use it for the notes, but it's also exposed on the pbs client AFAIR, there we show the data ofc note that the improvements are only relevant for groups with many snapshots. >> >> While at it, lift the restriction of only returning groups with >> snapshots in them, now returning also empty ones. >> >> To keep api compatibility, use a timestamp of 0 for those. >> (no valid backup could have been made at that time anyway) > > lazy question: Is this dependent on the previous patches? From a quick > check, it looks like it might be rather independent and one could apply > this one already. > the way i ordered the patches the previous refactorings here in the api (3/6, 4/6) are necessary. If you want i can reverse them though (first the improvement, second the refactoring) >> Signed-off-by: Dominik Csapak >> --- >> src/api2/admin/datastore.rs | 45 +++++++++++++++++++++++-------------- >> 1 file changed, 28 insertions(+), 17 deletions(-) >> >> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs >> index 0b133d166..2252dcfa4 100644 >> --- a/src/api2/admin/datastore.rs >> +++ b/src/api2/admin/datastore.rs >> @@ -166,36 +166,47 @@ fn backup_group_to_group_list_item( >> return None; >> } >> >> - let snapshots = match group.list_backups() { >> - Ok(snapshots) => snapshots, >> + let mut snapshots: Vec<_> = match group.iter_snapshots() { >> + Ok(snapshots) => snapshots.collect::, Error>>().ok()?, >> Err(_) => return None, >> }; >> >> let backup_count: u64 = snapshots.len() as u64; >> - if backup_count == 0 { >> - return None; >> - } >> + let last = if backup_count == 1 { >> + // we may have only one unfinished snapshot >> + snapshots.pop().and_then(|dir| BackupInfo::new(dir).ok()) >> + } else { >> + // we either have no snapshots, or at least one finished one, since we cannot have >> + // multiple unfinished ones >> + snapshots.sort_by_key(|b| std::cmp::Reverse(b.backup_time())); >> + snapshots >> + .iter() >> + .filter_map(|backup| match BackupInfo::new(backup.clone()) { >> + Ok(info) => { >> + if info.is_finished() { >> + Some(info) >> + } else { >> + None >> + } >> + } >> + Err(_) => None, > > nit: Could be written as a bit more concise way: > > snapshots > .iter() > .filter_map(|backup| BackupInfo::new(backup.clone()).ok()) > .filter(|info| info.is_finished()) > .next() > > With splitting the conversion to BackupInfo and filtering for finished > backups into two makes it a bit easier (or rather quicker) to follow to me. fine with me, I didn't want to have multiple filter/map calls but yeah it makes it easier to read > >> + }) >> + .next() >> + }; >> >> - let last_backup = snapshots >> - .iter() >> - .fold(&snapshots[0], |a, b| { >> - if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() { >> - a >> - } else { >> - b >> - } >> - }) >> - .to_owned(); >> + let (last_backup, files) = last >> + .map(|info| (info.backup_dir.backup_time(), info.files)) >> + .unwrap_or((0, Vec::new())); >> >> let notes_path = datastore.group_notes_path(ns, group.as_ref()); >> let comment = file_read_firstline(notes_path).ok(); >> >> let item = GroupListItem { >> backup: group.into(), >> - last_backup: last_backup.backup_dir.backup_time(), >> + last_backup, >> owner: Some(owner), >> backup_count, >> - files: last_backup.files, >> + files, >> comment, >> }; >> > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel