Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PDO::checkLiveness method #5935

Closed
wants to merge 2 commits into from

Conversation

ericnorris
Copy link
Contributor

@ericnorris ericnorris commented Aug 4, 2020

There are a number of questions and resources (example 1, example 2, example 3) that seek to check if a PDO connection is truly alive. From what I can tell, these all end up with people issuing pseudo "ping" statements (e.g. SELECT 1 FROM dual, currently the case at my employer), which results in potentially unnecessary overhead and latency.

This PR proposes adding a new function to PDO in order to expose the already existing pdo_dbh_check_liveness_func to userspace PHP programs that may want to check connection "liveness" manually.

Example:

<?php

$pdo = new PDO("mysql:host=127.0.0.1;port=3306;dbname=test-database", "root", "my-secret-pw");

$query = $pdo->query("SELECT 'hello world' AS _msg FROM dual");

print_r($query->fetch(PDO::FETCH_ASSOC));

echo "checkLiveness: " . var_export($pdo->checkLiveness(), true) . "\n";
echo "sleep(3)\n";

sleep(3);

echo "checkLiveness: " . var_export($pdo->checkLiveness(), true) . "\n";

$query = $pdo->query("SELECT 'hello world' AS _msg FROM dual");

print_r($query->fetch(PDO::FETCH_ASSOC));

Results in:

$ ./sapi/cli/php test.php
Array
(
    [_msg] => hello world
)
checkLiveness: true
sleep(3)
checkLiveness: false

Fatal error: Uncaught PDOException: SQLSTATE[HY000]: General error: 2006 MySQL server has gone away in /home/enorris/workspace/php-src/test.php:16
Stack trace:
#0 /home/enorris/workspace/php-src/test.php(16): PDO->query('SELECT 'hello w...')
#1 {main}
  thrown in /home/enorris/workspace/php-src/test.php on line 16

Notes to maintainers:

  • I'm not particularly tied to the method name, so open to suggestions.
  • I used beginTransaction as a reference, feel free to let me know if the method should be implemented differently.

Exposes the driver's underlying pdo_dbh_check_liveness_func to PHP
scripts that may want to check if a connection is still open.
@rentalhost
Copy link

Generally it is called "ping".

But in case of ping, it will try to reconnect if connection is gone. I do not think that reconnect should occur in this case, but mysqli does.

https://www.php.net/manual/en/mysqli.ping.php
https://www.php.net/manual/en/function.mysql-ping.php (ext deprecated)

@ericnorris
Copy link
Contributor Author

@rentalhost when I first started on this I had originally named it PDO::ping, but I noticed that (if I understand correctly) the pgsql driver does not actually ping:

/* {{{ */
static int pdo_pgsql_check_liveness(pdo_dbh_t *dbh)
{
pdo_pgsql_db_handle *H = (pdo_pgsql_db_handle *)dbh->driver_data;
if (!PQconsumeInput(H->server) || PQstatus(H->server) == CONNECTION_BAD) {
PQreset(H->server);
}
return (PQstatus(H->server) == CONNECTION_OK) ? SUCCESS : FAILURE;
}
/* }}} */

...and since the pdo_dbh_check_liveness_func does not actually define a requirement that the PDO driver ping the database, I decided to name it checkLiveness to match.

I wouldn't be opposed to submitting an additional PR for an explicit PDO::ping method, if there was an appetite for it. See #5947, as well.

@carusogabriel
Copy link
Contributor

cc @morozov @mbeccati, you might be interested in this one :)

@morozov
Copy link
Contributor

morozov commented Aug 14, 2020

@carusogabriel FWIW, we got rid of the “ping” API in favor of automatic recovery from a lost connection (doctrine/dbal#4119). At the application logic level, the fact that a call to checkLiveness() succeeds doesn't guarantee that a subsequent query won't fail due to a lost connection, so at the end, the caller will have to handle the failure of the query execution anyways.

@mbeccati
Copy link
Contributor

Thanks @carusogabriel. I do agree with @morozov here.

@ericnorris could you please better explain what's the use case with your employer and how does the method actually solve their problem?

@ericnorris
Copy link
Contributor Author

@morozov true! checkLiveness does not guarantee that a subsequent query will succeed, but it does provide you with a relatively low-cost way of having a pretty good idea before executing a query. If I understand correctly, internally the pdo_dbh_check_liveness_func is used for persistent connections in a similar capacity.

I discussed automatic retries with a fellow co-worker and we both felt that it would be too dangerous; is it truly safe to retry a query in the face of 2006 MySQL server has gone away ? Maybe we're incorrect, but I could imagine a situation where this error occurs but the query had successfully made it to the server.

Regarding a use-case @mbeccati: at the time of writing Etsy currently does the pseudo "ping" statement that I mentioned above (SELECT 1 FROM dual) when reusing a connection after some configurable amount of time in order to ensure the wait_timeout has not occurred. Recently I had the opportunity to investigate potential improvements to this functionality, and came up with this PR as one of the options. When I researched this problem online, I found a number of other folks doing similar things (see the links above), and thought that it may be useful for the general public, even if that does not end up being the way we solve the problem.

@morozov
Copy link
Contributor

morozov commented Aug 14, 2020

does not guarantee […] a pretty good idea

This sounds contradictory to me.

Maybe we're incorrect, but I could imagine a situation where this error occurs but the query had successfully made it to the server.

The usage of this API doesn't prevent this from happening. You can ping the server at the end of the allowed time window, and the subsequent query that relies on OK will still fail.

@morozov
Copy link
Contributor

morozov commented Aug 14, 2020

I discussed automatic retries with a fellow co-worker and we both felt that it would be too dangerous.

Above, I was talking about automatic reconnects, not retries. Of course, the retry logic should be implemented at the application level based on the query semantics.

@ericnorris
Copy link
Contributor Author

@morozov I'm not sure I'd agree it's contradictory; since queries aren't always safe to retry, and there is a low cost way of detecting that the next query will definitely fail, isn't it worth saving yourself the trouble when reusing a long-lived connection? That is, why even try the next query if you absolutely know that it will not work? That would be a waste.

Checking the liveness doesn't guarantee that it will work, but it can tell you if you should reconnect before even trying. It seems that this is a pretty common desire that I've seen around the internet, and (barring that this is a different language) GitHub does something similar in their go connection pool logic. From the article:

Clearly, the optimal solution would be checking for a connection's health once it's pulled from the connection pool, and before we attempt to write any requests to it.

Lastly, the PDO driver does this for you with persistent connections - as of today if you used persistent connections and didn't do any application-side connection pooling you would get the very same behavior that I am proposing to expose above. Since folks (including my employer) don't necessarily use persistent connections and/or they implement their own application-layer pools, it would be nice to have the same ability via an exposed checkLiveness method.

@twose
Copy link
Member

twose commented Aug 15, 2020

Hi everyone, should we discuss it on the internal mailing list? I have posted my opinion there.

@ramsey ramsey added the Feature label Jun 9, 2021
@ramsey
Copy link
Member

ramsey commented Jun 9, 2021

What's the status of this? It doesn't look like there was much discussion on the mailing list, and there appears to be some disagreement about the feature in this thread. Does this require changes, more review, more discussion?

@ericnorris
Copy link
Contributor Author

Thanks for following up @ramsey! I'm still interested in this, but was somewhat discouraged by the pushback here and lack of discussion in the mailing list.

My basic argument is that this functionality is asked for, has a low implementation cost, and is already used by persistent connections. The idea of checking a connection before issuing a query is non-controversial (e.g. the previously linked GitHub article), and although frameworks may not use this today, application developers can decide if it's right for their use case - it would be in our case, as we will not be moving to said frameworks anytime soon.

That being said, I'm not sure if this requires an RFC or more consensus work in the mailing list; this is my first time adding a function to PHP. It's worth noting this pairs well with #5947, which could reduce the cost of this function to a non-blocking read of the mysql socket.

@mbeccati
Copy link
Contributor

mbeccati commented Jun 10, 2021

Hi @ericnorris - Firstly, thanks for the PR. Feedback here was admittedly not great, nor on the mailing list.

TBH, if you ask me, in case the client is waiting for a long time my suggestion would be to close the connection and reopen when you need it, so that you free the unused connection slots. On postgres (at the very least) I'd also use a dedicated connection pooler.

However giving more control to the userland might be desirable for some applications of framework, so I would say this deserves an RFC, if you're willing ot pursue it.

@ericnorris
Copy link
Contributor Author

Hey all!

I reconvened with my team and we instead came up with a simpler solution for our purposes that we will attempt to implement at some time in the future. For posterity, the idea is to track how long a connection has been open ourselves, and if it is greater than our configured wait_timeout in MySQL, we'll assume the connection is dead and reopen it.

While this wouldn't allow us to catch connections that had prematurely closed, nothing would be able to prevent that 100% of the time, and this is probably good enough.

I'm going to go ahead and close the pull request, as I don't feel like taking this through the RFC process, but happy to help out if someone else decides it's worth it for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants