Merge branch 'master' into mx/fixIssue30352

sessionctx/variable/sysvar_test.go

@@ -674,6 +674,92 @@ func TestSettersandGetters(t *testing.T) {
 	}
 }
 
+// TestSkipInitIsUsed ensures that no new variables are added with skipInit: true.
+// This feature is deprecated, and if you need to run code to differentiate between init and "SET" (rare),
+// you can instead check if s.StmtCtx.StmtType == "Set".
+// The reason it is deprecated is that the behavior is typically wrong:
+// it means session settings won't inherit from global and don't apply until you first set
+// them in each session. This is a very weird behavior.
+// See: https://github.com/pingcap/tidb/issues/35051
+func TestSkipInitIsUsed(t *testing.T) {
+	for _, sv := range GetSysVars() {
+		if sv.skipInit {
+			// Many of these variables might allow skipInit to be removed,
+			// they need to be checked first. The purpose of this test is to make
+			// sure we don't introduce any new variables with skipInit, which seems
+			// to be a problem.
+			switch sv.Name {
+			case Timestamp,
+				WarningCount,
+				ErrorCount,
+				LastInsertID,
+				Identity,
+				TiDBTxnScope,
+				TiDBSnapshot,
+				TiDBOptDistinctAggPushDown,
+				TiDBOptWriteRowID,
+				TiDBChecksumTableConcurrency,
+				TiDBBatchInsert,
+				TiDBBatchDelete,
+				TiDBBatchCommit,
+				TiDBCurrentTS,
+				TiDBLastTxnInfo,
+				TiDBLastQueryInfo,
+				TiDBEnableChunkRPC,
+				TxnIsolationOneShot,
+				TiDBOptimizerSelectivityLevel,
+				TiDBOptimizerEnableOuterJoinReorder,
+				TiDBLogFileMaxDays,
+				TiDBConfig,
+				TiDBDDLReorgPriority,
+				TiDBSlowQueryFile,
+				TiDBWaitSplitRegionFinish,
+				TiDBWaitSplitRegionTimeout,
+				TiDBLowResolutionTSO,
+				TiDBAllowRemoveAutoInc,
+				TiDBMetricSchemaStep,
+				TiDBMetricSchemaRangeDuration,
+				TiDBFoundInPlanCache,
+				TiDBFoundInBinding,
+				RandSeed1,
+				RandSeed2,
+				TiDBLastDDLInfo,
+				TiDBGeneralLog,
+				TiDBSlowLogThreshold,
+				TiDBRecordPlanInSlowLog,
+				TiDBEnableSlowLog,
+				TiDBCheckMb4ValueInUTF8,
+				TiDBPProfSQLCPU,
+				TiDBDDLSlowOprThreshold,
+				TiDBForcePriority,
+				TiDBMemoryUsageAlarmRatio,
+				TiDBEnableCollectExecutionInfo,
+				TiDBPersistAnalyzeOptions,
+				TiDBEnableColumnTracking,
+				TiDBStatsLoadPseudoTimeout,
+				SQLLogBin,
+				ForeignKeyChecks,
+				CollationDatabase,
+				CharacterSetClient,
+				CharacterSetResults,
+				CollationConnection,
+				CharsetDatabase,
+				GroupConcatMaxLen,
+				CharacterSetConnection,
+				CharacterSetServer,
+				TiDBBuildStatsConcurrency,
+				TiDBOptTiFlashConcurrencyFactor,
+				TiDBOptSeekFactor,
+				TiDBOptJoinReorderThreshold,
+				TiDBStatsLoadSyncWait,
+				CharacterSetFilesystem:
+				continue
+			}
+			require.Equal(t, false, sv.skipInit, fmt.Sprintf("skipInit should not be set on new system variables. variable %s is in violation", sv.Name))
+		}
+	}
+}
+
 func TestSecureAuth(t *testing.T) {
 	sv := GetSysVar(SecureAuth)
 	vars := NewSessionVars()

store/helper/helper.go

@@ -840,42 +840,46 @@ func (h *Helper) requestPD(apiName, method, uri string, body io.Reader, res inte
 	if len(pdHosts) == 0 {
 		return errors.New("pd unavailable")
 	}
-	logutil.BgLogger().Debug("RequestPD URL", zap.String("url", util.InternalHTTPSchema()+"://"+pdHosts[0]+uri))
-	req := new(http.Request)
 	for _, host := range pdHosts {
-		req, err = http.NewRequest(method, util.InternalHTTPSchema()+"://"+host+uri, body)
-		if err != nil {
-			// Try to request from another PD node when some nodes may down.
-			if strings.Contains(err.Error(), "connection refused") {
-				continue
-			}
-			return errors.Trace(err)
+		err = requestPDForOneHost(host, apiName, method, uri, body, res)
+		if err == nil {
+			break
 		}
+		// Try to request from another PD node when some nodes may down.
 	}
+	return err
+}
+
+func requestPDForOneHost(host, apiName, method, uri string, body io.Reader, res interface{}) error {
+	urlVar := fmt.Sprintf("%s://%s%s", util.InternalHTTPSchema(), host, uri)
+	logutil.BgLogger().Debug("RequestPD URL", zap.String("url", urlVar))
+	req, err := http.NewRequest(method, urlVar, body)
 	if err != nil {
-		return err
+		logutil.BgLogger().Warn("requestPDForOneHost new request failed",
+			zap.String("url", urlVar), zap.Error(err))
+		return errors.Trace(err)
 	}
 	start := time.Now()
 	resp, err := util.InternalHTTPClient().Do(req)
 	if err != nil {
 		metrics.PDAPIRequestCounter.WithLabelValues(apiName, "network error").Inc()
+		logutil.BgLogger().Warn("requestPDForOneHost do request failed",
+			zap.String("url", urlVar), zap.Error(err))
 		return errors.Trace(err)
 	}
 	metrics.PDAPIExecutionHistogram.WithLabelValues(apiName).Observe(time.Since(start).Seconds())
 	metrics.PDAPIRequestCounter.WithLabelValues(apiName, resp.Status).Inc()
-
 	defer func() {
 		err = resp.Body.Close()
 		if err != nil {
-			logutil.BgLogger().Error("close body failed", zap.Error(err))
+			logutil.BgLogger().Warn("requestPDForOneHost close body failed",
+				zap.String("url", urlVar), zap.Error(err))
 		}
 	}()
-
 	err = json.NewDecoder(resp.Body).Decode(res)
 	if err != nil {
 		return errors.Trace(err)
 	}
-
 	return nil
 }