Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider process info implementation for statement context reuse #26161

Open
tiancaiamao opened this issue Jul 12, 2021 · 1 comment · May be fixed by #26196
Open

Reconsider process info implementation for statement context reuse #26161

tiancaiamao opened this issue Jul 12, 2021 · 1 comment · May be fixed by #26196
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@tiancaiamao
Copy link
Contributor

tiancaiamao commented Jul 12, 2021

Enhancement

To reduce object allocation in the point get, I aggressively reuse the objects.
When I try to reuse the StatementContext in #26007, I found some problem.

In the past, SetProcessInfo() get a snapshot of the process info:

	pi := util.ProcessInfo{
		ID:               s.sessionVars.ConnectionID,
		Port:             s.sessionVars.Port,
		DB:               s.sessionVars.CurrentDB,
		Command:          command,
		Plan:             p,
		PlanExplainRows:  plannercore.GetExplainRowsForPlan(p),
		RuntimeStatsColl: s.sessionVars.StmtCtx.RuntimeStatsColl,
		Time:             t,
		State:            s.Status(),
		Info:             sql,
		CurTxnStartTS:    curTxnStartTS,
		StmtCtx:          s.sessionVars.StmtCtx,
		StatsInfo:        plannercore.GetStatsInfo,
		MaxExecutionTime: maxExecutionTime,
		RedactSQL:        s.sessionVars.EnableRedactLog,
	}
	s.processInfo.Store(&pi)

This is a shallow copy, when ResetContextStmt() create objects, the pi referred objects are never touched again.
But if I reuse those object, the pi will be modified too! It's not a real snapshot any more.

So we can not reuse the old StmtCtx is the a new statement .... the pi is still referencing it.

@xhebox
Copy link
Contributor

xhebox commented Jul 12, 2021

Hmm, I think the PR could be reopened after solving the issue. My first idea is to provide a Clone for StmtCtx, and remove RuntimeStatsColl in the processinfo.

@tiancaiamao tiancaiamao linked a pull request Jul 13, 2021 that will close this issue
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants