feat(metastore): Add TableOfContentsWriter.ReplaceIndexPointers method by trevorwhitney · Pull Request #21932 · grafana/loki

trevorwhitney · 2026-05-15T16:02:28Z

What

Introduces TableOfContentsWriter.ReplaceIndexPointers(ctx, window, tenant, oldPaths, newEntries) (swapped bool, err error) — an atomic, idempotent ToC mutation primitive that swaps a set of index pointers for one tenant within a metastore window while preserving every other tenant's section unchanged.

Why

The existing metastore code only supports append-style writes (WriteEntry / AppendIndexPointer); the compactor's IndexConsolidate task needs to replace a tenant's source-index pointers with a single covering-index pointer in one conditional PUT. ReplaceIndexPointers is the cycle's terminal mutation. This PR ships the primitive alone with no callers — default Loki behavior is unchanged.

In the future we may want to consider an append-only pattern for the ToCs, allowing appends that would negate previous entries, but that is a thought experiment for another time.

How

Mirrors the existing WriteEntry pattern in pkg/dataobj/metastore/toc_writer.go: bounded backoff retry loop wrapping bucket.GetAndReplace (which gives us the S3 If-Match/If-None-Match conditional-PUT contract via the sizedGetAndReplaceBucket wrapper in pkg/storage/bucket/client.go). The callback:

Drains the existing ToC bytes up-front (never returns a drained ReadCloser).
Idempotency / race-loss check via scanForMatches: if ANY oldPath is still present in the target tenant's section, proceed; if none match, abort the conditional PUT via an internal sentinel error (errReplaceNoOp) and return (false, nil). The sentinel intentionally aborts the write so we never materialize an empty blob on the missing-ToC path.
Rebuilds the ToC into a fresh indexobj.Builder via replayFiltered, preserving every other tenant's rows verbatim and dropping the target tenant's oldPaths, then appending newEntries for the target tenant.
Full-drain handling: if the rebuild produces an empty builder (single-tenant ToC, all rows in oldPaths, no newEntries), the same sentinel is returned with swapped=true so the operation reports success without writing an empty ToC.
The successful-rebuild path returns objstore.NopCloserWithSize(bytes.NewReader(newBytes)) — the size-bearing reader required by the spec's Sharp-edge to preserve (otherwise If-Match is silently dropped by minio-go on S3 multipart uploads).

A deterministic fast-path returns (false, nil) when len(oldPaths) == 0 without touching object storage, so the no-op contract holds even against a degraded bucket.

Retry budget: MinBackoff 50ms, MaxBackoff 10s, MaxRetries 10. An internal replaceIndexPointers(..., backoffCfg) helper accepts an explicit backoff config — used only by the retry-exhaustion test (1ms→5ms, MaxRetries=3) to keep wall time <100ms.

Semantics summary

(true, nil) — swap applied.
(false, nil) — idempotent no-op: oldPaths empty, all oldPaths already gone (race-loss), or full-drain success.
(false, err) — transient error after retry exhaustion.

The race-loss check uses ANY-match (per the spec sketch): if ANY oldPath is still present we proceed and drop the matched subset. The godoc explicitly documents this and notes that cross-cycle partial overlaps produce bounded duplicate entries that the next cycle's plan will re-merge.

benclive · 2026-05-15T17:24:36Z

+			if err != nil {
+				return false, errors.Wrap(err, "reading rows")
+			}
+			if n == 0 {


I don't think this should trigger a break. AFAIR n == 0 is a valid return and you need to wait for an explicitly EOF

yup, good catch, though I'm not sure in reality how this would happen with the ToCs? isn't the usual case where this could happen when all rows are filtered out via a predicate, which we'll never do here? either way, you're right, and I've removed them.

benclive · 2026-05-15T17:25:38Z

+			if err != nil {
+				return errors.Wrap(err, "reading rows")
+			}
+			if n == 0 {


benclive · 2026-05-15T17:27:58Z

+
+	// Three tenants, each with three rows at well-separated time ranges.
+	seedRows := []tocRow{
+		{"tenantA", "idx/a-0", 10, 20},


This test would be better if multiple tenants pointed at the same index paths

good call, I refactored to have both, as compaction should work both from multi-tenant idx files -> single tenant and from N single-tenant files -> 1

benclive · 2026-05-15T17:30:54Z

+// present in the target tenant's current section, the swap proceeds and
+// drops the matched subset (leaving non-matched oldPaths' replacements, if
+// they were already swapped in by a concurrent coordinator, untouched).
+// Only when ZERO oldPaths match is the call treated as a no-op. Callers


I don't understand the semantics here. Does compaction require some old paths to remove? Is it intended to be a 1:1 swap of old for new, or can the sizes of each set differ?

it's talking about index paths, we've compacted N indexes -> 1 (or N, actually, for the L0 -> L1 multi-tenant -> single tenant compaction), and is operating under the assumption (which will be true once we start compacting log objects), that we will likely need to run multiple compactions before we converge to a single index file. The "ANY oldPath" is recovery/re-entry logic, in case there was a partial update (ie. some workers failed, but other's succeeded)

… happy path

…ants

…ases

…tract

…evive lint)

…ReplaceIndexPointers Addresses holistic /code-review findings: - empty oldPaths must no-op deterministically without storage I/O (was: full GET round-trip subject to retries on degraded buckets) - partial-match (ANY-match) semantic documented explicitly in godoc - regression test asserts 0 GetAndReplace calls on empty oldPaths even with a failing bucket - new delete-only test covers successful swap with empty newEntries

…ters When a single-tenant ToC has every row listed in oldPaths and newEntries is empty, the rebuild path appended zero rows and indexobj.Builder.Flush returned ErrBuilderEmpty, which the retry loop treated as a transient error. The operation logically succeeded — the caller's view of the source paths is gone — so the primitive now routes the empty- builder case to the existing errReplaceNoOp sentinel path with swapped=true, leaving the existing ToC bytes in place. The outer sentinel handler now returns swapped instead of hardcoding false so the success signal propagates.

dataset.RowReader documents (n==0, err==nil) as a valid "no progress this call" return — e.g. when a predicate filters every row in a batch — and callers must keep reading until io.EOF (see pkg/dataobj/internal/dataset/row_reader.go:114-116 and row_reader_basic.go:199-200, both citing the io.Reader convention). scanForMatches and replayFiltered were breaking on n==0 as a safety net, which would silently truncate iteration if a future predicate ever made (0, nil) reachable on the indexpointers.RowReader path. Also switches err == io.EOF to errors.Is for wrapped-EOF tolerance and uses Go 1.22+ integer range over the batch buffer.

Adds a second sub-test that seeds a ToC where multiple tenants' sections reference the same idx/... paths \u2014 the multi-tenant L0 shape \u2014 and asserts the swap removes those paths from the target tenant ONLY, leaving every other tenant's references to the shared L0 indexes intact. This exercises the `sectionTenant == tenant` guard in replayFiltered; without it the prior single-case test could not have caught a regression that dropped rows across all tenants when paths collided.

ivkalita · 2026-05-18T16:12:11Z

+	// Deterministic fast-path: with no oldPaths to remove, the call would always
+	// converge to a sentinel-aborted no-op inside the callback. Returning here
+	// makes the no-op behavior independent of bucket availability so callers can
+	// rely on (false, nil) without error-handling for transient storage issues.


(nit) IMHO removing this comment makes things more straightforward 😅

benclive · 2026-05-19T09:18:50Z

+
+			newObj, closer, ferr := builder.Flush()
+			if ferr != nil {
+				// Full drain: every row in every section was filtered out and


Is this case valid? If it removed all the old paths and didn't add any new ones, shouldn't it be written anyway in order to drop the old paths?

And what does "swapped" indicate? Because it aborts, nothing is actually swapped as far as I can tell.

it removed all the old paths and didn't add any new ones

I believe we should detect it way earlier, before the flushing

UPD: I think I misunderstood Ben's comment first. My chain of thoughts was that we should always have at least 1 new path, so maybe it's worth to detect when len(oldPaths) == 0 || len(newPaths) == 0 way earlier and fail there

ivkalita · 2026-05-20T12:28:15Z

+				var rerr error
+				existingBytes, rerr = io.ReadAll(existing)
+				if rerr != nil {
+					return nil, errors.Wrap(rerr, "reading existing ToC")


(nit) fmt.Errorf should be enough according to https://github.com/grafana/loki/blob/main/CODING_STANDARDS.md#error-handling

ivkalita · 2026-05-20T12:46:57Z

+	window time.Time,
+	tenant string,
+	oldPaths []string,
+	newEntries []TableOfContentsEntry,


what about len(newEntries) == 0 case?

ivkalita · 2026-05-20T12:47:49Z

+
+			newObj, closer, ferr := builder.Flush()
+			if ferr != nil {
+				// Full drain: every row in every section was filtered out and


it removed all the old paths and didn't add any new ones

I believe we should detect it way earlier, before the flushing

UPD: I think I misunderstood Ben's comment first. My chain of thoughts was that we should always have at least 1 new path, so maybe it's worth to detect when len(oldPaths) == 0 || len(newPaths) == 0 way earlier and fail there

ivkalita · 2026-05-20T12:50:44Z

+			newObj, closer, ferr := builder.Flush()
+			if ferr != nil {
+				// Full drain: every row in every section was filtered out and
+				// no newEntries were appended, so the rebuilt ToC would be
+				// empty. Treat this as a successful swap and abort the
+				// conditional PUT — leaving the existing ToC in place is
+				// acceptable here because the original entries are gone in
+				// the caller's view and a future cycle will replace the ToC
+				// when fresh content arrives. A full-drain ToC delete would
+				// be a separate primitive.
+				if stderrors.Is(ferr, indexobj.ErrBuilderEmpty) {
+					swapped = true
+					return nil, errReplaceNoOp
+				}
+				return nil, errors.Wrap(ferr, "flushing rebuilt ToC")
+			}
+			reader, rerr := newObj.Reader(ctx)
+			if rerr != nil {
+				_ = closer.Close()
+				return nil, errors.Wrap(rerr, "opening rebuilt ToC reader")
+			}
+			newBytes, rerr := io.ReadAll(reader)
+			closeErr := stderrors.Join(reader.Close(), closer.Close())
+			if rerr != nil {
+				return nil, errors.Wrap(rerr, "reading rebuilt ToC")
+			}
+			if closeErr != nil {
+				return nil, errors.Wrap(closeErr, "closing rebuilt ToC")
+			}
+			swapped = true
+			return objstore.NopCloserWithSize(bytes.NewReader(newBytes)), nil


we flush dataobj to newObj

then we create a reader newObj.Reader(ctx)

then we read all from this reader and close it

finally we create a new reader from the read bytes

is it necessary to create a new reader? Are there benefits comparing to the approach in toc_writer.go where we return a wrapped newObj.Reader(ctx) reader?

ivkalita

Left a bunch of comments but overall looks good to me 👍

trevorwhitney · 2026-05-20T17:47:22Z

I merged so I could keep moving with my local branches which are ahead of this and getting bit complicated to manage. Feedback is addressed in a followup: #21971

trevorwhitney requested a review from a team as a code owner May 15, 2026 16:02

pull-request-size Bot added the size/XL label May 15, 2026

trevorwhitney changed the title ~~feat(metastore): Add TableOfContentsWriter.ReplaceIndexPointers primitive (A7)~~ May 15, 2026

benclive reviewed May 15, 2026

View reviewed changes

trevorwhitney added 10 commits May 15, 2026 13:41

feat(metastore): Scaffold TableOfContentsWriter.ReplaceIndexPointers

7ebdfe5

feat(metastore): Implement TableOfContentsWriter.ReplaceIndexPointers…

a50dd45

… happy path

test(metastore): Assert ReplaceIndexPointers preserves non-target ten…

1e89f73

…ants

test(metastore): Cover ReplaceIndexPointers race-loss & idempotency c…

2d0fd1e

…ases

test(metastore): Cover ReplaceIndexPointers conditional-PUT retry con…

f7f25e2

…tract

fix(metastore): Move ctx to first parameter in readToC test helper (r…

445faf6

…evive lint)

Merge branch 'main' into dataobj-compactor/replace-index-pointers

9a4d336

trevorwhitney force-pushed the dataobj-compactor/replace-index-pointers branch from cfef6a2 to b8b9ba6 Compare May 15, 2026 19:42

ivkalita reviewed May 18, 2026

View reviewed changes

Merge branch 'main' into dataobj-compactor/replace-index-pointers

5b56f2e

benclive reviewed May 19, 2026

View reviewed changes

ivkalita reviewed May 20, 2026

View reviewed changes

ivkalita approved these changes May 20, 2026

View reviewed changes

trevorwhitney merged commit 10fc249 into main May 20, 2026
96 checks passed

trevorwhitney deleted the dataobj-compactor/replace-index-pointers branch May 20, 2026 16:51

trevorwhitney mentioned this pull request May 20, 2026

refactor(metastore): Address PR feedback on ReplaceIndexPointers #21971

Merged

This was referenced May 20, 2026

feat(dataobj-compactor): IndexMerge executor over postings + stats sections #21972

Closed

feat(dataobj-compactor): Compactor coordinator/planner main loop #21973

Merged

This was referenced May 25, 2026

chore(k308): release 3.8.0 #22007

Closed

chore(k309): release 3.8.0 #22136

Closed

loki-gh-app Bot mentioned this pull request Jun 8, 2026

chore(k310): Release 3.8.0 #22294

Merged

This was referenced Jun 8, 2026

chore(k310): Release 3.8.0 (backport main) #22303

Merged

chore(k310): Release 3.9.0 #22317

Closed

chore(k311): Release 3.9.0 #22415

Open

loki-gh-app Bot mentioned this pull request Jun 22, 2026

chore(k312): Release 3.9.0 #22492

Open

loki-gh-app Bot mentioned this pull request Jun 29, 2026

chore(k313): Release 3.9.0 #22761

Open

loki-gh-app Bot mentioned this pull request Jul 13, 2026

chore(k315): Release 3.9.0 #23246

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(metastore): Add TableOfContentsWriter.ReplaceIndexPointers method - #21932

feat(metastore): Add TableOfContentsWriter.ReplaceIndexPointers method#21932
trevorwhitney merged 12 commits into
mainfrom
dataobj-compactor/replace-index-pointers

trevorwhitney commented May 15, 2026 •

edited

Loading

benclive May 15, 2026 •

edited

Loading

trevorwhitney May 15, 2026

benclive May 15, 2026

trevorwhitney May 15, 2026

benclive May 15, 2026

trevorwhitney May 15, 2026 •

edited

Loading

benclive May 15, 2026

trevorwhitney May 15, 2026 •

edited

Loading

ivkalita May 18, 2026

benclive May 19, 2026

benclive May 19, 2026

ivkalita May 20, 2026 •

edited

Loading

ivkalita May 20, 2026

ivkalita May 20, 2026

ivkalita May 20, 2026 •

edited

Loading

ivkalita May 20, 2026

ivkalita left a comment

Uh oh!

trevorwhitney commented May 20, 2026

Labels

3 participants

Uh oh!

Conversation

trevorwhitney commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

benclive May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevorwhitney May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevorwhitney May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivkalita May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivkalita May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivkalita left a comment

Choose a reason for hiding this comment

Uh oh!

trevorwhitney commented May 20, 2026

Labels

3 participants

trevorwhitney commented May 15, 2026 •

edited

Loading

benclive May 15, 2026 •

edited

Loading

trevorwhitney May 15, 2026 •

edited

Loading

trevorwhitney May 15, 2026 •

edited

Loading

ivkalita May 20, 2026 •

edited

Loading

ivkalita May 20, 2026 •

edited

Loading