[WIP] Connection: Upsert

[jnvsor]

This proposes a method that would allow consistent cross-platform upserts, with the downside that you have to explicitly provide the fields to check for existence of the row (It doesn't automatically handle unique constraint violations)

$db->upsert('tablename', ['fielda' => 'valuea'], ['fieldb' => 'valueb']);
// INSERT INTO tablename (fielda, fieldb) VALUES(?, ?) ['valuea', 'valueb']
$db->upsert('tablename', ['fielda' => 'valuea2'], ['fieldb' => 'valueb']);
// UPDATE tablename SET fielda = ? WHERE fieldb = ? ['valuea2', 'valueb']
$db->upsert('tablename', ['fielda' => 'valuea3', 'fieldb' => 'valueb2'], ['fieldb' => 'valueb']);
// UPDATE tablename SET  fielda = ?, fieldb = ? WHERE fieldb = ? ['valuea3', 'valueb2', 'valueb']

• Do we want to allow upserts that affect multiple rows?
  • If not, should we make a new exception class to handle this edge case?
• Any other suggestions?

[morozov]

It's not really an UPSERT, it's a combination of SELECT, INSERT and UPDATE. A proper implementation could be built on ON DUPLICATE KEY UPDATE for MySQL or MERGE for other platforms which support it.

[jnvsor]

@morozov Is it really that easy? I was under the impression there were some insurmountable issues with that, given that it's been high on the wishlist for years now

[morozov]

@jnvsor you can try and see if it is. There indeed are some serious issues. You can try approaching it and seeing where you end up. The last time I tried it, I ended up with INSERT ON DUPLICATE KEY UPDATE and MERGE requiring a different number of parameters bound to the query. So the eventual implementation will spread across the platform and connection layers.

[morozov]

Getting COUNT(*) with arbitrary WHERE may be expensive depending on the table size and filtering conditions. We don't need the count, we only need if there's a single row matching the criteria. Maybe, WHERE EXISTS (SELECT *) would be better than SELECT COUNT(*).

[jnvsor]

You would need the count if you wanted to prevent upsert accidentally updating multiple rows here

[morozov]

It's a slippery slope. If the UPSERT query is not specific enough so that it can match more than one record, then it's invalid for this use case, no matter if zero, one or more matching records exist.

Could you give an example of a table and/or query where INSERT would succeed but UPDATE would affect multiple rows?

[jnvsor]

That's also a good point

[jnvsor]

@morozov Is there a code-style rule that says AbstractDriver can't return multiple values? If it could return both the SQL and the values to bind to it that would solve the problem, but it would be a first so I'm guessing that's not gonna fly.

[morozov]

both the SQL and the values to bind to it

Isn't it what QueryBuilder is for?

[jnvsor]

That... Is actually a really good point. So we'd pass a querybuilder and an array of fields into the driver and the driver would return a new querybuilder with the query completed? (Or should we just alter the qb passed in in the first place?)

[morozov]

@jnvsor it doesn't matter that much at this point. You can make whatever properly working and tested PoC first and then we can polish the details.

[jnvsor]

@morozov Actually querybuilder won't work well either. The reason the drivers all return strings is because they tend to be non-standard. We'd have to shove the entire ON DUPLICATE KEY UPDATE clause into the orderBy on the QB for mysql for example. I mean, we can but... Seems really ugly.

[jnvsor]

Would require a more flexible querybuilder first. I've added an issue for that: #2972

[jnvsor]

Since @Ocramius has said we can't make the querybuilder more flexible, I don't see any way upsert can be implemented in DBAL. Closing.

[Ocramius]

@jnvsor a public one cannot be implemented, but an upsert emulation API can be provided.

[jnvsor]

@Ocramius Is there any way to get the fields from the constraint in question from a UniqueConstraintViolationException? What if the upsert is violating multiple unique constraints?

[Ocramius]

If we work on upsert, then it should most likely not be implemented by trial and fail. Each platform has a different syntax for it, so that should really be the last resort, with a fetch+update/insert rather than exception catching for systems that are incapable of dealing with the operation

…

On 9 Jan 2018 19:00, "Jonathan Vollebregt" ***@***.***> wrote: @Ocramius <https://github.com/ocramius> Is there any way to get the fields from the constraint in question from a UniqueConstraintViolationException? What if the upsert is violating multiple unique constraints? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2939 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakBkECg48rUVxYTN4OukAw-krAPDBks5tI6lYgaJpZM4RDx3P> .

[jnvsor]

@Ocramius We'd need to fetch all of the unique constraints one by one before insert/updating. Worst case scenario we'd have multiple structure lookups followed by multiple fetches before even deciding whether to insert or update

[Ocramius]

Yes, that's the downside of emulation 😜

…

On 9 Jan 2018 19:07, "Jonathan Vollebregt" ***@***.***> wrote: @Ocramius <https://github.com/ocramius> We'd need to fetch all of the unique constraints one by one before insert/updating. Worst case scenario we'd have multiple structure lookups followed by multiple fetches before even deciding whether to insert or update — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2939 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakPveggrthcn4CgEyYEaopwaeCSXCks5tI6rYgaJpZM4RDx3P> .

[jnvsor]

Given that that could be better performed when structure information is cached, perhaps we should bounce this back to orm and leave it out of DBAL?

[Ocramius]

I don't follow: what is cached?

…

On 9 Jan 2018 19:11, "Jonathan Vollebregt" ***@***.***> wrote: Given that that could be better performed when structure information is cached, perhaps we should bounce this back to orm and leave it out of DBAL? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2939 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakNOzMxlHDdobHS9EpxNdlhNosdRDks5tI6vUgaJpZM4RDx3P> .

[jnvsor]

Sorry, I presumed the orm layer cached structure information and probably unique constraints too

[Ocramius]

The ORM does that, but DBAL should still provide an API for performing the operation. The problem is that it cannot be done via query builder, but only as atomic transactional operation to deal with emulation.

[jnvsor]

@morozov I don't feel the kind of complex emulation discussed above is worth implementing, since it's very complex and very slow. My original idea was simpler, but rejected since it's not a true upsert.

I don't intend to work on this, and I'm fine with leaving this as a wontfix personally. Perhaps someone will redesign the querybuilder/drivers to allow upserts some day.

Maybe this should be moved to a more general 'Upsert' issue?

[morozov]

Perhaps someone will redesign the querybuilder/drivers to allow upserts some day.

I don't believe it's going to happen any time soon because of the complexity of the issue and absence of real demand. In my experience, upserts are mostly used when a DB table is used as a key-value storage which is usually way slower than other alternatives.

[gjdanis]

@morozov this issue is linked to a similar issue about supporting custom entity persisters.

If we can't fix this issue, can we at least look at making the entity persistence strategy configurable? It seems like we should be able to inject this somewhere in the UnitOfWork or perhaps provide it as metadata on the entity: https://github.com/doctrine/orm/blob/99b7d1e9d4bfa6efb86f0a77b1e5522ec8e9bf9a/lib/Doctrine/ORM/UnitOfWork.php#L2562

The specific use case I'm asking about is for MS SQL Server where one of my tables has a trigger that inserts into another another table. For this entity I want to use OUTPUT in my INSERT statements to get the identity rather than @@IDENTITY or SCOPED_IDENTITY(), neither of which seem to work correctly.

[morozov]

If we can't fix this issue, can we at least look at making the entity persistence strategy configurable?

This would be a question to ask in doctrine/orm.