Fix CalculateNextIterationRangeEndValues order by.

The split between Temptable and Offset query builders was originally introduced in #471. Then, both query builders have been changed to calculate actual chunk sizes in #1500.

The updated implementation of the Offset query introduced a bug, where a missing `order by` clause in `select_osc_chunk` can result in end values potentially surpassing the chunk_size. This wasn't detected during our initial testing where the query just returned rows in their original order, but may happen in real-world scenarios in case the db returns data in an undefined order.

An obvious fix would be to just add an `order by` to the Offset builder subquery, however since both builders use temptables now, it makes more sense to simplify and use only one of them.

Alternatively, the builder could only use the less performant count query variants when `--panic-on-warnings` is enabled and otherwise use the simpler ones from pull/471. We decided to not follow this path for now, hoping `--panic-on-warnings` becomes an updated and safer default in the future.

Co-authored-by: Bastian Bartmann <[email protected]>

Author: grodowski

go/logic/applier.go

@@ -668,49 +668,44 @@ func (this *Applier) CalculateNextIterationRangeEndValues() (hasFurtherRange boo
 	if this.migrationContext.MigrationIterationRangeMinValues == nil {
 		this.migrationContext.MigrationIterationRangeMinValues = this.migrationContext.MigrationRangeMinValues
 	}
-	for i := 0; i < 2; i++ {
-		buildFunc := sql.BuildUniqueKeyRangeEndPreparedQueryViaOffset
-		if i == 1 {
-			buildFunc = sql.BuildUniqueKeyRangeEndPreparedQueryViaTemptable
-		}
-		query, explodedArgs, err := buildFunc(
-			this.migrationContext.DatabaseName,
-			this.migrationContext.OriginalTableName,
-			&this.migrationContext.UniqueKey.Columns,
-			this.migrationContext.MigrationIterationRangeMinValues.AbstractValues(),
-			this.migrationContext.MigrationRangeMaxValues.AbstractValues(),
-			atomic.LoadInt64(&this.migrationContext.ChunkSize),
-			this.migrationContext.GetIteration() == 0,
-			fmt.Sprintf("iteration:%d", this.migrationContext.GetIteration()),
-		)
-		if err != nil {
-			return hasFurtherRange, expectedRowCount, err
-		}
-
-		rows, err := this.db.Query(query, explodedArgs...)
-		if err != nil {
-			return hasFurtherRange, expectedRowCount, err
-		}
-		defer rows.Close()
 
-		iterationRangeMaxValues := sql.NewColumnValues(this.migrationContext.UniqueKey.Len() + 1)
-		for rows.Next() {
-			if err = rows.Scan(iterationRangeMaxValues.ValuesPointers...); err != nil {
-				return hasFurtherRange, expectedRowCount, err
-			}
+	query, explodedArgs, err := sql.BuildUniqueKeyRangeEndPreparedQueryViaTemptable(
+		this.migrationContext.DatabaseName,
+		this.migrationContext.OriginalTableName,
+		&this.migrationContext.UniqueKey.Columns,
+		this.migrationContext.MigrationIterationRangeMinValues.AbstractValues(),
+		this.migrationContext.MigrationRangeMaxValues.AbstractValues(),
+		atomic.LoadInt64(&this.migrationContext.ChunkSize),
+		this.migrationContext.GetIteration() == 0,
+		fmt.Sprintf("iteration:%d", this.migrationContext.GetIteration()),
+	)
+	if err != nil {
+		return hasFurtherRange, expectedRowCount, err
+	}
 
-			expectedRowCount = (*iterationRangeMaxValues.ValuesPointers[len(iterationRangeMaxValues.ValuesPointers)-1].(*interface{})).(int64)
-			iterationRangeMaxValues = sql.ToColumnValues(iterationRangeMaxValues.AbstractValues()[:len(iterationRangeMaxValues.AbstractValues())-1])
+	rows, err := this.db.Query(query, explodedArgs...)
+	if err != nil {
+		return hasFurtherRange, expectedRowCount, err
+	}
+	defer rows.Close()
 
-			hasFurtherRange = expectedRowCount > 0
-		}
-		if err = rows.Err(); err != nil {
+	iterationRangeMaxValues := sql.NewColumnValues(this.migrationContext.UniqueKey.Len() + 1)
+	for rows.Next() {
+		if err = rows.Scan(iterationRangeMaxValues.ValuesPointers...); err != nil {
 			return hasFurtherRange, expectedRowCount, err
 		}
-		if hasFurtherRange {
-			this.migrationContext.MigrationIterationRangeMaxValues = iterationRangeMaxValues
-			return hasFurtherRange, expectedRowCount, nil
-		}
+
+		expectedRowCount = (*iterationRangeMaxValues.ValuesPointers[len(iterationRangeMaxValues.ValuesPointers)-1].(*interface{})).(int64)
+		iterationRangeMaxValues = sql.ToColumnValues(iterationRangeMaxValues.AbstractValues()[:len(iterationRangeMaxValues.AbstractValues())-1])
+
+		hasFurtherRange = expectedRowCount > 0
+	}
+	if err = rows.Err(); err != nil {
+		return hasFurtherRange, expectedRowCount, err
+	}
+	if hasFurtherRange {
+		this.migrationContext.MigrationIterationRangeMaxValues = iterationRangeMaxValues
+		return hasFurtherRange, expectedRowCount, nil
 	}
 	this.migrationContext.Log.Debugf("Iteration complete: no further range to iterate")
 	return hasFurtherRange, expectedRowCount, nil

go/sql/builder.go

@@ -251,80 +251,6 @@ func BuildRangeInsertPreparedQuery(databaseName, originalTableName, ghostTableNa
 	return BuildRangeInsertQuery(databaseName, originalTableName, ghostTableName, sharedColumns, mappedSharedColumns, uniqueKey, uniqueKeyColumns, rangeStartValues, rangeEndValues, rangeStartArgs, rangeEndArgs, includeRangeStartValues, transactionalTable, noWait)
 }
 
-func BuildUniqueKeyRangeEndPreparedQueryViaOffset(databaseName, tableName string, uniqueKeyColumns *ColumnList, rangeStartArgs, rangeEndArgs []interface{}, chunkSize int64, includeRangeStartValues bool, hint string) (result string, explodedArgs []interface{}, err error) {
-	if uniqueKeyColumns.Len() == 0 {
-		return "", explodedArgs, fmt.Errorf("Got 0 columns in BuildUniqueKeyRangeEndPreparedQuery")
-	}
-	databaseName = EscapeName(databaseName)
-	tableName = EscapeName(tableName)
-
-	var startRangeComparisonSign ValueComparisonSign = GreaterThanComparisonSign
-	if includeRangeStartValues {
-		startRangeComparisonSign = GreaterThanOrEqualsComparisonSign
-	}
-	rangeStartComparison, rangeExplodedArgs, err := BuildRangePreparedComparison(uniqueKeyColumns, rangeStartArgs, startRangeComparisonSign)
-	if err != nil {
-		return "", explodedArgs, err
-	}
-	explodedArgs = append(explodedArgs, rangeExplodedArgs...)
-	rangeEndComparison, rangeExplodedArgs, err := BuildRangePreparedComparison(uniqueKeyColumns, rangeEndArgs, LessThanOrEqualsComparisonSign)
-	if err != nil {
-		return "", explodedArgs, err
-	}
-	explodedArgs = append(explodedArgs, rangeExplodedArgs...)
-
-	uniqueKeyColumnNames := duplicateNames(uniqueKeyColumns.Names())
-	uniqueKeyColumnAscending := make([]string, len(uniqueKeyColumnNames))
-	for i, column := range uniqueKeyColumns.Columns() {
-		uniqueKeyColumnNames[i] = EscapeName(uniqueKeyColumnNames[i])
-		if column.Type == EnumColumnType {
-			uniqueKeyColumnAscending[i] = fmt.Sprintf("concat(%s) asc", uniqueKeyColumnNames[i])
-		} else {
-			uniqueKeyColumnAscending[i] = fmt.Sprintf("%s asc", uniqueKeyColumnNames[i])
-		}
-	}
-	joinedColumnNames := strings.Join(uniqueKeyColumnNames, ", ")
-	result = fmt.Sprintf(`
-		select /* gh-ost %s.%s %s */
-			%s,
-			(select count(*) from (
-				select
-					%s
-				from
-					%s.%s
-				where
-					%s and %s
-				limit
-					%d
-			) select_osc_chunk)
-		from (
-			select
-				%s
-			from
-				%s.%s
-			where
-				%s and %s
-			limit
-				%d
-		) select_osc_chunk
-		order by
-			%s
-		limit 1
-		offset %d`,
-		databaseName, tableName, hint,
-		joinedColumnNames, joinedColumnNames,
-		databaseName, tableName,
-		rangeStartComparison, rangeEndComparison, chunkSize,
-		joinedColumnNames,
-		databaseName, tableName,
-		rangeStartComparison, rangeEndComparison, chunkSize,
-		strings.Join(uniqueKeyColumnAscending, ", "),
-		(chunkSize - 1),
-	)
-	// 2x the explodedArgs for the subquery (CTE would be possible but not supported by MySQL 5)
-	return result, append(explodedArgs, explodedArgs...), nil
-}
-
 func BuildUniqueKeyRangeEndPreparedQueryViaTemptable(databaseName, tableName string, uniqueKeyColumns *ColumnList, rangeStartArgs, rangeEndArgs []interface{}, chunkSize int64, includeRangeStartValues bool, hint string) (result string, explodedArgs []interface{}, err error) {
 	if uniqueKeyColumns.Len() == 0 {
 		return "", explodedArgs, fmt.Errorf("Got 0 columns in BuildUniqueKeyRangeEndPreparedQuery")

go/sql/builder_test.go

@@ -325,7 +325,7 @@ func TestBuildRangeInsertPreparedQuery(t *testing.T) {
 	}
 }
 
-func TestBuildUniqueKeyRangeEndPreparedQuery(t *testing.T) {
+func TestBuildUniqueKeyRangeEndPreparedQueryViaTemptable(t *testing.T) {
 	databaseName := "mydb"
 	originalTableName := "tbl"
 	var chunkSize int64 = 500