executor: Use JSON for structured field in ADMIN SHOW DDL #52921

dveeden · 2024-04-26T09:57:07Z

What problem does this PR solve?

Issue Number: close #52920

Problem Summary:

What changed and how does it work?

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.

The DDL job information in the output of `ADMIN SHOW DDL` is now in JSON.

Test

Before

mysql> admin show ddl\G
*************************** 1. row ***************************
   SCHEMA_VER: 5055
     OWNER_ID: eef31ae6-14a2-4664-85d7-dc842ccd62e4
OWNER_ADDRESS: 192.168.1.229:4000
 RUNNING_JOBS: ID:10106, Type:add index, State:running, SchemaState:write reorganization, SchemaID:2, TableID:10104, RowCount:1447630, ArgLen:0, start time: 2024-04-26 11:20:02.291 +0200 CEST, Err:<nil>, ErrCount:0, SnapshotVersion:449347112749760513, LocalMode: false, UniqueWarnings:0
      SELF_ID: eef31ae6-14a2-4664-85d7-dc842ccd62e4
        QUERY: alter table t1 add index (c1(100))
1 row in set (0.00 sec)

After

mysql> ADMIN SHOW DDL\G
*************************** 1. row ***************************
   SCHEMA_VER: 5067
     OWNER_ID: 943efab7-b7f2-4819-9f9d-951538427aec
OWNER_ADDRESS: 192.168.1.229:4000
 RUNNING_JOBS: [{"id":10108,"type":7,"schema_id":2,"table_id":10104,"schema_name":"test","table_name":"t1","state":1,"warning":null,"err":null,"err_count":0,"row_count":0,"raw_args":[false,{"O":"c1_3","L":"c1_3"},[{"Column":{"Schema":{"O":"","L":""},"Table":{"O":"","L":""},"Name":{"O":"c1","L":"c1"}},"Length":111,"Desc":false,"Expr":null}],null,[],false],"schema_state":3,"snapshot_ver":449347534336819200,"real_start_ts":449347534330527746,"start_ts":449347534329217024,"dependency_id":0,"query":"alter table t1 add index (c1(111))","binlog":{"SchemaVersion":0,"DBInfo":null,"TableInfo":null,"FinishedTS":0,"MultipleTableInfos":null},"version":1,"reorg_meta":{"sql_mode":1436549152,"warnings":{},"warnings_count":{},"location":{"name":"Europe/Amsterdam","offset":0},"reorg_tp":3,"is_fast_reorg":true,"is_dist_reorg":true,"use_cloud_storage":false,"resource_group_name":"default","version":1,"target_scope":""},"multi_schema_info":null,"priority":1,"seq_num":0,"charset":"utf8mb4","collate":"utf8mb4_0900_ai_ci","admin_operator":0,"trace_info":{"connection_id":2097156,"session_alias":""},"bdr_role":"","cdc_write_source":0,"local_mode":false,"sql_mode":1436549152}]
      SELF_ID: 943efab7-b7f2-4819-9f9d-951538427aec
        QUERY: alter table t1 add index (c1(111))
1 row in set (0.00 sec)

mysql> SELECT JSON_PRETTY('[{"id":10108,"type":7,"schema_id":2,"table_id":10104,"schema_name":"test","table_name":"t1","state":1,"warning":null,"err":null,"err_count":0,"row_count":0,"raw_args":[false,{"O":"c1_3","L":"c1_3"},[{"Column":{"Schema":{"O":"","L":""},"Table":{"O":"","L":""},"Name":{"O":"c1","L":"c1"}},"Length":111,"Desc":false,"Expr":null}],null,[],false],"schema_state":3,"snapshot_ver":449347534336819200,"real_start_ts":449347534330527746,"start_ts":449347534329217024,"dependency_id":0,"query":"alter table t1 add index (c1(111))","binlog":{"SchemaVersion":0,"DBInfo":null,"TableInfo":null,"FinishedTS":0,"MultipleTableInfos":null},"version":1,"reorg_meta":{"sql_mode":1436549152,"warnings":{},"warnings_count":{},"location":{"name":"Europe/Amsterdam","offset":0},"reorg_tp":3,"is_fast_reorg":true,"is_dist_reorg":true,"use_cloud_storage":false,"resource_group_name":"default","version":1,"target_scope":""},"multi_schema_info":null,"priority":1,"seq_num":0,"charset":"utf8mb4","collate":"utf8mb4_0900_ai_ci","admin_operator":0,"trace_info":{"connection_id":2097156,"session_alias":""},"bdr_role":"","cdc_write_source":0,"local_mode":false,"sql_mode":1436549152}]')\G
*************************** 1. row ***************************
JSON_PRETTY('[{"id":10108,"type":7,"schema_id":2,"table_id":10104,"schema_name":"test","table_name":"t1","state":1,"warning":null,"err":null,"err_count":0,"row_count":0,"raw_args":[false,{"O":"c1_3","L":"c1_3"},[{"Column":{"Schema":{"O":"","L":""},"Table":: [
  {
    "admin_operator": 0,
    "bdr_role": "",
    "binlog": {
      "DBInfo": null,
      "FinishedTS": 0,
      "MultipleTableInfos": null,
      "SchemaVersion": 0,
      "TableInfo": null
    },
    "cdc_write_source": 0,
    "charset": "utf8mb4",
    "collate": "utf8mb4_0900_ai_ci",
    "dependency_id": 0,
    "err": null,
    "err_count": 0,
    "id": 10108,
    "local_mode": false,
    "multi_schema_info": null,
    "priority": 1,
    "query": "alter table t1 add index (c1(111))",
    "raw_args": [
      false,
      {
        "L": "c1_3",
        "O": "c1_3"
      },
      [
        {
          "Column": {
            "Name": {
              "L": "c1",
              "O": "c1"
            },
            "Schema": {
              "L": "",
              "O": ""
            },
            "Table": {
              "L": "",
              "O": ""
            }
          },
          "Desc": false,
          "Expr": null,
          "Length": 111
        }
      ],
      null,
      [],
      false
    ],
    "real_start_ts": 449347534330527746,
    "reorg_meta": {
      "is_dist_reorg": true,
      "is_fast_reorg": true,
      "location": {
        "name": "Europe/Amsterdam",
        "offset": 0
      },
      "reorg_tp": 3,
      "resource_group_name": "default",
      "sql_mode": 1436549152,
      "target_scope": "",
      "use_cloud_storage": false,
      "version": 1,
      "warnings": {},
      "warnings_count": {}
    },
    "row_count": 0,
    "schema_id": 2,
    "schema_name": "test",
    "schema_state": 3,
    "seq_num": 0,
    "snapshot_ver": 449347534336819200,
    "sql_mode": 1436549152,
    "start_ts": 449347534329217024,
    "state": 1,
    "table_id": 10104,
    "table_name": "t1",
    "trace_info": {
      "connection_id": 2097156,
      "session_alias": ""
    },
    "type": 7,
    "version": 1,
    "warning": null
  }
]
1 row in set (0.00 sec)

[
  {
    "admin_operator": 0,
    "bdr_role": "",
    "binlog": {
      "DBInfo": null,
      "FinishedTS": 0,
      "MultipleTableInfos": null,
      "SchemaVersion": 0,
      "TableInfo": null
    },
    "cdc_write_source": 0,
    "charset": "utf8mb4",
    "collate": "utf8mb4_0900_ai_ci",
    "dependency_id": 0,
    "err": null,
    "err_count": 0,
    "id": 10108,
    "local_mode": false,
    "multi_schema_info": null,
    "priority": 1,
    "query": "alter table t1 add index (c1(111))",
    "raw_args": [
      false,
      {
        "L": "c1_3",
        "O": "c1_3"
      },
      [
        {
          "Column": {
            "Name": {
              "L": "c1",
              "O": "c1"
            },
            "Schema": {
              "L": "",
              "O": ""
            },
            "Table": {
              "L": "",
              "O": ""
            }
          },
          "Desc": false,
          "Expr": null,
          "Length": 111
        }
      ],
      null,
      [],
      false
    ],
    "real_start_ts": 449347534330527746,
    "reorg_meta": {
      "is_dist_reorg": true,
      "is_fast_reorg": true,
      "location": {
        "name": "Europe/Amsterdam",
        "offset": 0
      },
      "reorg_tp": 3,
      "resource_group_name": "default",
      "sql_mode": 1436549152,
      "target_scope": "",
      "use_cloud_storage": false,
      "version": 1,
      "warnings": {},
      "warnings_count": {}
    },
    "row_count": 0,
    "schema_id": 2,
    "schema_name": "test",
    "schema_state": 3,
    "seq_num": 0,
    "snapshot_ver": 449347534336819200,
    "sql_mode": 1436549152,
    "start_ts": 449347534329217024,
    "state": 1,
    "table_id": 10104,
    "table_name": "t1",
    "trace_info": {
      "connection_id": 2097156,
      "session_alias": ""
    },
    "type": 7,
    "version": 1,
    "warning": null
  }
]

ti-chi-bot · 2024-04-26T09:57:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from dveeden, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

tiprow · 2024-04-26T09:57:23Z

Hi @dveeden. 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/test-infra repository.

dveeden · 2024-04-26T10:02:44Z

Some questions:

Do we want to filter out any fields like raw_args and query? The query is already in another column. Currently (*Job).String() has a list of columns to include.
Should we change the column definition to a JSON column instead of a VARCHAR? If we keep the VARCHAR, what about the length?
Should multiple queries get separated by a ;?

codecov · 2024-04-26T10:16:21Z

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 52.6912%. Comparing base (10971ea) to head (1f79b59).
Report is 1 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #52921         +/-   ##
=================================================
- Coverage   74.3919%   52.6912%   -21.7008%     
=================================================
  Files          1482       1588        +106     
  Lines        355545     591218     +235673     
=================================================
+ Hits         264497     311520      +47023     
- Misses        71785     256763     +184978     
- Partials      19263      22935       +3672

Flag	Coverage Δ
integration	`36.6220% <0.0000%> (?)`

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

Components	Coverage Δ
dumpling	`56.2971% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.9075% <ø> (+3.1598%)`	⬆️

ti-chi-bot · 2024-04-26T10:29:16Z

@dveeden: 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
idc-jenkins-ci-tidb/check_dev_2	`1f79b59`	link	true	`/test check-dev2`
idc-jenkins-ci-tidb/unit-test	`1f79b59`	link	true	`/test unit-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/test-infra repository. I understand the commands that are listed here.

wjhuang2016 · 2024-04-28T03:20:59Z

pkg/executor/executor.go

 			query += "\n"
 		}
 	}

+	ddlJobs, err := json.Marshal(e.ddlInfo.Jobs)


It would print many unnecessary fields.

wjhuang2016 · 2024-04-28T03:27:24Z

Some questions:

Do we want to filter out any fields like raw_args and query? The query is already in another column. Currently (*Job).String() has a list of columns to include.

Should we change the column definition to a JSON column instead of a VARCHAR? If we keep the VARCHAR, what about the length?

Should multiple queries get separated by a ;?

Some fields are not necessary and should be skipped.
Varchar is enough, the length can be 65536.
I think it's ok

executor: Use JSON for structured field in ADMIN SHOW DDL

1f79b59

ti-chi-bot bot added the release-note label Apr 26, 2024

ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 26, 2024

dveeden requested a review from cfzjywxk April 26, 2024 09:57

dveeden requested review from crazycs520 and zimulala April 26, 2024 09:58

wjhuang2016 reviewed Apr 28, 2024

View reviewed changes

pkg/executor/executor.go

query += "\n"

}

}

ddlJobs, err := json.Marshal(e.ddlInfo.Jobs)

Copy link

Member

wjhuang2016 Apr 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would print many unnecessary fields.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: Use JSON for structured field in ADMIN SHOW DDL #52921

executor: Use JSON for structured field in ADMIN SHOW DDL #52921

dveeden commented Apr 26, 2024

ti-chi-bot bot commented Apr 26, 2024

tiprow bot commented Apr 26, 2024

dveeden commented Apr 26, 2024 •

edited

codecov bot commented Apr 26, 2024 •

edited

ti-chi-bot bot commented Apr 26, 2024

wjhuang2016 Apr 28, 2024

wjhuang2016 commented Apr 28, 2024

executor: Use JSON for structured field in ADMIN SHOW DDL #52921

Are you sure you want to change the base?

executor: Use JSON for structured field in ADMIN SHOW DDL #52921

Conversation

dveeden commented Apr 26, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Test

ti-chi-bot bot commented Apr 26, 2024

tiprow bot commented Apr 26, 2024

dveeden commented Apr 26, 2024 • edited

codecov bot commented Apr 26, 2024 • edited

Codecov Report

ti-chi-bot bot commented Apr 26, 2024

wjhuang2016 Apr 28, 2024

Choose a reason for hiding this comment

wjhuang2016 commented Apr 28, 2024

dveeden commented Apr 26, 2024 •

edited

codecov bot commented Apr 26, 2024 •

edited