feat: Bubble error up to writer if fields are dropped #26998

devanbenz · 2025-12-01T20:41:29Z

Modify WritePoints to return an error if a user defined time field is inserted. Previously we would silently prune it, this PR will bubble an error up to the writer to inform that a field was dropped but not the entire point.

> INSERT test_foo,tag1=abc,tag2=xyz uuuu=7777777i,time=123i
ERR: {"error":"partial write: invalid field name: input field \"time\" on measurement \"test_foo\" is invalid. Field \"time\" has been stripped from point. dropped_points=0 for database: foo for retention policy: autogen"}

> select * from test_foo
name: test_foo
time                tag1  tag2 uuuu
----                ----  ---- ----
1764705088831162000  abc  xyz  7777777

davidby-influx · 2025-12-02T01:02:18Z

tsdb/shard.go

+		newFields, partialWriteError := ValidateAndCreateFields(mf, p, s.options.Config.SkipFieldSizeValidation, s.logger)
 		createdFieldsToSave = append(createdFieldsToSave, newFields...)

 		if partialWriteError != nil {


Here we would only want to continue if the PartialWriteError.Dropped > 0, otherwise do the point assignment a increment j. Then in the test on 730, check a bool if we had any PartialWriteError, rather than the count of dropped lines.

Set a bool here partialwrites = true, then use on shard.go:L730 to decide whether to return an error.

davidby-influx · 2025-12-02T01:02:23Z

tsdb/field_validator.go

 		fieldKey := iter.FieldKey()
 		// Skip fields name "time", they are illegal.
 		if bytes.Equal(fieldKey, timeBytes) {
+			logger.Warn("invalid field name \"time\" for measurement, dropping \"time\" field during write.")


My difficulty with this is that the writer does not get the error. I would prefer creating a PartialWriteError with a Dropped count of zero, and the same error message we give with single field time problems.

In the caller we then, rather than checking for PartialWriteError and continuing if we have one, we check for a PartialWriteError with a Dropped count > 0.

We will be swamped in our log files if we log every time field that occurs; that could be millions of erroneous points filling the logs before the customer catches the problem. We want the error to go back to the writer.

bubble error up to writer.

…ters

davidby-influx

Some suggestions. Don't forget to make cherry-pick issues for main-2.x in both related projects.

What should happen with engine.WritePoints check for timeBytes? Is it redundant, or does it plug some gap the other checks leave?

And the check on line 692 of shard.go - redundant, or plugging a gap in other checks?

davidby-influx · 2025-12-03T18:29:06Z

tsdb/field_validator.go

+			partialWriteError = &PartialWriteError{
+				Reason: fmt.Sprintf(
+					"invalid field name: input field \"%s\" on measurement \"%s\" is invalid. Field \"%s\" has been stripped from point.",
+					"time", string(point.Name()), "time"),


Don't use the literal "time" here, use the fieldKey value (use square bracket notation to only pass once to Sprintf). Or maybe rewrite the message to only print the value once.

davidby-influx · 2025-12-03T19:34:36Z

tsdb/shard.go

+			// Sometimes we will drop fields like 'time' but not an entire point
+			// we want to inform the writer that something occurred.
+		} else if partialWriteError != nil && partialWriteError.Dropped <= 0 {
+			err = PartialWriteError{


Do we need to copy the error here into a new error?

~~No we want to use the existing error since it gets returned~~

Sorry, misread the question. We don't need to, I was doing that so we could set the DB and RP. But, I can just mutate the error that is already there and assign it to err.

davidby-influx · 2025-12-03T19:47:48Z

tsdb/field_validator.go


 		fieldKey := iter.FieldKey()
 		// Skip fields name "time", they are illegal.
 		if bytes.Equal(fieldKey, timeBytes) {


Let's unify the two timeBytes declarations into one tsdb.TimeBytes in shard.go

devanbenz · 2025-12-04T15:42:46Z

Ticket for backport #27006

devanbenz · 2025-12-04T16:02:23Z

What should happen with engine.WritePoints check for timeBytes? Is it redundant, or does it plug some gap the other checks leave?

It does appear this check is not needed. I had removed it and observed the same behavior without it. Taking a look at the code the only place engine.WritePoints gets called is from within Shard.WritePoints which calls in to the field validator logic that strips the time field

	points, fieldsToCreate, err := s.validateSeriesAndFields(points, tracker) // Here is where we validate and strip the field
	if err != nil {
		if _, ok := err.(PartialWriteError); !ok {
			return err
		}
		// There was a partial write (points dropped), hold onto the error to return
		// to the caller, but continue on writing the remaining points.
		writeError = err
	}

.......

	// Write to the engine.
	if err := engine.WritePoints(points, engineTracker); err != nil { // Here is where engine.WritePoints gets called
		atomic.AddInt64(&s.stats.WritePointsErr, int64(len(points)))
		atomic.AddInt64(&s.stats.WriteReqErr, 1)
		return fmt.Errorf("engine: %w", err)
	}

And the check on line 692 of shard.go - redundant, or plugging a gap in other checks?

The check on line 692 is an optimization where if we have a single field using time we immediately just drop the point. This check plugs a gap. Since we don't immediately drop the point where the other check is. I think we should keep this and not alter the code too much attempting to combine the checks.

davidby-influx

Good changes; two code suggestions, one additional question.

davidby-influx · 2025-12-04T17:03:56Z

tsdb/field_validator.go

+			partialWriteError = &PartialWriteError{
+				Reason: fmt.Sprintf(
+					"invalid field name: input field \"%[1]s\" on measurement \"%s\" is invalid. Field \"%[1]s\" has been stripped from point.",
+					string(fieldKey), string(point.Name())),


You probably do not need the cast to string; Sprintf can treat byte slices correctly with %s, I believe

How do we handle multiple time fields in a single point? Do we need to return multiple partial write errors, or should we short-circuit the creation of the second and subsequent ones like the following?

if bytes.Equal(fieldKey, TimeBytes) && (nil == partialWriteError) {

If we have multiple time fields we want to hit the continue line regardless so I think the existing code is fine.

Also, output when you have multiple time fields.

> use foo Using database foo > INSERT test_foo7,tag1=abc,tag2=xyz uuuu=8989i,time=123i,time=99999i ERR: {"error":"partial write: invalid field name: input field \"time\" on measurement \"test_foo7\" is invalid. Field \"time\" has been stripped from point. dropped=0 for database: foo for retention policy: autogen"} > select * from test_foo7 name: test_foo7 time tag1 tag2 uuuu ---- ---- ---- ---- 1764948955470709000 abc xyz 8989

If you only have time fields (drops the entire point as expected):

> INSERT test_foo8,tag1=abc,tag2=xyz time=12345i,time=123i,time=99999i ERR: {"error":"partial write: invalid field name: input field \"time\" on measurement \"test_foo8\" is invalid dropped=1 for database: foo for retention policy: autogen"} > select * from test_foo8 >

Eliminate the string() casts for the fmt.Sprintf and put a nested if check whether partialWriteError is nil so you only set it once, even for multiple time fields in a single point.

tsdb/engine/tsm1/engine.go

davidby-influx

A few small changes for clarity and memory allocation.

davidby-influx · 2025-12-05T20:14:40Z

tsdb/field_validator.go

+			partialWriteError = &PartialWriteError{
+				Reason: fmt.Sprintf(
+					"invalid field name: input field \"%[1]s\" on measurement \"%s\" is invalid. Field \"%[1]s\" has been stripped from point.",
+					string(fieldKey), string(point.Name())),


Eliminate the string() casts for the fmt.Sprintf and put a nested if check whether partialWriteError is nil so you only set it once, even for multiple time fields in a single point.

davidby-influx · 2025-12-05T20:17:21Z

tsdb/shard.go

 			continue
+			// Sometimes we will drop fields like 'time' but not an entire point
+			// we want to inform the writer that something occurred.
+		} else if partialWriteError != nil && partialWriteError.Dropped <= 0 {


Is the check for Dropped redundant here?

devanbenz added 2 commits December 1, 2025 14:40

chore: Add warn log for when we drop a "time" field from a point

6ea8c71

chore: add newline back

cb33dcf

devanbenz self-assigned this Dec 1, 2025

davidby-influx reviewed Dec 2, 2025

View reviewed changes

devanbenz changed the title ~~chore: Add warn log for when we drop a "time" field from a point~~ feat: Bubble error up to writer if fields are dropped Dec 2, 2025

devanbenz added 8 commits December 2, 2025 13:08

feat: Instead of warning on "time" being written

fa5ba4f

bubble error up to writer.

feat: Use pointers and errors.As

724391c

feat: Do type coercion instead we have a mix of pointers and non-poin…

c464b65

…ters

chore: revert back to dropped

d3bf074

feat: Adds a test for dropped fields

c15199b

feat: Update TestWriteTimeTag

18e835a

feat: Add measurement field assert

01c5a3f

feat: use different string for ErrorContains

928b1c6

devanbenz marked this pull request as ready for review December 3, 2025 16:33

devanbenz requested a review from davidby-influx December 3, 2025 16:33

davidby-influx requested changes Dec 3, 2025

View reviewed changes

devanbenz mentioned this pull request Dec 4, 2025

[2.x] Backport: Bubble error up to writer if fields are dropped #27006

Open

devanbenz added 2 commits December 4, 2025 10:03

feat: Export TimeBytes from tsdb, remove check that wasn't needed

2044458

feat: assign to original error

281877e

devanbenz requested a review from davidby-influx December 4, 2025 16:29

davidby-influx requested changes Dec 4, 2025

View reviewed changes

devanbenz requested a review from davidby-influx December 4, 2025 21:50

davidby-influx requested changes Dec 5, 2025

View reviewed changes

feat: Bubble error up to writer if fields are dropped #26998

Are you sure you want to change the base?

feat: Bubble error up to writer if fields are dropped #26998

Conversation

devanbenz commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

davidby-influx 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!

devanbenz Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devanbenz commented Dec 4, 2025

Uh oh!

devanbenz commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidby-influx 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidby-influx 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

devanbenz commented Dec 1, 2025 •

edited

Loading

devanbenz Dec 3, 2025 •

edited

Loading

devanbenz commented Dec 4, 2025 •

edited

Loading