Support for 'RESTART' statement

[dveeden]

Enhancement

MySQL 8.0 has a RESTART statement. https://github.com/pingcap/tidb/issues/7968 identified this is one of the things TiDB needs for improved MySQL 8.0 compatibility.

The parser part seems easy: https://github.com/pingcap/parser/pull/1267

The documentation in MySQL for this command are quite detailed.

We could do something similar in TiDB:

• In tiup
  • Add Environment=TIDB_PARENT_PID=1 to the systemd service file
  • Check if Restart=on-failure is already set
• In tidb-server:
  • if TIDB_IS_SUPERVISED is not defined: RESTART returns an error (MySQL has MYSQLD_PARENT_PID)
  • if TIDB_IS_SUPERVISED is defined: RESTART shuts down the server with exit code 16

When MySQL Server is running without systemd, for example with dbdeployer it uses old mysqld_safe to implement the right logic. As tidb-server doesn't have such an 'angel' script this would result this to not work outside of systemd, which is probably fine as the server can still be used, just not with the RESTART statement.

All this relies on OS specific things, this means that Windows would need a specific implementation.

It might be possible to implement this differently by:

• executing /proc/<pid>/exe (What if the binary is updated?)
• using SO_REUSEPORT etc
• Starting the server again before where it would exit from main()

1. Is relying on systemd ok? or should we add a wrapper script like mysqld_safe?
2. Do we need Windows support?
3. Should we try a different way of implementing this than the exit status?

[dveeden]

cc @morgo

[morgo]

I think wrapper scripts like mysqld_safe are dangerous. If it's needed, it's much better to rely on systemd.

There is another option, which is to not rely on the OS at all. The server could close all its ports, and then go through the startup loop again without changing the PID.

There are some risks to this approach though. If the server had rogue handles to temporary files open they wouldn't be released for example (I think this is more an issue for MySQL). Memory leaks also wouldn't be guaranteed free.

But one important difference is that the config file could be reparsed.

[dveeden]

I think wrapper scripts like mysqld_safe are dangerous. If it's needed, it's much better to rely on systemd.

• Yes and relying on systemd is what MySQL does if you install the RPMs etc.
• However for MySQL the process is easier as the server and systemd service file are part of the same RPM.

There is another option, which is to not rely on the OS at all. The server could close all its ports, and then go through the startup loop again without changing the PID.

Yes like #26052 ?

There are some risks to this approach though. If the server had rogue handles to temporary files open they wouldn't be released for example (I think this is more an issue for MySQL). Memory leaks also wouldn't be guaranteed free.

Another possible issue is that this wouldn't help when restarting as part of doing upgrades.

But one important difference is that the config file could be reparsed.

In MySQL the main purpose of restart is to be able to do SET PERSIST_ONLY some_non_dynamic_var = "foobar"; RESTART which for TiDB doesn't really make sense. In which cases would it make sense to restart a tidb-server process? does this help in that case?

[dveeden]

Another set of options would be:

• Not implementing this in TiDB (as SET PERSIST... is not applicable) and tiup and TiDB Operator are already handling rolling restarts.
• Implementing this as a noop
• Having this trigger a full cluster rolling restart. (I don't really like this as that means this works differently than the command in MySQL)

[morgo]

I like the implementation in #26052, I think it is useful actually. We can review it from there when you are ready.

[dveeden]

I like the implementation in #26052, I think it is useful actually. We can review it from there when you are ready.

Who would be good reviewers on this? I'd like to make sure that this doesn't leave things half-initialized etc. and I don't know if there are other ways of doing this in Go that might be better.

[morgo]

I like the implementation in #26052, I think it is useful actually. We can review it from there when you are ready.

Who would be good reviewers on this? I'd like to make sure that this doesn't leave things half-initialized etc. and I don't know if there are other ways of doing this in Go that might be better.

Maybe @tiancaiamao and @lysu ?