Hey Twosee, Thanks for following up!
> checkLiveness() is not cost-free, it will also cause redundant system call overhead, In fact, it has been heavily abused. I noticed that many PHP network programming implementations do not care about the overhead of system calls, such as constantly using recv(MSG_PEEK) to check the connection before new requests, this is how checkLiveness() works. This will cause severe performance degradation because failure is always unexpected. I would assume that this overhead is negligible compared to the cost of retrying a failed query - especially for the times when a query cannot be safely retried. That being said, making the method available for folks to use doesn't force them to use it :). An application developer can weigh the tradeoffs for their use-case. > And it even can't guarantee that the next call is successful, the connection may be reset after check. > What can you do if you noticed that the connection is disconnected? phpredis checks the connection before each request and tries to reconnect because the redis connection is almost stateless. So in phpredis, it makes some sense to do so (phpredis will retry 10 times when the connection is down, but this may be bad when the server is busy). Absolutely! As I mentioned in the GitHub pull request thread, checkLiveness does not guarantee the next call is successful, but it *can* tell you if the next call will *definitely* fail without reconnecting. I think that would be a worthwhile check, considering that it costs roughly a syscall to do so. If checkLiveness returns false, your database code would simply reconnect without potentially having to throw an exception (in the case of a non-retryable query) and may end up being a better experience for a hypothetical web user. > All in all, let it fail and try again, the overall performance will be better than checking before calling it every time, so I don't think this API is necessary. > Please don't use it, it will only make your program worse. To be clear, I am not suggesting that developers call checkLiveness before *every* query, I agree that would likely be unnecessary. Where it can be useful though is for checking long-lived connections after some period of inactivity, much like how PDO checks the connection state via check_liveness before reusing a persistent connection (see https://github.com/php/php-src/blob/22982eee339c4983c87cea5d59aaf48601ad7030/ext/pdo/pdo_dbh.c#L317-L331). My PR makes it so that you can do the *very thing that PDO does*, but application side. I disagree that this makes your program worse, if so, why should PDO do the same thing? On Sat, Aug 15, 2020 at 10:38 AM twosee <tw...@qq.com> wrote: > > > 2020年8月15日 上午2:16，Eric Norris <eric.t.nor...@gmail.com> 写道： > > > > Hey internals, > > > > I've submitted https://github.com/php/php-src/pull/5935 as a way to > expose > > an underlying PDO driver's check_liveness function to userland scripts. > > Often advice given on the internet is to issue a no-op statement (e.g. > > SELECT 1 FROM dual), but this adds potentially unnecessary overhead and > > latency. Using the PDO driver's check_liveness function offers a > lower-cost > > way of ensuring the connection is still alive. > > > > As noted in the PR, I am not tied to the method name, and open to any > > suggestions to making the PR better - I'm mostly interested in the > > underlying concept. > > > > It appears the test I added is currently failing for pgsql. I didn't > have a > > test setup for that on hand but will look into it if there is positive > > feedback for this PR. > > > > Relatedly, I've also submitted https://github.com/php/php-src/pull/5947 > as > > a way to potentially improve the mysqlnd driver's check_liveness > function. > > > > Thanks! > > > > Eric Norris > > Hi, I am the author of Swoole (https://github.com/swoole/swoole-src), I > have a little network programming experience, the following is my personal > opinion: > > I suggest not to check it, let it throw an exception (PHP8 has used > exceptions as the default error reporting way). > > checkLiveness() is not cost-free, it will also cause redundant system call > overhead, In fact, it has been heavily abused. I noticed that many PHP > network programming implementations do not care about the overhead of > system calls, such as constantly using recv(MSG_PEEK) to check the > connection before new requests, this is how checkLiveness() works. This > will cause severe performance degradation because failure is always > unexpected. > > And it even can't guarantee that the next call is successful, the > connection may be reset after check. > > What can you do if you noticed that the connection is disconnected? > phpredis checks the connection before each request and tries to reconnect > because the redis connection is almost stateless. So in phpredis, it makes > some sense to do so (phpredis will retry 10 times when the connection is > down, but this may be bad when the server is busy). > > But MySQL can't do this, the context of it is too complex, let it throw an > exception, catch it, and start from the beginning again is the best and > cheapest way to solve it. > > All in all, let it fail and try again, the overall performance will be > better than checking before calling it every time, so I don't think this > API is necessary. > > Please don't use it, it will only make your program worse. > > Regards, > Twosee > >
