Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary read query on ORM insert #7072

Open
sminnee opened this issue Jun 28, 2017 · 7 comments
Open

Unnecessary read query on ORM insert #7072

sminnee opened this issue Jun 28, 2017 · 7 comments

Comments

@sminnee
Copy link
Member

sminnee commented Jun 28, 2017

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

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
Copy link
Contributor

so

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

Right?

@sminnee
Copy link
Member Author

sminnee commented Jun 28, 2017

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
Copy link
Member Author

sminnee commented Jun 28, 2017

I've created #7073. affectedRows() didn't seem to work but I suspect that the whole feature might be unnecessary.

@dhensby
Copy link
Contributor

dhensby commented Jun 30, 2017

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
Copy link
Member Author

sminnee commented Jun 30, 2017

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

  • create a page, set title
  • write
  • convert to blogpage, set author
  • write

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:

  • dev/build creates a new subclass table
  • changing the class of an existing record

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

@sminnee
Copy link
Member Author

sminnee commented Jun 30, 2017

$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
Copy link
Member Author

sminnee commented Jun 30, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants