Implement CMD_RESET_CONNECTION

[testn]

Address #1328

still in the draft...

To add integration tests

[sidorares]

I think mysqljs/mysql has CMD_RESET_CONNECTION implemented ( not sure if .reset() is exposed in the public api ) and it's added automatically to a pool connections before releasing them back. IMO it's a useful feature, maybe worth discussing compatibility and perf impact if we make it on by default

[testn]

I think mysqljs/mysql has CMD_RESET_CONNECTION implemented ( not sure if .reset() is exposed in the public api ) and it's added automatically to a pool connections before releasing them back. IMO it's a useful feature, maybe worth discussing compatibility and perf impact if we make it on by default

I have thought of that too. This will help make pooled connection works just like a typical connection including reset local temp table.

[sidorares]

looks like it's not merged yet - mysqljs/mysql#1469
it might be implemented with "change user" command wich re-authenticates and resets connection without re-establishing tcp connection.

[sidorares]

ok changeUser is sent only when connection did change user on itself and its different from what's configured on the pool - https://github.com/mysqljs/mysql/blob/3430c513d6b0ca279fb7c79b210c9301e9315658/lib/Pool.js#L116

to me that should be done on release in the background. Not sure what's mysql2 behaviour in that case ( get connection, change user, release connection )

[testn]

I think that we should add an option to turn it on/off defaulting to on?

[sidorares]

agree. default on is a sensible security / "no surprises" option while allowing explicit "off" to gain a bit of performance.

[sidorares]

I wonder if it's possible to have "auto" mode - if no change to variables / affected rows / user / db don't do reset

[testn]

I wonder if it's possible to have "auto" mode - if no change to variables / affected rows / user / db don't do reset

I think in general, reset should take that into account already. It would just add another roundtrip cost.

[sidorares]

hey @testn I might have some time this weekend to clear backlog of PRs - do you need any help with integration tests for this?

[testn]

hey @testn I might have some time this weekend to clear backlog of PRs - do you need any help with integration tests for this?

Awesome. I still cannot figure out why it failed in some CI setting.

Also, connection.end() is not "async". However, to end() the connection, I would like to make a reset() call which is async network call. What should we do with it?

[danielgindi]

I just stumbled upon a bug caused by this. I was wondering "do they reset the connection?", and the answer is apparently "no".
Now discussing a "no surprises" methodology should always result to a "default: true". As no surprises mean: I expect a clean connection from the pool. How the heck should I know what is currently pending on a random connection I just got from the pool?
Anyone expecting the opposite, is working in some extreme case where there is zero session and wants to gain 1-2% more in perf.

[sidorares]

@danielgindi 100% agree ( though would be great to have some tests to support 1-2% perf hit assumption ). Any preference on the name for this flag? resetOnAcquire ( default true ) ?

[danielgindi]

@danielgindi 100% agree ( though would be great to have some tests to support 1-2% perf hit assumption ). Any preference on the name for this flag? resetOnAcquire ( default true ) ?

As for the naming - this should just be "connectionReset". Other connectors do this too: https://mysqlconnector.net/connection-options/.
I'd like to have a connectionResetOnRelease option, to automatically avoid deadlocks (if using GET_LOCK etc.), but this could introduce an additional overhead for connections that will not be used again, and should be opt-in.

As for the perf hit - I tested about a year ago with the .net connector, with and without the reset. I believe it's safe to assume it would be the same here. Anyway it's a non-issue, as you never over-optimize at safety's expense.

[danielgindi]

@sidorares Note that in some cases, the perf hit may be larger. i.e:

1. When the roundtrip (ping mysql_server) is too big. Because in this case the code has to wait for an additional roundtrip until it can start querying.
2. When there's a pending transaction that will be rolled back by the reset connection command, and the rollback is a large set of rows and takes some time.

In these cases, it may even be better to always reset the connection when returning to the pool.
And then we can just mark it as "not ready yet for acquiring" until the reset has completed.

You can see some discussion here: mysql-net/MySqlConnector#178

[sidorares]

I'd like to have a connectionResetOnRelease option, to automatically avoid deadlocks (if using GET_LOCK etc.), but this could introduce an additional overhead for connections that will not be used again, and should be opt-in.

hm, doing reset on connection release might actually be better for performance ( or rather consumer latency ) as in this case there is a chance that reset is happening while no one is waiting

[sidorares]

I believe this need to perform some additional client cleanup, for example prepared statements need to be discarded.

[testn]

ok let me take a look

[testn]

Preparedstatement is discarded automatically
https://mariadb.com/kb/en/com_reset_connection/

[sidorares]

yes, server side, but we need to delete statements cache otherwise it might be used incorrectly with execute() after reset

I think this is what needs to be done

1. add private connection helper method connection._initialiseStatementCache({ disableDispose: boolean })

something along the lines:

_initialiseStatementCache({ disableDispose }) {
  if (this._statements) {
    if (disableDispose) {
       this._statements.dispose = null;
    }
    this._statements.clear();
  }
  this._statements = new LRU({
    max: this.config.maxPreparedStatements,
    dispose: function(key, statement) {
      statement.close();
    }
  })
}

2. replace line 66 in connection.js with this._initialiseStatementCache({ disableDispose: false });

3. in ResetConnectionCommand.resetConnectionResponse(packet, connection) add call connection._initialiseStatementCache({ disableDispose: true }) at the beginning before calling onResult()

[testn]

Ah... got it

[testn]

I thought statement can be reused even though the connection is reset

[sidorares]

I need to double check but I'm pretty sure statement id is sequential 1, 2, 3, ... for each prepare() command ( can't remember if 0 or 1 based ) and after reset it goes again as 1, 2, 3, ...

[sidorares]

small correction: set dispose to noop instead of null, e.i this._statements.dispose = () => {};

[testn]

gotcha

[danielgindi]

@testn I believe that releaseConnection should look like this:

  releaseConnection(connection, reset = true) {
    let cb;
    if (!connection._pool) {
      // The connection has been removed from the pool and is no longer good.
      if (this._connectionQueue.length) {
        cb = this._connectionQueue.shift();
        process.nextTick(this.getConnection.bind(this, cb));
      }
    } else {
      if (reset) {
        connection.reset(() => {
          this.releaseConnection(connection, false);
        });
      } else {
        if (this._connectionQueue.length) {
          cb = this._connectionQueue.shift();
          process.nextTick(cb.bind(null, null, connection));
        } else {
          this._freeConnections.push(connection);
          this.emit('release', connection);
        }
      }
    }
  }

[danielgindi]

We've tried this in a production environment, after modifying the releaseConnection function -> it looks like problem solved!

[danielgindi]

@testn I believe that releaseConnection should look like this:

  releaseConnection(connection, reset = true) {

    let cb;

    if (!connection._pool) {

      // The connection has been removed from the pool and is no longer good.

      if (this._connectionQueue.length) {

        cb = this._connectionQueue.shift();

        process.nextTick(this.getConnection.bind(this, cb));

      }

    } else {

      if (reset) {

        connection.reset(() => {

          this.releaseConnection(connection, false);

        });

      } else {

        if (this._connectionQueue.length) {

          cb = this._connectionQueue.shift();

          process.nextTick(cb.bind(null, null, connection));

        } else {

          this._freeConnections.push(connection);

          this.emit('release', connection);

        }

      }

    }

  }

@testn have you taken a look at this? This is to actually trigger the reset.

[testn]

@testn I believe that releaseConnection should look like this:

  releaseConnection(connection, reset = true) {

    let cb;

    if (!connection._pool) {

      // The connection has been removed from the pool and is no longer good.

      if (this._connectionQueue.length) {

        cb = this._connectionQueue.shift();

        process.nextTick(this.getConnection.bind(this, cb));

      }

    } else {

      if (reset) {

        connection.reset(() => {

          this.releaseConnection(connection, false);

        });

      } else {

        if (this._connectionQueue.length) {

          cb = this._connectionQueue.shift();

          process.nextTick(cb.bind(null, null, connection));

        } else {

          this._freeConnections.push(connection);

          this.emit('release', connection);

        }

      }

    }

  }

@testn have you taken a look at this? This is to actually trigger the reset.

Can you explain a little bit?

[danielgindi]

@testn we want the connector to reset a connection in the background before making it available again.
So what I did there is added a flag, that makes it reset the connection, and then call the logic again to return to the pool.
It could of course be split into two functions instead, like releaseConnection and _releaseConnectionImpl.

Now if there was an error, even in the reset command, then it will have its _pool prop removed, which will make the function release the connection without returning to the pool.

We can make this flag based, but this should be the default behavior anyway.

[mscdex]

What's left to get this over the finish line?