silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

Unnecessary read query on ORM insert #7072

Open sminnee opened 7 years ago

sminnee commented 7 years ago

This goes way back to 2008: 2e955b498e0438bbe24f953f6e670cf069794339...

In order to correct a mistaken update query that should be an insert, there's an extra read query to check if the value is already in the database. This isn't efficient.

Instead, this should probably use the Database::affectedRows() instead.

tractorcow commented 7 years ago

so

$affected = 'INSERT IF NOT EXISTS' if (!$affected) { 'UPDATE' }

Right?

sminnee commented 7 years ago

It's the other way around - it needs to convert an update query to an insert one.

The other approach would be to remove this switch-over altogether and just assume that an update is correct. I think that it's probably a band-aid over other ORM bugs.

sminnee commented 7 years ago

I've created https://github.com/silverstripe/silverstripe-framework/pull/7073. affectedRows() didn't seem to work but I suspect that the whole feature might be unnecessary.

dhensby commented 6 years ago

During my dbal work I found this strange also. I think it's pretty much the only reason we have the ability to turn an insert into a select, which is also a strange api which is, unsurprisingly, missing from dbal.

sminnee commented 6 years ago

I've hit a few test failures on this. I think the use-case its catering to is:

Writing an existing blogpage should be UPDATEs, not INSERTs. However, at the time of class conversion I don't know that the blank record is created. So the updates, if they fail, switch over to an insert.

The other, better, way of fixing this would be to generate the missing records at the time they're needed.

A couple of situations come to mind:

Note: using affectedRows() doesn't seem to work; I think it returns 0 if you try to re-write the same data.

sminnee commented 6 years ago

$affected = 'INSERT IF NOT EXISTS' if (!$affected) { 'UPDATE' }

The weakness of this approach is that it still executes 2 queries in the normal case. Given the 2nd query is an edge-case for when data is inconsistent, I don't think that's good.

An UPSERT query, if the backend allows, would be better, but even that I suspect is slower than an UPDATE

sminnee commented 6 years ago

An initial read suggests that MySQL does an upsert and PostgreSQL does not, and that under the hood they're pretty hard things to do. So fixing the root cause is likely the best approach here.