pageserver: don't store tenant/timeline ID on each layer, synchronize with Timeline shutdown #5967

jcsp · 2023-11-28T22:25:23Z

Problem

Layers are implicitly scoped to a Timeline: it is wasteful to store the timeline+tenant ID on each one
Layers store their full local path, but only need it occasionally & can construct it on-demand via Timeline
Layer::drop does I/O to a Timeline's local path, but this can happen after the Timeline is shut down and destroyed.

Summary of changes

Checklist before requesting a review

I have performed a self-review of my code.
If it is a core feature, I have added thorough tests.
Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

Do not forget to reformat commit message to not include the above checklist

github-actions · 2023-11-28T22:33:49Z

2148 tests run: 2064 passed, 0 failed, 84 skipped (full report)

Flaky tests (4)

Postgres 16

test_pitr_gc: debug

Postgres 15

test_physical_replication: release

Postgres 14

test_statvfs_pressure_min_avail_bytes: debug
test_statvfs_pressure_usage: debug

Code coverage (full report)

functions: 54.8% (9370 of 17084 functions)
lines: 82.1% (54423 of 66250 lines)

_{The comment gets automatically updated with the latest test results
93e069c at 2023-12-11T18:50:38.500Z :recycle:}

We can build it on-demand via Timeline.

koivunej · 2023-12-07T09:31:13Z

pageserver/src/tenant/storage_layer/layer.rs

+    }
+
+    /// Use this instead of `local_path` if you already have a Timeline to hand.
+    pub(crate) fn build_local_path(


Who uses this from outside? Or why does it need to be exposed at all.

Used by:

Timeline, constructing paths for layer files in e.g. create_delta_layer

RemoteTimelineClient, constructing path to download to.

koivunej · 2023-12-07T09:33:30Z

pageserver/src/tenant/storage_layer/layer.rs

+        // We will only do I/O during drop if our Timeline's layer_gate is open: this avoids
+        // the risk that we would race with Timeline::shutdown and end up doing I/O to a timeline
+        // path for which the Timeline object has been torn down already.
+        let _gate_guard = match timeline.layer_gate.enter() {
+            Ok(g) => g,
+            Err(GateError::GateClosed) => return,
+        };


Now that I am looking at this, wouldn't it just be better to acquire a gate for all layers before creating them?

For the simplest reasoning about correctness, we could hold a gate around the lifetime of all layer objects.

However, that's relatively expensive (layers are very numerous) and isn't necessary: we already block timeline shutdown on all related tasks completing, so that should mean that layers aren't doing any I/O after shutdown complete. The only exception to that is here in drop(), where we need to check the gate in case some other code kept a Layer alive past the point that the other code kept its GateGuard (or task_mgr task lifetime).

koivunej · 2023-12-07T09:37:28Z

pageserver/src/tenant/storage_layer/layer.rs

+        let local_path = self
+            .local_path()
+            .map_err(|_| EvictionCancelled::TimelineGone)?;


But now we would be upgrading the timeline all over? It must already be upgraded to get to here because otherwise we would be running without a span.

It's not upgraded frequently (the paths that need the local path aren't called super often), but yeah, I agree that it's a bit dangerous to make this implicit, since callers might naively use it for logging or something.

I've refactored this to take Timeline as a parameter: as you say, callers generally have a Timeline to hand already at the point they call this - 0f53319

koivunej

Getting rid of ids in the persistentlayerdesc is a good change but I dislike the adhoc timeline upgrading which is now spread around layer.

jcsp · 2023-12-07T10:16:46Z

pageserver/src/tenant/storage_layer/layer.rs

@@ -1004,7 +1038,10 @@ impl LayerInner {
            //
            // FIXME: this is not true anymore, we can safely evict wanted deleted files.
        } else if can_evict && evict {
-            let span = tracing::info_span!(parent: None, "layer_evict", tenant_id = %self.desc.tenant_shard_id.tenant_id, shard_id = %self.desc.tenant_shard_id.shard_slug(), timeline_id = %self.desc.timeline_id, layer=%self, %version);
+            // If timeline is alive, we can construct a span with IDs for this function.
+            let span = self.timeline.upgrade().map(|timeline| {


@koivunej at this point, we could instead return if the timeline is gone. I didn't make that change because it wasn't obvious if that would violate any other expectations, but it's probably fine since once Timeline is destroyed we don't have any obligation to make progress with eviction -- your thoughts?

arpad-m

I think we should

pageserver/src/tenant/timeline/layer_manager.rs

pageserver/src/tenant/storage_layer/layer.rs

jcsp · 2024-04-05T10:40:37Z

#7082 fixed the important part of this (deletions happening after timeline shutdown)

jcsp added c/storage/pageserver Component: storage: pageserver a/tech_debt Area: related to tech debt labels Nov 28, 2023

jcsp force-pushed the jcsp/layer-tenant-id branch from 96371ee to 6b60f7c Compare November 29, 2023 10:49

jcsp mentioned this pull request Nov 29, 2023

pageserver: refactor TenantId to TenantShardId in Tenant & Timeline #5957

Merged

jcsp force-pushed the jcsp/layer-tenant-id branch 2 times, most recently from 9f36f14 to 8b2f996 Compare December 5, 2023 17:21

jcsp mentioned this pull request Dec 5, 2023

fix(layer): metric splitting, span rename #5902

Merged

jcsp added 3 commits December 5, 2023 17:41

pageserver: don't store tenant/timeline ID on layers

d07bc7b

pageserver: avoid storing path in layer

2a8197b

We can build it on-demand via Timeline.

pageserver: make Timeline::shutdown safe against Layer::drop

41401ea

jcsp force-pushed the jcsp/layer-tenant-id branch from 8b2f996 to 41401ea Compare December 5, 2023 17:41

koivunej reviewed Dec 7, 2023

View reviewed changes

koivunej requested changes Dec 7, 2023

View reviewed changes

jcsp added 2 commits December 7, 2023 09:49

Merge remote-tracking branch 'upstream/main' into jcsp/layer-tenant-id

5b18144

Avoid implict Weak ugprade in local_path

0f53319

jcsp commented Dec 7, 2023

View reviewed changes

fix doc link

90e27f1

jcsp marked this pull request as ready for review December 11, 2023 09:58

jcsp requested a review from a team as a code owner December 11, 2023 09:58

jcsp requested review from arpad-m and removed request for a team December 11, 2023 09:58

arpad-m requested changes Dec 11, 2023

View reviewed changes

pageserver/src/tenant/timeline/layer_manager.rs Outdated Show resolved Hide resolved

pageserver/src/tenant/storage_layer/layer.rs Outdated Show resolved Hide resolved

refactors

93e069c

arpad-m approved these changes Dec 11, 2023

View reviewed changes

jcsp mentioned this pull request Dec 20, 2023

pageserver: implement secondary-mode downloads #6123

Merged

7 tasks

jcsp closed this Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pageserver: don't store tenant/timeline ID on each layer, synchronize with Timeline shutdown #5967

pageserver: don't store tenant/timeline ID on each layer, synchronize with Timeline shutdown #5967

jcsp commented Nov 28, 2023

github-actions bot commented Nov 28, 2023 •

edited

Loading

Postgres 16

Postgres 15

Postgres 14

koivunej Dec 7, 2023 •

edited

Loading

jcsp Dec 7, 2023

koivunej Dec 7, 2023

jcsp Dec 7, 2023

koivunej Dec 7, 2023

jcsp Dec 7, 2023

koivunej left a comment

jcsp Dec 7, 2023

arpad-m left a comment

jcsp commented Apr 5, 2024

pageserver: don't store tenant/timeline ID on each layer, synchronize with Timeline shutdown #5967

pageserver: don't store tenant/timeline ID on each layer, synchronize with Timeline shutdown #5967

Conversation

jcsp commented Nov 28, 2023

Problem

Summary of changes

Checklist before requesting a review

Checklist before merging

github-actions bot commented Nov 28, 2023 • edited Loading

2148 tests run: 2064 passed, 0 failed, 84 skipped (full report)

Postgres 16

Postgres 15

Postgres 14

Code coverage (full report)

koivunej Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

jcsp Dec 7, 2023

Choose a reason for hiding this comment

koivunej Dec 7, 2023

Choose a reason for hiding this comment

jcsp Dec 7, 2023

Choose a reason for hiding this comment

koivunej Dec 7, 2023

Choose a reason for hiding this comment

jcsp Dec 7, 2023

Choose a reason for hiding this comment

koivunej left a comment

Choose a reason for hiding this comment

jcsp Dec 7, 2023

Choose a reason for hiding this comment

arpad-m left a comment

Choose a reason for hiding this comment

jcsp commented Apr 5, 2024

github-actions bot commented Nov 28, 2023 •

edited

Loading

koivunej Dec 7, 2023 •

edited

Loading