fix(agent): Fixes warning from mysqli when explaining slow SQL queries (GH issue 875) #881
Conversation
In Github issue 875 it was reported that the agent causes a warning
to be generated when using the mysqli extension. The cause of the
warning was the agent reusing a stale SQL string associated with a
previous mysqli::prepare statement. This commit ensures that stale
SQL strings are cleared when this state is detected.
mfulb
requested review from
ZNeumann,
zsistla,
lavarou,
bduranleau-nr and
hahuja2
|
| */ | ||
| metadata = nr_php_mysqli_query_find(handle); | ||
| if (NULL == metadata) { | ||
| return NR_FAILURE; |
There was a problem hiding this comment.
Is this an error condition (the comment above suggests it is not)? Does nr_php_mysqli_query_clear_query need to return any value? It's return value is not checked in nr_php_prepared_prepare_general where it's used.
There was a problem hiding this comment.
A good question - was just mirroring the API for the set() function. In the current use case of this function it is not important to know if any data was actually cleared - just that if it exists it was cleared. It is possible the handle has never been used before so there would be no data to clear. So this particular case (lines 362-364) is not a failure but a consequence of a reasonable, possible state of the agent.
But the NR_FAILURE return code does give the caller the context that there was no existing data for the handle which might be useful in a future, different use?
There was a problem hiding this comment.
NR_FAILURE definitely brings in connotations that something went wrong. However, as you say, this is a normal condition and hence, NR_FAILURE seems wrong here. If there was no data, it's as if it's already been cleared. If it's desired to separate result codes that indicate that nothing got cleared vs data was cleared vs clearing data failed, a different return value should be used for all cases.
There was a problem hiding this comment.
This PR can be merged with the code as is.
|
I would prefer if the PR title explained the fix in addition to the issue number, rather than just refer to the issue |
mfulb
changed the title
Is this better? |
A EXPECT_SLOW_SQLS specification was left out of one test from PR #881 .
In GH issue #875 it was reported a warning is generated when using prepared statements with
mysqli. The issue included excellent instructions on how to reproduce the issue. This PR addresses the cause of the warning.To understand the cause of the warning requires understanding how the agent handles
mysqli::stmtobjects. When a SQL statement is prepared via mysqli the agent will store the SQL string in a global hashmap calledmysqli_queriesif the SQL is considered explainable (determined vianr_php_explain_mysql_query_is_explainable()), otherwise it is just ignored. Later when a slow SQL query is detected the SQL string is retrieved and used to explain the query.An additional factor is when a
mysqli::stmtobject is released it goes into a free pool PHP keeps which allows it to quickly hand out an allocated section of memory when a new object is created. Each object has aobject ID(referenced as ahandlein the agent for the relevant code). When an the allocated object is reused from the free pool it retains the sameobject ID.The agent used the
object ID(handle) as the key for storing the prepared SQL string for explainable SQL strings. Now what if an explainable query comes through, the string is stored, and then that object is released. A new query is prepared with a string that is NOT explainable and gets the object container with the sameobject IDas the explainable one just released. The string stored inmysqli_queriesusing thatobject IDas the key is now stale. If the new, unexplainable query is slow then the agent will pull this stale string and try to run an explain with it - leading to the warning.The fix is to clear any query string for a given
object IDif the query string is unexplainable.Integration tests were added (based on the reproduction case) that test for a regression of this fix.