TiFlash server can not restart normally when meet tidb#31048

[JaySon-Huang]

Enhancement

A user met this error when using tidb v5.2.2 pingcap/tidb#31048. The error message indicates that tidb-server unexpectedly change the column type from Nullable(datetime(0)) to Nullable(int).

After that, the user set the tiflash replica to be 0 for the table (with table_id=756 in this case) through SQL, and restarts all three TiFlash servers. However, after restarted, two of the TiFlash server can restart but one TiFlash server can not restart normally. This made trouble for the user cause it made almost all queries to the TiFlash server fail.

---

We'd better figure out after setting the tiflash replica to be 0, why 2 out of 3 TiFlash servers can restart normally but 1 of them is NOT.
Minimizing the impact on users after this kind of exception (e.g. make the tiflash server can restart and serve queries to other tables that don't meet this exception)

[JaySon-Huang]

After user setting the tiflash replica to be 0, we may accept the column type change from datetime to int. But the log above is not as expected.

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 1298 to 1304 in ad6b831

	/// Rename if needed.
	applyRenameLogicalTable(db, table, storage);
	/// Update replica info if needed.
	applySetTiFlashReplica(db, table, storage);
	/// Alter if needed.
	applyAlterLogicalTable(db, table, storage);
	LOG_FMT_DEBUG(log, "Table {} synced during sync all schemas", name_mapper.debugCanonicalName(*db, *table));

tiflash/dbms/src/Storages/StorageDeltaMerge.cpp

Lines 1066 to 1095 in ad6b831

	if (unlikely(!isSupportedDataTypeCast(col_iter->type, command.data_type)))
	{
	// If this table has no tiflash replica, simply ignore this check because TiDB constraint
	// on DDL is not strict. (https://github.com/pingcap/tidb/issues/17530)
	// If users applied unsupported column type change on table with tiflash replica. To get rid of
	// this exception and avoid of reading broken data, they have truncate that table.
	if (table_info && table_info.value().get().replica_info.count == 0)
	{
	LOG_FMT_WARNING(
	log,
	"Accept lossy column data type modification. Table (id:{}) modify column {}({}) from {} to {}",
	table_info.value().get().id,
	command.column_name,
	command.column_id,
	col_iter->type->getName(),
	command.data_type->getName());
	}
	else
	{
	// check that lossy changes is forbidden
	// check that changing the UNSIGNED attribute is forbidden
	throw Exception(
	fmt::format("Storage engine {} doesn't support lossy data type modification. Try to modify column {}({}) from {} to {}",
	getName(),
	command.column_name,
	command.column_id,
	col_iter->type->getName(),
	command.data_type->getName()),
	ErrorCodes::NOT_IMPLEMENTED);
	}

[JaySon-Huang]

/assign @hongyunyan

[ti-chi-bot]

@JaySon-Huang: GitHub didn't allow me to assign the following users: hongyunyan.

Note that only pingcap members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @hongyunyan

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[hongyunyan]

/assign hongyunyan

[JaySon-Huang]

close as not reproducible