Fix inspector column types #661
Conversation
The given `columnLists` may not contain all columns available in the
given table. This patch prevents the code to fail. Before this patch the
code was panicking whenever it was trying to set a value into a `nil`
object, e.g. `columnList.GetColumn('non-existant').Type = SomeType`.
For some reason the application was not completely failing but as a
side-effect any column after the non-existant column would never get its
Type properly set.
There is no need to call `applyColumnTypes` more than once for the same `databaseName` and `tableName`, it is just move the additional `columnList` to the first call.
There was a problem hiding this comment.
Thank you for this PR. Can you please clarify inline about the changes you made?
Why the removal of this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, &this.migrationContext.UniqueKey.Columns)?
Why the reverse of strings.Contains vs for _, columnsList := range columnsLists ?
| @@ -173,8 +173,7 @@ func (this *Inspector) inspectOriginalAndGhostTables() (err error) { | |||
| // This additional step looks at which columns are unsigned. We could have merged this within | |||
| // the `getTableColumns()` function, but it's a later patch and introduces some complexity; I feel | |||
| // comfortable in doing this as a separate step. | |||
| this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, this.migrationContext.OriginalTableColumns, this.migrationContext.SharedColumns) | |||
| this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, &this.migrationContext.UniqueKey.Columns) | |||
| this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, this.migrationContext.OriginalTableColumns, this.migrationContext.SharedColumns, &this.migrationContext.UniqueKey.Columns) | |||
There was a problem hiding this comment.
This is just a small improvement, instead of calling applyColumnTypes twice for the same databaseName and tableName, I've moved the &this.migrationContext.UniqueKey.Columns to this call.
| } | ||
| if strings.Contains(columnType, "mediumint") { | ||
| for _, columnsList := range columnsLists { | ||
| columnsList.GetColumn(columnName).Type = sql.MediumIntColumnType |
There was a problem hiding this comment.
This code fails when columnsList.GetColumn(columnName) returns nil. This is going to happen in this.migrationContext.MappedSharedColumns and this.migrationContext.SharedColumns due to the fact they they only have the intersection of original and ghost tables. If you add a new column by the end of the column list this problem won't be noticed as the code is going to fail when setting the type of the new column (which is not used during copy of data), but if you alter table and add a column in the second position, all other columns will have the default Type (0) because the code will not iterate over all of them.
| columnsList.SetUnsigned(columnName) | ||
| for _, columnsList := range columnsLists { | ||
| column := columnsList.GetColumn(columnName) | ||
| if column == nil { |
There was a problem hiding this comment.
The for loop was changed in order to prevent accessing a column that is not available in the columnsList object.
There was a problem hiding this comment.
This code ignores columns that are not available in the columnsList and continues iterating over the next columns.
|
Hi @shlomi-noach , thanks for you fast reply. I've added some inline comments, I hope they make clear the intent of this PR. Let me know if you need any further clarification. This is a big issue for me as it prevented me to use |
|
Thank you for replying. I'll take this change to testing. It'll like take a few days or over a week for me to respond to this PR, given my current workload, sorry for the expected delay. |
|
No worries and thanks for the heads up regarding the time to release. |
|
Hey @shlomi-noach 👋, any news on this one? Thanks! |
shlomi-noach
temporarily deployed
to
production/mysql_role=ghost_testing
|
sent to production testing |
|
Meanwhile sorry for the delay. |
|
This looks good in testing and I'll merge this PR early next week. |
Issue
Related issue: #660
Description
This PR fix an issue in
Inspector.applyColumnTypes. The function fails when acolumnListdoes not contain all table columns. For some reason the code fails silently, so any other column after the first failure will be identified as type0(defaultintvalue).As a side-effect when copying rows from original table to
gh-osttemporary one, it may fail due to bad type conversion as describe in the related issue.Notes
I've considered adding tests but they would require extract the column type logic to a different function or add an external library to mock
sql.DB.