Implement SQL_ATTR_CONNECTION_DEAD for ODBC
#9353
Conversation
This is semantically appropriate and should be used whenever the driver supports it. In the event that it fails or says the connection isn't dead (which may be inaccurate in some cases), try the old heuristic.
This is semantically appropriate and should be used whenever the driver supports it. In the event that it fails or says the connection isn't dead (which may be inaccurate in some cases), try the old heuristic.
ext/odbc/php_odbc.c
Outdated
| zend_hash_str_del(&EG(persistent_list), hashed_details, hashed_len); | ||
| goto try_and_get_another_connection; | ||
| } | ||
| /* If the driver doesn't support it, fall back to old heuristic */ |
There was a problem hiding this comment.
Hmm, can't we assume that the driver supports SQL_ATTR_CONNECTION_DEAD if ret == SQL_SUCCESS? I mean, can't we skip the fallback (SQL_DATA_SOURCE_READ_ONLY) regardless of the value of dead?
There was a problem hiding this comment.
Unfortunately, the Db2i driver at least had a case where it would return SQL_CD_FALSE, but the heuristic would detect a failed connection. However, that case (specifically, admin killing the database server process) is pretty weird and uncommon (though people bug me enough about it I have to mention it). You're probably right; it'd be better to just trust SQL_ATTR_CONNECTION_DEAD, and get driver authors to implement more accurate results if they're going to use it.
|
FWIW, at least for the Db2i driver, this thread from node-odbc covers some of the specific behaviours of the dead check (and why it's probably a bad idea to do more thorough checks) |
|
Sigh, ODBC and driver shenanigans! Apparently this is the best we can do. Could you please adjust the comments; it's not only about falling back if |
This, or return early if |
Form what I can tell, the liveness checks are only required for persistent connections; I think, we better make sure that we don't get a false positive for this. And from reading the thread you've linked above, it seems to me that I'm not sure how important persistent connections are for (PDO_)ODBC; I assume that ODBC connection pooling is almost always preferable (certainly for FCGI, where multiple processes won't necessarily share persistent connections). |
|
I think there's also a lot of code using persistent connections out of habit when it did matter/help more that hasn't been changed. That said, it's fair regarding that for user consistency. I don't think the current fallback check is too harmful to run; I just don't want it to accumulate too many checks that could just risk making it work. |
|
8.2 feature freeze is tomorrow; do we want to get this in before then? |
Feature freeze was already shortly before beta1 has been tagged. This doesn't really matter for this PR, since this is a small and self-contained improvement. Anyhow, thanks for the reminder! I'm going to merge. |
This uses the semantically correct
SQL_ATTR_CONNECTION_DEADattribute when possible, and in the event it fails or returns a successful false (since at least for Db2i in some circumstances, may be inaccurate), tries the old heuristic.May address GH-9347.