diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index d4f6384d9b..f846e145c5 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -294,11 +294,11 @@ pub struct Tenant { /// During timeline creation, we first insert the TimelineId to the /// creating map, then `timelines`, then remove it from the creating map. - /// **Lock order**: if acquiring both, acquire`timelines` before `timelines_creating` + /// **Lock order**: if acquiring all (or a subset), acquire them in order `timelines`, `timelines_offloaded`, `timelines_creating` timelines_creating: std::sync::Mutex>, /// Possibly offloaded and archived timelines - /// **Lock order**: if acquiring both, acquire`timelines` before `timelines_offloaded` + /// **Lock order**: if acquiring all (or a subset), acquire them in order `timelines`, `timelines_offloaded`, `timelines_creating` timelines_offloaded: Mutex>>, // This mutex prevents creation of new timelines during GC. @@ -584,13 +584,19 @@ impl OffloadedTimeline { } } +impl fmt::Debug for OffloadedTimeline { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "OffloadedTimeline<{}>", self.timeline_id) + } +} + #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] pub enum MaybeOffloaded { Yes, No, } -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum TimelineOrOffloaded { Timeline(Arc), Offloaded(Arc), @@ -1815,6 +1821,8 @@ impl Tenant { } /// Loads the specified (offloaded) timeline from S3 and attaches it as a loaded timeline + /// + /// Counterpart to [`offload_timeline`]. async fn unoffload_timeline( self: &Arc, timeline_id: TimelineId, @@ -1823,6 +1831,24 @@ impl Tenant { ) -> Result, TimelineArchivalError> { info!("unoffloading timeline"); let cancel = self.cancel.clone(); + + // Protect against concurrent attempts to use this TimelineId + // We don't care much about idempotency, as it's ensured a layer above. + let allow_offloaded = true; + let _create_guard = self + .create_timeline_create_guard( + timeline_id, + CreateTimelineIdempotency::FailWithConflict, + allow_offloaded, + ) + .map_err(|err| match err { + TimelineExclusionError::AlreadyCreating => TimelineArchivalError::AlreadyInProgress, + TimelineExclusionError::AlreadyExists { .. } => { + TimelineArchivalError::Other(anyhow::anyhow!("Timeline already exists")) + } + TimelineExclusionError::Other(e) => TimelineArchivalError::Other(e), + })?; + let timeline_preload = self .load_timeline_metadata(timeline_id, self.remote_storage.clone(), cancel.clone()) .await; @@ -4129,7 +4155,8 @@ impl Tenant { new_timeline_id: TimelineId, idempotency: CreateTimelineIdempotency, ) -> Result, CreateTimelineError> { - match self.create_timeline_create_guard(new_timeline_id, idempotency) { + let allow_offloaded = false; + match self.create_timeline_create_guard(new_timeline_id, idempotency, allow_offloaded) { Ok(create_guard) => { pausable_failpoint!("timeline-creation-after-uninit"); Ok(StartCreatingTimelineResult::CreateGuard(create_guard)) @@ -4141,10 +4168,21 @@ impl Tenant { Err(CreateTimelineError::AlreadyCreating) } Err(TimelineExclusionError::Other(e)) => Err(CreateTimelineError::Other(e)), - Err(TimelineExclusionError::AlreadyExists { existing, arg }) => { + Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Offloaded(_existing), + .. + }) => { + info!("timeline already exists but is offloaded"); + Err(CreateTimelineError::Conflict) + } + Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Timeline(existing), + arg, + }) => { { let existing = &existing.create_idempotency; let _span = info_span!("idempotency_check", ?existing, ?arg).entered(); + debug!("timeline already exists"); match (existing, &arg) { // FailWithConflict => no idempotency check @@ -4467,17 +4505,26 @@ impl Tenant { /// Get a guard that provides exclusive access to the timeline directory, preventing /// concurrent attempts to create the same timeline. + /// + /// The `allow_offloaded` parameter controls whether to tolerate the existence of + /// offloaded timelines or not. fn create_timeline_create_guard( &self, timeline_id: TimelineId, idempotency: CreateTimelineIdempotency, + allow_offloaded: bool, ) -> Result { let tenant_shard_id = self.tenant_shard_id; let timeline_path = self.conf.timeline_path(&tenant_shard_id, &timeline_id); - let create_guard = - TimelineCreateGuard::new(self, timeline_id, timeline_path.clone(), idempotency)?; + let create_guard = TimelineCreateGuard::new( + self, + timeline_id, + timeline_path.clone(), + idempotency, + allow_offloaded, + )?; // At this stage, we have got exclusive access to in-memory state for this timeline ID // for creation. diff --git a/pageserver/src/tenant/timeline/uninit.rs b/pageserver/src/tenant/timeline/uninit.rs index 7d66c5aec8..c398289a5c 100644 --- a/pageserver/src/tenant/timeline/uninit.rs +++ b/pageserver/src/tenant/timeline/uninit.rs @@ -8,7 +8,7 @@ use utils::{fs_ext, id::TimelineId, lsn::Lsn}; use crate::{ context::RequestContext, import_datadir, - tenant::{CreateTimelineIdempotency, Tenant}, + tenant::{CreateTimelineIdempotency, Tenant, TimelineOrOffloaded}, }; use super::Timeline; @@ -177,7 +177,7 @@ pub(crate) struct TimelineCreateGuard<'t> { pub(crate) enum TimelineExclusionError { #[error("Already exists")] AlreadyExists { - existing: Arc, + existing: TimelineOrOffloaded, arg: CreateTimelineIdempotency, }, #[error("Already creating")] @@ -194,31 +194,41 @@ impl<'t> TimelineCreateGuard<'t> { timeline_id: TimelineId, timeline_path: Utf8PathBuf, idempotency: CreateTimelineIdempotency, + allow_offloaded: bool, ) -> Result { // Lock order: this is the only place we take both locks. During drop() we only // lock creating_timelines let timelines = owning_tenant.timelines.lock().unwrap(); + let timelines_offloaded = owning_tenant.timelines_offloaded.lock().unwrap(); let mut creating_timelines: std::sync::MutexGuard< '_, std::collections::HashSet, > = owning_tenant.timelines_creating.lock().unwrap(); if let Some(existing) = timelines.get(&timeline_id) { - Err(TimelineExclusionError::AlreadyExists { - existing: existing.clone(), + return Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Timeline(existing.clone()), arg: idempotency, - }) - } else if creating_timelines.contains(&timeline_id) { - Err(TimelineExclusionError::AlreadyCreating) - } else { - creating_timelines.insert(timeline_id); - Ok(Self { - owning_tenant, - timeline_id, - timeline_path, - idempotency, - }) + }); } + if !allow_offloaded { + if let Some(existing) = timelines_offloaded.get(&timeline_id) { + return Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Offloaded(existing.clone()), + arg: idempotency, + }); + } + } + if creating_timelines.contains(&timeline_id) { + return Err(TimelineExclusionError::AlreadyCreating); + } + creating_timelines.insert(timeline_id); + Ok(Self { + owning_tenant, + timeline_id, + timeline_path, + idempotency, + }) } }