Enable the errcheck linter

.golangci.yml

@@ -1,8 +1,6 @@
 run:
-  timeout: 5m
+  timeout: 3m
 linters:
-  disable:
-    - errcheck
   enable:
     - bodyclose
     - containedctx
@@ -23,8 +21,23 @@ linters:
     - prealloc
     - rowserrcheck
     - sqlclosecheck
+    - structcheck
     - unconvert
     - unparam
     - unused
     - wastedassign
     - whitespace
+issues:
+  exclude-rules:
+    - linters:
+        - errcheck
+      path: '(.+)_test\.go'
+    - linters:
+        - errcheck
+      text: 'Error return value of `(this\.)?migrationContext\.Log\.(Errore|Errorf|Fatal|Fatale|Fatalf|Warning|Warningf)`'
+    - linters:
+        - errcheck
+      text: 'Error return value of `tx.Rollback`'
+    - linters:
+        - errcheck
+      text: 'Error return value of `log\.(Errore|Errorf|Fatal|Fatale|Fatalf|Warning|Warningf)`'

go/cmd/gh-ost/main.go

@@ -183,7 +183,10 @@ func main() {
 	if migrationContext.AlterStatement == "" {
 		log.Fatal("--alter must be provided and statement must not be empty")
 	}
-	parser := sql.NewParserFromAlterStatement(migrationContext.AlterStatement)
+	parser, err := sql.NewParserFromAlterStatement(migrationContext.AlterStatement)
+	if err != nil {
+		log.Fatale(err)
+	}
 	migrationContext.AlterStatementOptions = parser.GetAlterStatementOptions()
 
 	if migrationContext.DatabaseName == "" {
@@ -302,7 +305,9 @@ func main() {
 
 	migrator := logic.NewMigrator(migrationContext, AppVersion)
 	if err := migrator.Migrate(); err != nil {
-		migrator.ExecOnFailureHook()
+		if err = migrator.ExecOnFailureHook(); err != nil {
+			migrationContext.Log.Errore(err)
+		}
 		migrationContext.Log.Fatale(err)
 	}
 	fmt.Fprintln(os.Stdout, "# Done")

go/logic/applier.go

@@ -146,19 +146,19 @@ func (this *Applier) readTableColumns() (err error) {
 }
 
 // showTableStatus returns the output of `show table status like '...'` command
-func (this *Applier) showTableStatus(tableName string) (rowMap sqlutils.RowMap) {
+func (this *Applier) showTableStatus(tableName string) (rowMap sqlutils.RowMap, err error) {
 	query := fmt.Sprintf(`show /* gh-ost */ table status from %s like '%s'`, sql.EscapeName(this.migrationContext.DatabaseName), tableName)
-	sqlutils.QueryRowsMap(this.db, query, func(m sqlutils.RowMap) error {
+	err = sqlutils.QueryRowsMap(this.db, query, func(m sqlutils.RowMap) error {
 		rowMap = m
 		return nil
 	})
-	return rowMap
+	return rowMap, err
 }
 
 // tableExists checks if a given table exists in database
-func (this *Applier) tableExists(tableName string) (tableFound bool) {
-	m := this.showTableStatus(tableName)
-	return (m != nil)
+func (this *Applier) tableExists(tableName string) (tableFound bool, err error) {
+	m, err := this.showTableStatus(tableName)
+	return (m != nil), err
 }
 
 // ValidateOrDropExistingTables verifies ghost and changelog tables do not exist,
@@ -169,7 +169,10 @@ func (this *Applier) ValidateOrDropExistingTables() error {
 			return err
 		}
 	}
-	if this.tableExists(this.migrationContext.GetGhostTableName()) {
+	tableExists, err := this.tableExists(this.migrationContext.GetGhostTableName())
+	if err != nil {
+		return err
+	} else if tableExists {
 		return fmt.Errorf("Table %s already exists. Panicking. Use --initially-drop-ghost-table to force dropping it, though I really prefer that you drop it or rename it away", sql.EscapeName(this.migrationContext.GetGhostTableName()))
 	}
 	if this.migrationContext.InitiallyDropOldTable {
@@ -181,7 +184,10 @@ func (this *Applier) ValidateOrDropExistingTables() error {
 		this.migrationContext.Log.Fatalf("--timestamp-old-table defined, but resulting table name (%s) is too long (only %d characters allowed)", this.migrationContext.GetOldTableName(), mysql.MaxTableNameLength)
 	}
 
-	if this.tableExists(this.migrationContext.GetOldTableName()) {
+	tableExists, err = this.tableExists(this.migrationContext.GetOldTableName())
+	if err != nil {
+		return err
+	} else if tableExists {
 		return fmt.Errorf("Table %s already exists. Panicking. Use --initially-drop-old-table to force dropping it, though I really prefer that you drop it or rename it away", sql.EscapeName(this.migrationContext.GetOldTableName()))
 	}
 
@@ -378,7 +384,9 @@ func (this *Applier) WriteChangelog(hint, value string) (string, error) {
 }
 
 func (this *Applier) WriteAndLogChangelog(hint, value string) (string, error) {
-	this.WriteChangelog(hint, value)
+	if hint, err := this.WriteChangelog(hint, value); err != nil {
+		return hint, err
+	}
 	return this.WriteChangelog(fmt.Sprintf("%s at %d", hint, time.Now().UnixNano()), value)
 }
 
@@ -404,7 +412,9 @@ func (this *Applier) InitiateHeartbeat() {
 		}
 		return nil
 	}
-	injectHeartbeat()
+	if err := injectHeartbeat(); err != nil {
+		this.migrationContext.Log.Errorf("Failed to inject heartbeat: %+v", err)
+	}
 
 	ticker := time.NewTicker(time.Duration(this.migrationContext.HeartbeatIntervalMilliseconds) * time.Millisecond)
 	defer ticker.Stop()
@@ -419,6 +429,7 @@ func (this *Applier) InitiateHeartbeat() {
 			continue
 		}
 		if err := injectHeartbeat(); err != nil {
+			this.migrationContext.Log.Errorf("Failed to inject heartbeat: %+v", err)
 			return
 		}
 	}
@@ -863,10 +874,10 @@ func (this *Applier) ExpectProcess(sessionId int64, stateHint, infoHint string)
 func (this *Applier) DropAtomicCutOverSentryTableIfExists() error {
 	this.migrationContext.Log.Infof("Looking for magic cut-over table")
 	tableName := this.migrationContext.GetOldTableName()
-	rowMap := this.showTableStatus(tableName)
-	if rowMap == nil {
-		// Table does not exist
-		return nil
+	rowMap, err := this.showTableStatus(tableName)
+	if err != nil || rowMap == nil {
+		// Error or table does not exist (nil)
+		return err
 	}
 	if rowMap["Comment"].String != atomicCutOverMagicHint {
 		return fmt.Errorf("Expected magic comment on %s, did not find it", tableName)

go/logic/inspect.go

@@ -135,7 +135,9 @@ func (this *Inspector) inspectOriginalAndGhostTables() (err error) {
 	}
 	sharedUniqueKeys := this.getSharedUniqueKeys(this.migrationContext.OriginalTableUniqueKeys, this.migrationContext.GhostTableUniqueKeys)
 	for i, sharedUniqueKey := range sharedUniqueKeys {
-		this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, &sharedUniqueKey.Columns)
+		if err = this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, &sharedUniqueKey.Columns); err != nil {
+			return err
+		}
 		uniqueKeyIsValid := true
 		for _, column := range sharedUniqueKey.Columns.Columns() {
 			switch column.Type {
@@ -177,8 +179,14 @@ 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.migrationContext.UniqueKey.Columns)
-	this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.GetGhostTableName(), this.migrationContext.GhostTableColumns, this.migrationContext.MappedSharedColumns)
+	if err = this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, this.migrationContext.OriginalTableColumns,
+		this.migrationContext.SharedColumns, &this.migrationContext.UniqueKey.Columns); err != nil {
+		return err
+	}
+	if err = this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.GetGhostTableName(), this.migrationContext.GhostTableColumns,
+		this.migrationContext.MappedSharedColumns); err != nil {
+		return err
+	}
 
 	for i := range this.migrationContext.SharedColumns.Columns() {
 		column := this.migrationContext.SharedColumns.Columns()[i]

go/logic/migrator.go

@@ -21,6 +21,7 @@ import (
 	"github.com/github/gh-ost/go/binlog"
 	"github.com/github/gh-ost/go/mysql"
 	"github.com/github/gh-ost/go/sql"
+	"github.com/openark/golib/log"
 )
 
 var (
@@ -304,7 +305,7 @@ func (this *Migrator) countTableRows() (err error) {
 		this.migrationContext.SetCountTableRowsCancelFunc(rowCountCancel)
 
 		this.migrationContext.Log.Infof("As instructed, counting rows in the background; meanwhile I will use an estimated count, and will update it later on")
-		go countRowsFunc(rowCountContext)
+		go countRowsFunc(rowCountContext) // nolint:errcheck
 
 		// and we ignore errors, because this turns to be a background job
 		return nil
@@ -380,7 +381,7 @@ func (this *Migrator) Migrate() (err error) {
 	if err := this.initiateServer(); err != nil {
 		return err
 	}
-	defer this.server.RemoveSocketFile()
+	defer this.server.RemoveSocketFile() // nolint:errcheck
 
 	if err := this.countTableRows(); err != nil {
 		return err
@@ -397,8 +398,16 @@ func (this *Migrator) Migrate() (err error) {
 	if err := this.hooksExecutor.onBeforeRowCopy(); err != nil {
 		return err
 	}
-	go this.executeWriteFuncs()
-	go this.iterateChunks()
+	go func() {
+		if err := this.executeWriteFuncs(); err != nil {
+			this.migrationContext.Log.Errorf("Failed to execute write operation(s): %+v", err)
+		}
+	}()
+	go func() {
+		if err := this.iterateChunks(); err != nil {
+			this.migrationContext.Log.Errorf("Failed to iterate chunks: %+v", err)
+		}
+	}()
 	this.migrationContext.MarkRowCopyStartTime()
 	go this.initiateStatus()
 
@@ -444,10 +453,12 @@ func (this *Migrator) ExecOnFailureHook() (err error) {
 	return this.hooksExecutor.onFailure()
 }
 
-func (this *Migrator) handleCutOverResult(cutOverError error) (err error) {
+func (this *Migrator) handleCutOverResult(cutOverError error) error {
 	if this.migrationContext.TestOnReplica {
 		// We're merely testing, we don't want to keep this state. Rollback the renames as possible
-		this.applier.RenameTablesRollback()
+		if err := this.applier.RenameTablesRollback(); err != nil {
+			log.Errorf("Failed to rollback table renames: %+v", err)
+		}
 	}
 	if cutOverError == nil {
 		return nil
@@ -471,7 +482,7 @@ func (this *Migrator) handleCutOverResult(cutOverError error) (err error) {
 			}
 		}
 	}
-	return nil
+	return cutOverError
 }
 
 // cutOver performs the final step of migration, based on migration
@@ -488,7 +499,7 @@ func (this *Migrator) cutOver() (err error) {
 
 	this.migrationContext.MarkPointOfInterest()
 	this.migrationContext.Log.Debugf("checking for cut-over postpone")
-	this.sleepWhileTrue(
+	if err = this.sleepWhileTrue(
 		func() (bool, error) {
 			heartbeatLag := this.migrationContext.TimeSinceLastHeartbeatOnChangelog()
 			maxLagMillisecondsThrottle := time.Duration(atomic.LoadInt64(&this.migrationContext.MaxLagMillisecondsThrottleThreshold)) * time.Millisecond
@@ -516,7 +527,10 @@ func (this *Migrator) cutOver() (err error) {
 			}
 			return false, nil
 		},
-	)
+	); err != nil {
+		return err
+	}
+
 	atomic.StoreInt64(&this.migrationContext.IsPostponingCutOver, 0)
 	this.migrationContext.MarkPointOfInterest()
 	this.migrationContext.Log.Debugf("checking for cut-over postpone: complete")
@@ -549,8 +563,8 @@ func (this *Migrator) cutOver() (err error) {
 	default:
 		return this.migrationContext.Log.Fatalf("Unknown cut-over type: %d; should never get here!", this.migrationContext.CutOverType)
 	}
-	this.handleCutOverResult(err)
-	return err
+
+	return this.handleCutOverResult(err)
 }
 
 // Inject the "AllEventsUpToLockProcessed" state hint, wait for it to appear in the binary logs,
@@ -632,7 +646,9 @@ func (this *Migrator) atomicCutOver() (err error) {
 	defer func() {
 		okToUnlockTable <- true
 		dropCutOverSentryTableOnce.Do(func() {
-			this.applier.DropAtomicCutOverSentryTableIfExists()
+			if err := this.applier.DropAtomicCutOverSentryTableIfExists(); err != nil {
+				this.migrationContext.Log.Errorf("Failed to drop cut-over sentry table: %+v", err)
+			}
 		})
 	}()
 
@@ -752,8 +768,10 @@ func (this *Migrator) initiateInspector() (err error) {
 	if err := this.inspector.InspectOriginalTable(); err != nil {
 		return err
 	}
+
 	// So far so good, table is accessible and valid.
 	// Let's get master connection config
+	inspectorConnConfig := this.migrationContext.InspectorConnectionConfig
 	if this.migrationContext.AssumeMasterHostname == "" {
 		// No forced master host; detect master
 		if this.migrationContext.ApplierConnectionConfig, err = this.inspector.getMasterConnectionConfig(); err != nil {
@@ -766,7 +784,7 @@ func (this *Migrator) initiateInspector() (err error) {
 		if err != nil {
 			return err
 		}
-		this.migrationContext.ApplierConnectionConfig = this.migrationContext.InspectorConnectionConfig.DuplicateCredentials(*key)
+		this.migrationContext.ApplierConnectionConfig = inspectorConnConfig.DuplicateCredentials(*key)
 		if this.migrationContext.CliMasterUser != "" {
 			this.migrationContext.ApplierConnectionConfig.User = this.migrationContext.CliMasterUser
 		}
@@ -781,14 +799,16 @@ func (this *Migrator) initiateInspector() (err error) {
 			return fmt.Errorf("Instructed to --test-on-replica or --migrate-on-replica, but the server we connect to doesn't seem to be a replica")
 		}
 		this.migrationContext.Log.Infof("--test-on-replica or --migrate-on-replica given. Will not execute on master %+v but rather on replica %+v itself",
-			*this.migrationContext.ApplierConnectionConfig.ImpliedKey, *this.migrationContext.InspectorConnectionConfig.ImpliedKey,
+			*this.migrationContext.ApplierConnectionConfig.ImpliedKey, *inspectorConnConfig.ImpliedKey,
 		)
-		this.migrationContext.ApplierConnectionConfig = this.migrationContext.InspectorConnectionConfig.Duplicate()
+		this.migrationContext.ApplierConnectionConfig = inspectorConnConfig.Duplicate()
 		if this.migrationContext.GetThrottleControlReplicaKeys().Len() == 0 {
-			this.migrationContext.AddThrottleControlReplicaKey(this.migrationContext.InspectorConnectionConfig.Key)
+			if err = this.migrationContext.AddThrottleControlReplicaKey(inspectorConnConfig.Key); err != nil {
+				return err
+			}
 		}
 	} else if this.migrationContext.InspectorIsAlsoApplier() && !this.migrationContext.AllowedRunningOnMaster {
-		return fmt.Errorf("It seems like this migration attempt to run directly on master. Preferably it would be executed on a replica (and this reduces load from the master). To proceed please provide --allow-on-master. Inspector config=%+v, applier config=%+v", this.migrationContext.InspectorConnectionConfig, this.migrationContext.ApplierConnectionConfig)
+		return fmt.Errorf("It seems like this migration attempt to run directly on master. Preferably it would be executed on a replica (and this reduces load from the master). To proceed please provide --allow-on-master. Inspector config=%+v, applier config=%+v", inspectorConnConfig, this.migrationContext.ApplierConnectionConfig)
 	}
 	if err := this.inspector.validateLogSlaveUpdates(); err != nil {
 		return err
@@ -1030,16 +1050,21 @@ func (this *Migrator) printStatus(rule PrintStatusRule, writers ...io.Writer) {
 		state,
 		eta,
 	)
-	this.applier.WriteChangelog(
+	if _, err := this.applier.WriteChangelog(
 		fmt.Sprintf("copy iteration %d at %d", this.migrationContext.GetIteration(), time.Now().Unix()),
 		state,
-	)
+	); err != nil {
+		this.migrationContext.Log.Errorf("Failed to write to changelog: %+v", err)
+	}
+
 	w := io.MultiWriter(writers...)
 	fmt.Fprintln(w, status)
 
 	hooksStatusIntervalSec := this.migrationContext.HooksStatusIntervalSec
 	if hooksStatusIntervalSec > 0 && elapsedSeconds%hooksStatusIntervalSec == 0 {
-		this.hooksExecutor.onStatus(status)
+		if err := this.hooksExecutor.onStatus(status); err != nil {
+			this.migrationContext.Log.Errorf("Failed to run on-status hook: %+v", err)
+		}
 	}
 }
 
@@ -1049,14 +1074,16 @@ func (this *Migrator) initiateStreaming() error {
 	if err := this.eventsStreamer.InitDBConnections(); err != nil {
 		return err
 	}
-	this.eventsStreamer.AddListener(
+	if err := this.eventsStreamer.AddListener(
 		false,
 		this.migrationContext.DatabaseName,
 		this.migrationContext.GetChangelogTableName(),
 		func(dmlEvent *binlog.BinlogDMLEvent) error {
 			return this.onChangelogEvent(dmlEvent)
 		},
-	)
+	); err != nil {
+		return err
+	}
 
 	go func() {
 		this.migrationContext.Log.Debugf("Beginning streaming")
@@ -1138,7 +1165,9 @@ func (this *Migrator) initiateApplier() error {
 			return err
 		}
 	}
-	this.applier.WriteChangelogState(string(GhostTableMigrated))
+	if _, err := this.applier.WriteChangelogState(string(GhostTableMigrated)); err != nil {
+		return err
+	}
 	go this.applier.InitiateHeartbeat()
 	return nil
 }