infoschema: add affected_rows to the processlist table #54488

ekexium · 2024-07-08T06:37:05Z

What problem does this PR solve?

Issue Number: close #54486

Problem Summary:

We need a straightforward way to know the progress of long-running statements.

What changed and how does it work?

Add a column affected_rows to information_schema.processlist

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Add a column `CURRENT_AFFECTED_ROWS` to `information_schema.processlist`.

tiprow · 2024-07-08T06:37:22Z

Hi @ekexium. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2024-07-08T06:56:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.4427%. Comparing base (08147e7) to head (c093336).
Report is 115 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54488         +/-   ##
=================================================
- Coverage   72.8476%   57.4427%   -15.4049%     
=================================================
  Files          1542       1682        +140     
  Lines        436136     629851     +193715     
=================================================
+ Hits         317715     361804      +44089     
- Misses        98837     244656     +145819     
- Partials      19584      23391       +3807

Flag	Coverage Δ
integration	`38.4056% <100.0000%> (?)`
unit	`72.8142% <100.0000%> (+0.9463%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.0785% <ø> (+6.1943%)`	⬆️

Signed-off-by: ekexium <eke@fastmail.com>

cfzjywxk · 2024-07-09T09:43:39Z

pkg/infoschema/tables.go

@@ -854,6 +854,7 @@ var tableProcesslistCols = []columnInfo{
 	{name: "TxnStart", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag, deflt: ""},
 	{name: "RESOURCE_GROUP", tp: mysql.TypeVarchar, size: resourcegroup.MaxGroupNameLength, flag: mysql.NotNullFlag, deflt: ""},
 	{name: "SESSION_ALIAS", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag, deflt: ""},
+	{name: "AFFECTED_ROWS", tp: mysql.TypeLonglong, size: 21, flag: mysql.UnsignedFlag},


Is there compatibility risks for tools like dashboard?

I suppose they should not depend on the number of the columns...
@lcwangchao have you met any compatibility problems when adding SESSION_ALIAS?

I think it's fine. The added row is in the last position. @bb7133 Could you take a look?

I believe it is fine for compatibility. However, my concern is about the semantics when it is called 'affected rows': all columns in PROCESSLIST are the 'current status' but AFFECTED_ROWS means the affected rows by the last statement, which could be confusing.

For example, after executing INSERT INTO .., we have:

ID: 2097158 USER: root HOST: 127.0.0.1:50816 DB: test COMMAND: Sleep TIME: 16 STATE: autocommit INFO: NULL DIGEST: 04fa858fa491c62d194faec2ab427261cc7998b3f1ccf8f6844febca504cb5e9 MEM: 4096 DISK: 0 TxnStart: RESOURCE_GROUP: default SESSION_ALIAS: AFFECTED_ROWS: 2 2 rows in set (0.01 sec)

How about displaying 0 when INFO is null or len(INFO) == 0?

I'm not sure, to be honest. Maybe we could also consider renaming it 'AFFECTED_ROWS_LAST_STATEMENT'?

What is the expected result of this column for pipelined DMLs?

It's introduced to reveal the current progress of the statement during its execution, as a pipelined DML may run for a long time. So 'AFFECTED_ROWS_LAST_STATEMENT' is not a good idea I suppose.

Or just rename it "CURRENT_AFFECTED_ROWS" to distinguish them and make it clear.

Sounds good

pkg/util/processinfo.go

lcwangchao · 2024-07-16T09:25:08Z

pkg/infoschema/tables.go

@@ -854,6 +854,7 @@ var tableProcesslistCols = []columnInfo{
 	{name: "TxnStart", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag, deflt: ""},
 	{name: "RESOURCE_GROUP", tp: mysql.TypeVarchar, size: resourcegroup.MaxGroupNameLength, flag: mysql.NotNullFlag, deflt: ""},
 	{name: "SESSION_ALIAS", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag, deflt: ""},
+	{name: "AFFECTED_ROWS", tp: mysql.TypeLonglong, size: 21, flag: mysql.UnsignedFlag},


I think it's fine. The added row is in the last position. @bb7133 Could you take a look?

Signed-off-by: ekexium <eke@fastmail.com>

ekexium · 2024-07-18T03:29:30Z

/retest

tiprow · 2024-07-18T03:29:53Z

@ekexium: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot · 2024-07-23T02:59:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, tangenta

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfzjywxk,tangenta]
~~pkg/infoschema/OWNERS~~ [tangenta]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-07-23T02:59:24Z

[LGTM Timeline notifier]

Timeline:

2024-07-18 03:27:36.887129458 +0000 UTC m=+497278.878070919: ☑️ agreed by cfzjywxk.
2024-07-23 02:59:23.185322321 +0000 UTC m=+927585.176263789: ☑️ agreed by tangenta.

tiprow · 2024-07-23T03:01:02Z

@ekexium: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`c093336`	link	true	`/test fast_test_tiprow`
tidb_parser_test	`c093336`	link	true	`/test tidb_parser_test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

ekexium · 2024-07-23T03:03:02Z

/retest

tiprow · 2024-07-23T03:03:24Z

@ekexium: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot bot added release-note size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2024

ekexium force-pushed the feat-processlist-affected-rows branch from 5311adf to d98e730 Compare July 8, 2024 06:51

ekexium force-pushed the feat-processlist-affected-rows branch 2 times, most recently from 2772c86 to 43286ac Compare July 8, 2024 07:28

feat: add affected_rows to processlist

52b509b

Signed-off-by: ekexium <eke@fastmail.com>

ekexium force-pushed the feat-processlist-affected-rows branch from 43286ac to 52b509b Compare July 8, 2024 08:54

ekexium requested review from lcwangchao, you06 and cfzjywxk July 8, 2024 09:37

cfzjywxk reviewed Jul 9, 2024

View reviewed changes

lcwangchao reviewed Jul 16, 2024

View reviewed changes

ekexium added 2 commits July 16, 2024 18:13

refactor: remove unnecessary code

c403a97

Signed-off-by: ekexium <eke@fastmail.com>

fix: rename to CURRENT_AFFECTED_ROWS

c093336

Signed-off-by: ekexium <eke@fastmail.com>

ekexium force-pushed the feat-processlist-affected-rows branch from 4f535ba to c093336 Compare July 18, 2024 02:33

cfzjywxk approved these changes Jul 18, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label Jul 18, 2024

ekexium requested review from tangenta and bb7133 July 23, 2024 02:36

tangenta approved these changes Jul 23, 2024

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm labels Jul 23, 2024

ti-chi-bot bot merged commit 15cc6e8 into pingcap:master Jul 23, 2024
21 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

infoschema: add affected_rows to the processlist table #54488

infoschema: add affected_rows to the processlist table #54488

ekexium commented Jul 8, 2024 •

edited

Loading

tiprow bot commented Jul 8, 2024

codecov bot commented Jul 8, 2024 •

edited

Loading

cfzjywxk Jul 9, 2024

ekexium Jul 9, 2024

lcwangchao Jul 16, 2024

bb7133 Jul 17, 2024 •

edited

Loading

ekexium Jul 17, 2024

bb7133 Jul 17, 2024

ekexium Jul 17, 2024

cfzjywxk Jul 17, 2024

ekexium Jul 18, 2024

lcwangchao Jul 16, 2024

ekexium commented Jul 18, 2024

tiprow bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 23, 2024

ti-chi-bot bot commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

ekexium commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

infoschema: add affected_rows to the processlist table #54488

infoschema: add affected_rows to the processlist table #54488

Conversation

ekexium commented Jul 8, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 8, 2024

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bb7133 Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekexium commented Jul 18, 2024

tiprow bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 23, 2024

ti-chi-bot bot commented Jul 23, 2024

[LGTM Timeline notifier]

tiprow bot commented Jul 23, 2024

ekexium commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

ekexium commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

bb7133 Jul 17, 2024 •

edited

Loading