WireDatabasePDO's "execute" function does not re-create connection before retry

[romaincazier]

Short description of the issue

When a request takes too long, the MySQL server goes away thus making the next query fail. The execute function's comment states that it "will retry queries if they failed due to a lost connection", however it seems it just retries the query, which is still linked to the connection that has gone away.

Expected behavior

Even though we could play with the MySQL wait_timeout setting, it is not necessarily possible in some cases (e.g. shared hosting), making the retry aspect of Processwire's PDO highly valuable. But it doesn't work here.

From my understanding, once it detects a fail / retries, it should first re-create the connection and then re-construct the query based on this new connection.

Suggestion for a possible fix

What I tried to do, and seemed to work at first, was adding this code before the line 583:

if ($tryAgain > 0) {
  $newQuery = $this->wire('database')->prepare($query->queryString);
  foreach ($query->getParams() as $value) {
    $newQuery->bindValue($value["parameter"], $value["value"], $value["data_type"]);
  }
  $query = $newQuery;
}

Two things to note: I had to make every PDOStatement a WireDatabasePDOStatement, and in the WireDatabasePDOStatement file I created a params var populated with the values binded in the bindValue function:

$this->params[] = [
  "parameter" => $parameter,
  "value" => $value,
  "data_type" => $data_type,
];

And accessible with the getParams function mentionned above.

However, even if it worked for my code below (in the "Step to reproduce the issue"), I still had the 2006 MySQL error afterwards at this line.

Side note: this second error led me to the DatabaseQuery file which seems to provide the abstraction for what I'm describing above...? So maybe it could be there that we should retry the query ? I don't know at this point if every query in PW is a DatabaseQuery or not.

Step to reproduce the issue

Here is a code sample that triggers this error:

$page->of(false);
$title = $page->title;
$page->title = "Error";
$page->save();
sleep(60); // adjust according to your server's wait_timeout setting
$page->title = $title;
$page->save();

Setup/Environment

Server Details

Software	Version
ProcessWire	3.0.165
PHP	7.3.25
Webserver	Apache
MySQL Server	5.7.30-log
MySQL Client	mysqlnd 5.0.12-dev - 20150407 - $Id: 7cc7cc96e675f6d72e5cf0f267f48e167c2abb23 $

Server Settings

Parameter	Value
allow_url_fopen
max_execution_time	60 (changeable)
max_input_nesting_level	64
max_input_time	0
max_input_vars	3000
memory_limit	640M
post_max_size	300M
upload_max_filesize	300M
xdebug
xdebug.max_nesting_level
mod_rewrite
mod_security
EXIF Support	1
FreeType	1

GD Settings

Parameter	Value
Version	bundled (2.1.0 compatible)
GIF	1
JPG	1
PNG	1
WebP	1

iMagick Settings

Parameter	Value
Version	6.8.9
GIF	1
JPG	1
PNG	1
SVG	1
PDF	1
WebP

Module Details

Module ClassName	Version
AdminThemeUikitModified	0.3.1
ImageCreateVariations	1.0.0
ProcessTracyAdminer	1.0.7
TracyDebugger	4.21.23

[ryancramerdesign]

@romaincazier Thanks for the report. The database will attempt reconnect automatically, but in this case you are right that the existing query isn't going to cut it, because it's bound to the PDO instance that failed rather than the new one (good catch). As far as I can tell, there isn't a way to inject a new PDO instance to an existing PDOStatement, and that's what we'd need to do this cleanly.

I think your suggestion for a fix is a very good one, but in the interests of keeping the class as optimized as possible, I also think keeping copies of all params is likely going to be too much overhead (notably memory) relative to the potential benefits, as some param values can be quite large. Plus, the WireDatabasePDOStatement is a class for debug mode only, where we are okay with some extra overhead.

I think the re-connect logic should move into the DatabaseQuery class instead (also per your suggestion), where the structure of the logic better supports it. DatabaseQuery is only used by queries in ProcessWire that need to be passed around and populated to different methods that don't necessarily know about each other. An example is PageFinder queries, which get passed around to different Fieldtypes. Admittedly this is not as useful as having it directly in WireDatabasePDO, as the majority of queries in PW do not use DatabaseQuery. But it is at least practical there, without overhead, and available for anyone that wants to use it for their own queries. So at least that puts us in a better spot than before, where the logic was built into WireDatabasePDO but wasn't actual functional.

[romaincazier]

Thank you for the fix. In my case it won't help indeed, but it's still a good thing to do !

I have two questions though:

• About the memory overhead — I'm clearly not a php expert so I have very limited knowledge here — but do you think about the memory consumed by cumulative PDOs or a single one ? If cumulative, once a PDO is executed, isn't it just a matter of relieving the memory from what's been cached ?
• What if you add a timer and check the delta after the request, and if it crosses a threeshold (say 30s, configurable with $config), you preemptively close the connection ? Again I don't know the implications of this (re connecting many more times potentially), but it could replace what you had in mind initially.

Let me know what you think, and thanks again for having a look at this.

[ryancramerdesign]

@romaincazier Memory is released yes, but in this case it would be doubling the current memory usage of queries, increasing the overall memory footprint (or max memory used). Then there is also some time overhead. These things don't fit that well with the overall strategy of keeping things as lean and lightweight as possible. Losing a DB connection should be a rare event, if it ever happens at all. So coding around it is something few PW users will ever need, but overhead everyone would pay for (memory, timers, code, etc.). And when such an issue does occur, my experience has been that it's usually because something outside PW needs to be fixed or reset, and trying to recover a connection would just further tax the server. In cases where we can support this kind of feature without much overhead or risk, I think it's definitely worthwhile. But for broader cases, I think it's best for PW to leave specific features like this to the 3rd party module implementations or custom integrations for cases where they might be needed.

[romaincazier]

Thanks for taking the time to respond to my questions and I totally get your point. I wouldn't want PW to be bloated either so I follow your call on this.

This issue usually occurs to me when there's a lot of image resizing happening on the same page. I definitely think I should dedicate some time to address this issue with a module.

I'm closing the issue.

[adrianbj]

@ryancramerdesign - I am getting occasional SQL Gone Away errors when doing a findMany() call and I am wondering if it's meant to be protected by the new code in DatabaseQuery? The problem is happening in Pages::getById because PageIterator is passing through an array of 250 page IDs at a time via loadChunk(). As far as I can tell, this means that DatabaseQuery is not involved at this point, and therefore the retries feature is not being triggered.

The problem seems to be that it goes from the $result = $database->execute($query); call in PagesLoader straight to $result = $query->execute(); in WireDatabasePDO.

Any chance you could look at this again please and see if there would be a way to implement the retry in this case as well?

[adrianbj]

@ryancramerdesign - am I correct in assuming that processwire/processwire@9803df9 is an attempt to fix this?

[ryancramerdesign]

@adrianbj This update was added primarily for the reset() method, which I'm using for an instance that commonly gets disconnected from MySQL (as it runs all day), so I use that to reconnect, and I start over with a fresh query. I updated the execute() method to use it, but to be honest I'm not certain it'll work. That's because the execute() method takes a PDOStatement and I'm not sure that a PDOStatement initiated on the old connection will still work on the new connection. If not, then the execute() method wouldn't work either. But I've not had an opportunity to test and find out whether it works or not.