server, sessionctx: Support for the status command of MySQL Shell

[dveeden]

What problem does this PR solve?

Fixes #8841

Problem Summary:

When running MySQL Shell (mysqlsh mysql://root:@127.0.0.1:4000 --sql) the \status (aka \s) doesn't work.

Example:

 MySQL  127.0.0.1:4000  SQL > \status
MySQL Shell version 8.0.23

get_int(6): field type is String

What is changed and how it works?

What's Changed:

The type of @@port was changed from a string type to an integer type.

This can be checked by running mysqlsh mysql://root:@127.0.0.1:4000 --sql --column-type-info and then running select @@port. The change is from VAR_STRING to LONGLONG.

This will also affect the type of other unsigned system variables.

In addition to this COM_STATISTICS has been implemented to return
Uptime: 0 Threads: 0 Questions: 0 Slow queries: 0 Opens: 0 Flush tables: 0 Open tables: 0 Queries per second avg: 0.000
Note that all values are always zero. Also note that mysqlsh parses out the integer for the Uptime.

This can also be checked with mysqladmin status -u root -P 4000 -h 127.0.0.1 -p.

How it Works:

1. GetNativeValType now has a case or TypeUnsigned. This corrects the data type for variables with TypeUnsigned.
2. dispatch now has a case for mysql.ComStatistics.
3. The Port global variable now has Type: TypeUnsigned
4. writeStats now has a very basic implementation of COM_STATISTICS

Check List

Tests

• Manual test (add detailed scripts or steps below)

Side effects

Release note

• Support for COM_STATISTICS was added
• The type of unsigned global variables has been corrected

[CLAassistant]

All committers have signed the CLA.

[dveeden]

Looks like CI detected that values larger than LLONG_MAX (9223372036854775807) are not handled correctly. I'll have a look at this later.

[morgo]

I assume clients will be okay if NULL is mentioned instead of some of the values here? I'm not worried about lying on Opens/Open tables, but it does seem possible that on QPS and Uptime it could cause problems.

[dveeden]

I could modify it like this:

$ git diff -- server/conn.go util/sys/linux/sys_linux.go
diff --git a/server/conn.go b/server/conn.go
index 698571dfd..0f1c50631 100644
--- a/server/conn.go
+++ b/server/conn.go
@@ -81,6 +81,7 @@ import (
        "github.com/pingcap/tidb/util/logutil"
        "github.com/pingcap/tidb/util/memory"
        "github.com/pingcap/tidb/util/sqlexec"
+       "github.com/pingcap/tidb/util/sys/linux"
        "github.com/prometheus/client_golang/prometheus"
        "go.uber.org/zap"
 )
@@ -1069,7 +1070,11 @@ func (cc *clientConn) dispatch(ctx context.Context, data []byte) error {
 }
 
 func (cc *clientConn) writeStats(ctx context.Context) error {
-       msg := []byte("Uptime: 0  Threads: 0  Questions: 0  Slow queries: 0  Opens: 0  Flush tables: 0  Open tables: 0  Queries per second avg: 0.000")
+       // Note that `mysqlsh` (MySQL Shell) parses out the `Uptime` when running `\status`.
+       // for this to work this needs to be an integer with one space before and two spaces
+       // after it.
+       stat := fmt.Sprintf("Uptime: %d  ", linux.Uptime())
+       msg := []byte(stat)
        data := cc.alloc.AllocWithLen(4, len(msg))
        data = append(data, msg...)
 
diff --git a/util/sys/linux/sys_linux.go b/util/sys/linux/sys_linux.go
index 976815956..f361eb3f9 100644
--- a/util/sys/linux/sys_linux.go
+++ b/util/sys/linux/sys_linux.go
@@ -52,3 +52,9 @@ func SetAffinity(cpus []int) error {
        }
        return unix.SchedSetaffinity(unix.Getpid(), &cpuSet)
 }
+
+func Uptime() int64 {
+       si := &unix.Sysinfo_t{}
+       unix.Sysinfo(si)
+       return si.Uptime
+}

This would return the system (not TiDB) uptime of the host running this process. This might not be the best thing, but it is more informative than always returning 0. Removing the other values seem to work just fine, at least for mysqlsh.

This seems to be slightly related to #8842 as that should probably return the same value for consistency.

[morgo]

Yeah, I prefer this - but lets see what other reviewers prefer too. I commented in #8842 that it is possible to retrieve the correct uptime from prometheus, but because it is an optional component it might not be a good fit here.

[morgo]

Looks like CI detected that values larger than LLONG_MAX (9223372036854775807) are not handled correctly. I'll have a look at this later.

This looks to be caused by this cast:
https://github.com/pingcap/tidb/blob/master/types/datum.go#L153

Internally i needs to be an int64, but I'm not sure why the precision is just lost here. I appologize in advance that reviews will be a little bit slow, as it's now Spring Festival in China :-) Another option you have is that you could set Port to TypeInt. This is not strictly MySQL compatible of course.

[dveeden]

Looks like CI detected that values larger than LLONG_MAX (9223372036854775807) are not handled correctly. I'll have a look at this later.

This looks to be caused by this cast:
https://github.com/pingcap/tidb/blob/master/types/datum.go#L153

Internally i needs to be an int64, but I'm not sure why the precision is just lost here. I appologize in advance that reviews will be a little bit slow, as it's now Spring Festival in China :-) Another option you have is that you could set Port to TypeInt. This is not strictly MySQL compatible of course.

It looks like the issue is that the mysql.UnsignedFlag is not set for this field in the response while it should. I've modified GetNativeValType to return this flag for TypeUnsigned. Then rewriteVariable passes this on to DatumToConstant which sets this on the Constant it returns.

[morgo]

LGTM

[bb7133]

PTAL @jackysp , thanks!

[jackysp]

LGTM

[jackysp]

/merge

[ti-srebot]

/run-all-tests

[ti-srebot]

@dveeden merge failed.

[morgo]

/merge

[ti-chi-bot]

@morgo: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

[ti-chi-bot]

@morgo: /merge in this pull request requires 3 /lgtm.

In response to this:

/merge

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 ti-community-infra/tichi repository.

[zz-jason]

@morgo: /merge in this pull request requires 3 /lgtm.

In response to this:

/merge

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 ti-community-infra/tichi repository.

@hi-rustin I think we only need 2 LGTMs to merge this PR. Is it caused by the incorrect config?

[morgo]

/run-all-tests

[morgo]

/merge

[ti-chi-bot]

@morgo: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: 6553213

[ti-chi-bot]

@dveeden: Your PR has out-of-dated, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

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 ti-community-infra/tichi repository.

[bb7133]

Thanks for your contribution! @dveeden