feat: cache generation and sequence to reduce TSM filename parsing by davidby-influx · Pull Request #26798 · influxdata/influxdb

davidby-influx · 2025-09-13T01:12:31Z

…rations

tsdb/engine/tsm1/file_store.go

gwossum · 2025-09-15T20:49:44Z

tsdb/engine/tsm1/reader.go

+	if nil != t.parseFileNameFunc {
+		// If parseFileNameFunc is nil, we are in a test or another TSMReader use
+		// that does not involve compaction planning!
+		t.generation, t.sequence, err = t.parseFileNameFunc(t.Path())
+		if err != nil {
+			return nil, err
+		}
+	}


I have concerns about allowing t.generation and t.sequence to be 0 if WithParseFileNameFunc is not used. How disruptive would it be to require supplying the parsing function? Alternatively, does this function really need to be parameterized? I remember discussion that we never use anything but the default parsing function.

This is a tough one. We have lots of tests that use non-formatted files names (created by Go standard packages) where we don't care about sequence and generation. When I enforced name parsing, all those tests broke, and I couldn't think of a better way to allow unexamined filenames while still caching the sequence and generation efficiently.

gwossum · 2025-09-15T20:50:43Z

tsdb/engine/tsm1/reader.go

+	t.generation, t.sequence, err = t.parseFileNameFunc(path)
+	if err != nil {
+		return fmt.Errorf("failed parsing filename %q for generation and sequence numbers: %w", path, err)
+	}


No check if t.parseFilenameFunc is nil. This isn't an issue if we force supplying the parsing function or stop parameterizing it because we always use the default.

I'll add a check (see above why we want to allow it to be nil for test compatibility)

gwossum · 2025-09-15T20:52:34Z

tsdb/engine/tsm1/compact.go

 		}

-		group := generations[gen]
+		group := generations[f.Generation]


Should we check that f.Generation and f.Sequence are not 0 before using them? This isn't an issue if we force supplying a parsing function or stop parameterizing it and always use the default.

I believe we have to parameterize it to preserve tests which use unformatted TSM file names (from various mktemp sort of calls).

gwossum · 2025-09-15T20:55:09Z

tsdb/engine/tsm1/reader.go

 		return nil, err
 	}

+	if nil != t.parseFileNameFunc {


Alternatively, we could fallback to using the default parsing function if WithParseFileNameFunc is not used.

That would break tests.

That breaks tests. The whole parsing function pointer could now be a boolean flag (parse/no_parse) and the function hard-coded, if that seems cleaner

No_parse for some tests, parse by default

devanbenz

LGTM, please wait for Geoffrey's re-review.

gwossum · 2025-09-17T19:17:16Z

tsdb/engine/tsm1/file_store.go

 		parseFileName: DefaultParseFileName,
 		copyFiles:     runtime.GOOS == "windows",
-		readerOptions: options,
+		readerOptions: append(options, WithParseFileNameFunc(DefaultParseFileName)),


To get the expected behavior of NewFileStore("dirname", tsm1.WithParseFileNameFunc(myParseFunc)), the DefaultParseFileName must be prepended to the options. The appended DefaultParseFileName will cause DefaultParseFileName to be used instead of myParseFunc.

gwossum · 2025-09-17T19:31:03Z

While I'm still not a fan allowing partially working file stores, most of practical concerns could be addressed by adding a way for NewDefaultPlanner to determine if the fileStore it is given is usable for compaction. This could be addressed by adding a FileStore.SupportsCompaction method that lets the caller know if it is capable of being used for compaction (e.g. has a non-nil ParseFileNameFunc). Since DefaultPlanner is the only place that currently uses the generation and sequence number provided by the filename parser, the only possible place this causes a problem would be protected.

gwossum · 2025-09-17T19:32:43Z

There's also a "cute" way to check if FileStore has a parse function, which is to ask it to parse a filename generated by the file formatting function. This feels pretty kludgy, though, plus is more invasive to the code because the format file name function has to be passed in more places for this to work.

gwossum · 2025-09-17T19:42:36Z

Just did a quick PoC of the FileStore.SupportsCompaction idea. Takes about ~10 lines of code and all tests pass and influxd is happy. We can either say that this proves we don't have an issue with the current code misusing FileStore and call it a day, or we can add the code to prevent future issues.

davidby-influx · 2025-09-17T19:45:14Z

Do you wan to add your commit to the branch, or is it too PoC-ish?

gwossum · 2025-09-17T19:46:46Z

Let me add a few comments and then I can add it. I won't be able to approve it once I do that, so @devanbenz will have to approve the PR.

Add `FileStore.SupportsCompactionPlanning()` which allows compaction planners to check if a `FileStore` supports compaction planning. Currently this means the `FileStore` must have a TSM filename parsing function available.

devanbenz

Just one comment about the panic

devanbenz · 2025-09-17T20:31:55Z

tsdb/engine/tsm1/compact.go

 func NewDefaultPlanner(fs fileStore, writeColdDuration time.Duration) *DefaultPlanner {
+	if !fs.SupportsCompactionPlanning() {
+		// This should only happen due to developer mistakes.
+		panic("fileStore must support compaction planning")


I know this should only occur if a dev messes up, but, do we really want a panic in production code? Also, would it be better to have a method called MustSupportCompactionPlanning() to put this in that is similar to other parts of the codebase where we panic?

We had this convo in slack. Not panicking would be more invasive to the code (mainly test code), and the reason for even allowing a FileStore that didn't support compaction planning was to avoid a lot of test code changes. I'm also confident that you won't get this panic in production. If you panic in NewDefaultPlanner, influxd won't even start. You also can't a panic if the code is used properly. NewFileStore always sets a parse filename function. You have to either either to force the parse function to nil (not in codebase), create a FileStore without using NewFileStore (also not in codebase), or create various objects directly (only in tests).

With regards to MustSupportCompactionPlanning, SupportsCompactionPlanning isn't where the panic comes from. It leaves the choice of how to handle the issue up to the caller.

Sounds good 👍

devanbenz

LGTM

(cherry picked from commit 0ec0101)

davidby-influx · 2025-10-14T01:04:28Z

Work moved to feat: cache generation and sequence to reduce TSM filename parsing for cleanliness.

feat: cache generation and sequence to reduce TSM filename parsing

3a8a3b9

Fixes #26794

davidby-influx requested a review from gwossum September 13, 2025 01:12

davidby-influx self-assigned this Sep 13, 2025

davidby-influx added area/tsm kind/enhancement 1.x kind/perf team/edge labels Sep 13, 2025

fix: remove incorrect generation caching from DefaultPlanner.findGene…

256db1b

…rations

davidby-influx requested a review from devanbenz September 14, 2025 22:44

davidby-influx added the kind/bug label Sep 14, 2025

davidby-influx added 2 commits September 14, 2025 15:48

chore: format to reduce nugatory diffs

32dbf8c

chore: add TSM generation and sequence parsing to tests

706a091

davidby-influx marked this pull request as ready for review September 15, 2025 20:26

gwossum reviewed Sep 15, 2025

View reviewed changes

tsdb/engine/tsm1/file_store.go Outdated Show resolved Hide resolved

gwossum reviewed Sep 15, 2025

View reviewed changes

tsdb/engine/tsm1/file_store.go Outdated Show resolved Hide resolved

gwossum reviewed Sep 15, 2025

View reviewed changes

fix: better errors and checks

7d276df

devanbenz previously approved these changes Sep 16, 2025

View reviewed changes

fix: use nil check version of FileStore.ParseFileName

40e7d75

davidby-influx dismissed devanbenz’s stale review via 40e7d75 September 17, 2025 18:21

gwossum reviewed Sep 17, 2025

View reviewed changes

fix: option function ordering for FileStore

f71fac2

davidby-influx requested a review from devanbenz September 17, 2025 20:17

devanbenz reviewed Sep 17, 2025

View reviewed changes

devanbenz previously approved these changes Sep 17, 2025

View reviewed changes

fix: actually sort files for adjacency testing

542214c

(cherry picked from commit 0ec0101)

davidby-influx dismissed devanbenz’s stale review via 542214c September 24, 2025 22:38

davidby-influx closed this Oct 14, 2025

davidby-influx deleted the DSB_cache_generation branch October 14, 2025 01:07

Conversation

davidby-influx commented Sep 13, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devanbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwossum commented Sep 17, 2025

Uh oh!

gwossum commented Sep 17, 2025

Uh oh!

gwossum commented Sep 17, 2025

Uh oh!

davidby-influx commented Sep 17, 2025

Uh oh!

gwossum commented Sep 17, 2025

Uh oh!

devanbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devanbenz left a comment

Choose a reason for hiding this comment

Uh oh!

davidby-influx commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidby-influx commented Oct 14, 2025 •

edited

Loading