fisharebest / webtrees

Online genealogy
https://webtrees.net
GNU General Public License v3.0
463 stars 299 forks source link

SQL Error on Family Page #3347

Open miqrogroove opened 4 years ago

miqrogroove commented 4 years ago

Something broke rather dramatically when I clicked on a link to a family just now (my F353).

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1-family.php-F353' for key 'PRIMARY' (SQL: insert into `wt_hit_counter` (`gedcom_id`, `page_name`, `page_parameter`, `page_count`) values (1, family.php, F353, 1)) …/vendor/illuminate/database/Connection.php:664
#0 …/vendor/illuminate/database/Connection.php(624): Illuminate\Database\Connection->runQueryCallback('insert into `wt...', Array, Object(Closure))
#1 …/vendor/illuminate/database/Connection.php(459): Illuminate\Database\Connection->run('insert into `wt...', Array, Object(Closure))
#2 …/vendor/illuminate/database/Connection.php(411): Illuminate\Database\Connection->statement('insert into `wt...', Array)
#3 …/vendor/illuminate/database/Query/Builder.php(2646): Illuminate\Database\Connection->insert('insert into `wt...', Array)
#4 …/vendor/illuminate/database/Query/Builder.php(2735): Illuminate\Database\Query\Builder->insert(Array)
#5 …/app/Module/HitCountFooterModule.php(193): Illuminate\Database\Query\Builder->updateOrInsert(Array, Array)
#6 …/app/Module/HitCountFooterModule.php(144): Fisharebest\Webtrees\Module\HitCountFooterModule->countHit(Object(Fisharebest\Webtrees\Tree), 'family.php', 'F353')
#7 …/vendor/oscarotero/middleland/src/Dispatcher.php(136): Fisharebest\Webtrees\Module\HitCountFooterModule->process(Object(Nyholm\Psr7\ServerRequest), Object(Middleland\Dispatcher))
#8 …/app/Http/Middleware/CheckCsrf.php(75): Middleland\Dispatcher->handle(Object(Nyholm\Psr7\ServerRequest))
#9 …/vendor/oscarotero/middleland/src/Dispatcher.php(136): Fisharebest\Webtrees\Http\Middleware\CheckCsrf->process(Object(Nyholm\Psr7\ServerRequest), Object(Middleland\Dispatcher))

The error is transient or one time only. No further problem after refresh.

mysql> SELECT * FROM wt_hit_counter WHERE page_parameter = 'F353';
+-----------+------------+----------------+------------+
| gedcom_id | page_name  | page_parameter | page_count |
+-----------+------------+----------------+------------+
|         1 | family.php | F353           |          2 |
+-----------+------------+----------------+------------+
1 row in set (0.00 sec)

webtrees v2.0.3

fisharebest commented 4 years ago

This can happen if two requests are made at the same time - for a page that has never been visited before.

Both will see no existing record, and attempt to insert one.

At the end of the transaction, one will fail.

To reproduce this

1) open the same page in two tabs. 2) delete entries from wt_hit_counter. 3) delete entries from wt_session (because we cache the hit counter in the session). 4) select both tabs (shift-click in firefox) and reload together

This can only happen for two "read-only" requests. An "update" request must follow a read request, which would create the row in wt_hit_counter.

So this bug can never lose data.

We could lock the table - this means we would get a "deadlock" error instead of a "duplicate value" error.

There is logic to handle deadlock errors - the entire request is repeated (up to 3 times).

But this doesn't help. We update the counter at the start of the request, and this simply means that the the second request fails three times before the first request has completed.

We can't move the hit-count logic to the end of the request - we need the number during the request.

Maybe we can split it in two...

miqrogroove commented 4 years ago

Is your database driver compatible with the command INSERT ON DUPLICATE KEY UPDATE ? That is a common way to resolve insert races.

fisharebest commented 4 years ago

I don't think so.

We use Laravel's query builder, and it implements it as select + insert/update.

Presumably because not all databases support it.

fisharebest commented 4 years ago

Although that still wouldn't help.

Both would try to insert, because we use transactions, and the statement wouldn't know if the row exists until we commit the transaction

miqrogroove commented 4 years ago

If I disable the hit counter setting, does that stop the queries and errors? Or will it still count the hits internally?

fisharebest commented 4 years ago

If you disable the module, it will stop counting hits.

If the module is enabled, but the footer is disabled, then it will count hits, but not display them.

miqrogroove commented 4 years ago

I would either commit and retry once per request, or commit and immediately ignore any errors. Hit counting should be a very low priority DB use and should not lock the table.

fisharebest commented 4 years ago

I have been trying to solve this using "pre-emptive locking" - e.g. locking a row which does not (yet) exist.

This is allowed in "Standard SQL", but MySQL has a very old bug which means that it does not work:

https://bugs.mysql.com/bug.php?id=25847

It may work in other databases, such as Postgres - but I don't have a postgres database on my dev machine.

I could lock the entire table - but the performance hit would be unacceptable.

The only solution might be a try/catch block which ignores all errors. But I really don't like doing that.

You need an unusual set of circumstances to trigger this, plus some careful timing.

Maybe just mark this one "wontfix`?

fisharebest commented 4 years ago

There are almost certainly many other similar cases, for example, writing to all the wt*settings tables.

miqrogroove commented 4 years ago

Up to you, but I would research this a bit more. You're designing an asynchronous interface for a multi-user platform and query races are a fact of life here. I would be shocked if your database driver doesn't have a way to handle this.

miqrogroove commented 4 years ago

It might even be as simple as checking how many rows were inserted in the catch block and then adding the update query.

ric2016 commented 4 years ago

Just a note that these kinds of errors occur in other places as well, e.g. in Place.php, see this issue.

(in this case triggered by a custom module handling a place multiple times concurrently, but the actual problem is in the core code - the insert isn't safe wrt concurrency)

melizaa commented 4 years ago

I get the error lately in testing for individuals, mediaviewer and places.

fisharebest commented 4 years ago

The same error or a similar error.

This error is in the hit-counter module. The places page does not use the hit-counter.

melizaa commented 4 years ago

I get a similar error: Integrity constraint violation: 1062 Duplicate entry.

miqrogroove commented 4 years ago

To add some context: I just figured out that my mouse is worn out and sometimes sending multiple clicks while the button is pressed. So that's one way to encounter the problem.