Vttablet schema tracking: Fix _vt.schema_version corruption

[brendar]

Description

This PR fixes a race condition which caused protobuf marshaled schema data in _vt.schema_version rows to become corrupted when the ColumnType of query.Field pointers was modified between the time when Field message sizes were calculated and when Field message data was written to the buffer.

On Vitess versions <= 16, the race condition leads to invalid protobuf data being written to the schema_version table. When the schema historian tries to unmarshal the data it encounters an error, which breaks schema tracking on running tablets, and prevents newly started tablets from serving.

On Vitess main the race condition leads to a panic and the schema_version row is never written. This is due to the switch to using MarshalVT in #12525

I'm proposing to fix this by copying the fields returned from schema.Engine before modifying them. As far as I can tell, nothing relied on the side effect of setting ColumnType on the shared fields.

This PR also includes a defensive change to acquire the schema.Engine mutex while marshaling the schema to protobuf. This is not strictly necessary to fix the bug, but it could help avoid future race conditions or ones that haven't been discovered yet. I would be happy to remove it if anyone feels that it's unnecessary.

This bug was pretty disruptive for us, so I think the fix should be backported, but note that if it is backported, it may be desirable to change the call to MarshalVT back to proto.Marshal to match the existing code in released versions.

Related Issue(s)

Fixes #12981

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation was added or is not required

Deployment Notes

[mattlord]

Looks good! I only had a few minor comments/requests. Please let me know if I'm missing something. I'll quickly approve once that's resolved.

Then we'll need someone from Query Serving to take a quick look as well.

Thank you for the contribution, @brendar! ❤️

[mattlord]

Lastly, if you merge in latest vitessio/main then you'll get fixes for the unit_race workflow that's failing.

[deepthi]

This bug was pretty disruptive for us, so I think the fix should be backported, but note that if it is backported, it may be desirable to change the call to MarshalVT back to proto.Marshal to match the existing code in released versions.

Agreed. However, the backport will need to be done manually, so I'm going to remove the label.

[harshit-gangal]

changes look good.

[rohit-nayak-ps]

Acquiring the schema engine mutex while marshalling makes sense.
Thanks for fixing this tricky race condition!

[austenLacy]

@mattlord can we get another 👀 on this when you have a chance? 🙇

[mattlord]

@austenLacy can you please merge in vitessio/main? That should resolve the unit_race workflow failures.

The static code check workflow is also failing: https://github.com/vitessio/vitess/actions/runs/5070727297/jobs/9106509343?pr=13045

Otherwise it's good! Once the tests are passing I'll approve and merge. ❤️

[austenLacy]

@mattlord fixed the gofmt issue and rebased off f5a460c.

$ git rev-parse --abbrev-ref HEAD
fix-vt-schema_version-corruption

$ git log --pretty=oneline f5a460ca4a73f5750c2262efc5663d9de04a31ac^..HEAD
8887af88c771dba36d6d65d0cca16479a0ec01e2 (HEAD -> fix-vt-schema_version-corruption) gofmt
50186f23971401a4692a17caaab7f3d8a9e33bf8 reorganize imports and allocate slices with make
7cdbb99196750a2baf6d9bda545c4d37503be51b Acquire schema engine mutex while marshaling schema
607031f753b1421f313d2e9da074fda9a883b2fc Copy fields from schema.Engine before modifying
f5a460ca4a73f5750c2262efc5663d9de04a31ac (origin/main, origin/HEAD) VReplication: Do not delete sharded target vschema table entries on Cancel (#13146)

[mattlord]

The changes in this PR are causing this test failure:

$ go test -count 1 -timeout 30s -run ^TestVStreamCopyCompleteFlow$ vitess.io/vitess/go/vt/vttablet/tabletserver/vstreamer

Summary:

--- FAIL: TestVStreamCopyCompleteFlow (2.18s)
    uvstreamer_flaky_test.go:316: Event 2 did not match, want

WANT:
type:FIELD field_event:{table_name:"t1" fields:{name:"id11" type:INT32 table:"t1" org_table:"t1" database:"vttest" org_name:"id11" column_length:11 charset:63 column_type:"int"} fields:{name:"id12" type:INT32 table:"t1" org_table:"t1" database:"vttest" org_name:"id12" column_length:11 charset:63 column_type:"int"}}

GOT:
type:FIELD field_event:{table_name:"t1" fields:{name:"id11" type:INT32 table:"t1" org_table:"t1" database:"vttest" org_name:"id11" column_length:11 charset:63} fields:{name:"id12" type:INT32 table:"t1" org_table:"t1" database:"vttest" org_name:"id12" column_length:11 charset:63}} keyspace:"vttest" shard:"0"

E0525 16:42:38.247619   51270 vstreamer.go:1015] stream (at source tablet) error @ 239a1164-fb1b-11ed-8845-6045bda98c87:1-139: cannot determine table columns for ddl_test2: event has [3 15 15], schema has [name:"id" type:INT32 table:"ddl_test2" org_table:"ddl_test2" database:"vttest" org_name:"id" column_length:11 charset:63 flags:53251 name:"val1" type:VARBINARY table:"ddl_test2" org_table:"ddl_test2" database:"vttest" org_name:"val1" column_length:128 charset:63 flags:128]

[austenLacy]

@rohit-nayak-ps

Can you take another look at this when you have a moment? There were some unit tests failing in uvstreamer_flakey_test.go that were definitely caused by my changes since they pass on main. I fixed them by modifying the expectations in the tests in feba91a, but I'm not familiar enough with the vstreamer and uvstreamer tests to know if that was the right thing to do or not.

Something that I also noticed was that the expected events in the uvstreamer and vstreamer tests are not the same.

vstreamer_flaky_test.go (doesn't have the column_type field in the event)

"type:FIELD field_event:{table_name:\"t1\" fields:{name:\"id1\" type:INT32 table:\"t1\" org_table:\"t1\" database:\"vttest\" org_name:\"id1\" column_length:11 charset:63} fields:{name:\"id2\" type:INT32 table:\"t1\" org_table:\"t1\" database:\"vttest\" org_name:\"id2\" column_length:11 charset:63}}",

uvstreamer_flaky_test.go (includes the column_type field)

"type:FIELD field_event:{table_name:\"t1\" fields:{name:\"id11\" type:INT32 table:\"t1\" org_table:\"t1\" database:\"vttest\" org_name:\"id11\" column_length:11 charset:63 column_type:\"int(11)\"} fields:{name:\"id12\" type:INT32 table:\"t1\" org_table:\"t1\" database:\"vttest\" org_name:\"id12\" column_length:11 charset:63 column_type:\"int(11)\"}}",

Is it possible the race condition of updating the ColumnType on the original Field was being worked around in the uvstreamer tests by adding it to the expected events?

Will rereview after the unit test failure is investigated.

[rohit-nayak-ps]

Is it possible the race condition of updating the ColumnType on the original Field was being worked around in the uvstreamer tests by adding it to the expected events?

Hmm, if that were true and if normally the ColumnType attribute was not set, the test should be erroring out very often, right? That is not what we see in CI.

Also, are you saying that ColumnType should not being set in the vstreamer in that test?

fields = st.Fields[:len(tm.Types)]
	extColInfos, err := vs.getExtColInfos(tm.Name, tm.Database)
	if err != nil {
		return nil, err
	}
	for _, field := range fields {
		// we want the MySQL column type info so that we can properly handle
		// ambiguous binlog events and other cases where the internal types
		// don't match the MySQL column type. One example being that in binlog
		// events CHAR columns with a binary collation are indistinguishable
		// from BINARY columns.
		if extColInfo, ok := extColInfos[field.Name]; ok {
			field.ColumnType = extColInfo.columnType
		}
	}
	return fields, nil

[austenLacy]

Also, are you saying that ColumnType should not being set in the vstreamer in that test?

@rohit-nayak-ps

I can't say for sure because I'm not super familiar with the differences between vstreamer and uvstreamer. Do you know why some of the events would have the ColumnType, but others don't?

[rohit-nayak-ps]

@austenLacy, @brendar, I debugged this branch and figured out the issue. You were right: there is a (planned) side-effect of the previous code that has caused the test to fail.

The rowstreamer also generates vevents in the copy phase. It was using the fields directly from the tablet's schema engine tables collection in its FieldEvent. The vstreamer used to first populate the ColumnInfo attribute of this collection in the vstreamers buildTablePlan. Now that we are cloning Fields in this PR the rowstreamer is no longer seeing the ColumnInfo since we no longer update the schema engine's tables.

So we need to modify rowstreamer as well to call getExtColInfos() and update that attribute before sending the field event. Some amount of refactor will be needed since getExtColInfos() is scoped only to vstreamer right now. It might also require other unit tests to be modified.

I can also work on this, but it will be a week or so before I can get to this since we have a Vitess v17 release coming up and I also have a couple of urgent issues on my plate at the moment. Let me know ...

[brendar]

Passing rs.vschema.keyspace may be problematic if the underlying database name is different from the keyspace name.

[rohit-nayak-ps]

Good catch. Sorry, I had caught it in the vstreamer call but forgot to update the rowstreamer one. Fixed here: 700e2dd

[austenLacy]

done in 48fd6ee

[austenLacy]

@rohit-nayak-ps I've updated the PR with your PR's latest commits and all checks are ✅ 🎉

[rohit-nayak-ps]

lgtm
Thank you for the PR!

[mattlord]

❤️