[WIP] Upsert support prototype. #163

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
6 participants
Member

kimhemsoe commented Jun 29, 2012

No description provided.

Owner

beberlei commented Jun 29, 2012

You committed a .DS_STORE file, please remove it.

lib/Doctrine/DBAL/Connection.php
+ } else {
+ $sql = 'SELECT COUNT(*) FROM ' . $tableName
+ . ' WHERE ' . implode(' = ? AND ', array_keys($identifier))
+ . ' = ?';
@beberlei

beberlei Jun 29, 2012

Owner

Should be with locking, since otherwise the row could be deleted in that timeframe.

[UPSERT] Removed a sneaky .DS_Store file.
[UPSERT] Added locking when not using native upsert support.

This pull request fails (merged 3322794 into 4eaa44c).

This pull request fails (merged a2f602d into 4eaa44c).

This pull request fails (merged 024410a into 4eaa44c).

This pull request passes (merged 03840fc into 4eaa44c).

lib/Doctrine/DBAL/Connection.php
+ if ($this->_platform->supportsNativeUpsert()) {
+ list ($sql, $params, $types) = $this->_platform->getUpsertSql($tableName, $data, $identifier, $types);
+ return $this->executeUpdate($sql, $params, $types);
+ } else {
@stof

stof Jul 2, 2012

Member

no need to use else here as the if returns.

lib/Doctrine/DBAL/Connection.php
+
+ if ($haveRows) {
+ return $this->update($tableName, $data, $identifier, $types);
+ } else {
@stof

stof Jul 2, 2012

Member

same here

+ * @throws Doctrine\DBAL\DBALException
+ * @return array
+ */
+ public function getUpsertSql($tableName, array $data, array $identifier, array $types)
@beberlei

beberlei Jul 5, 2012

Owner

Why $types?

Member

kimhemsoe commented Jul 5, 2012

well.. it is a update or insert. was thinking i could follow the same rule as with update().

it is broken atm tho.

Member

stof commented Jul 5, 2012

btw, it seems a bit weird to add a way to support native upsert, but without supporting it in any platform.

Member

kimhemsoe commented Jul 5, 2012

I will add atleast mysql support before its getting merged.

staabm commented Jul 5, 2012

In mysql you could use REPLACE INTO instead, so only one statement will be required..

Owner

beberlei commented Jul 5, 2012

-1 on REPLACE INTO, ON DUPLICATE KEY is much better.

staabm commented Jul 5, 2012

Is there any difference between a REPLACE INTO and INSERT ... ON DUPLICATE KEY..?

Member

kimhemsoe commented Jul 5, 2012

REPLACE into have some nasty side effects, cant find a good text about it right now. Another problem with replace is that it have no "where clause" ON DUPLICATE KEY is the same, but a little different as i use unique constraints for it.

It wont be possible to make upsert() to behave 100% the same on all platforms.

Owner

beberlei commented Jul 5, 2012

if you have auto_increment on another oclumn than primary key, then REPLACE INTO will increment it.

Edit: In MySQL its literally implemented as DELETE, then INSERT INTO. Wheras ON DUPLICATE is an UPDATE.

Member

kimhemsoe commented Jul 5, 2012

Think we have the same problem with sqlite and replace there. Dont think we get any native support there anytime soon.

sqlserver and OCI8 have merge support and im hoping someone will do those.

postgre dont have anything easy accessible , but can be done on never versions with some magic or.. something.

Ive been thinking we may need a swith to turn off native support.

staabm commented Jul 5, 2012

@kimhemsoe @beberlei thanks for the explanation.

Member

kimhemsoe commented Jul 16, 2012

@mvrhov Thanks i will give a read, but..yeah it is giving me the shivers. It may end up with one big warning clause in the documentation :-P

kimhemsoe added some commits Sep 22, 2012

Merge branch 'master' into upsert
Conflicts:
	lib/Doctrine/DBAL/Connection.php
Owner

beberlei commented Dec 20, 2013

Closing for now, @deeky666 will work on a new solution in the future.

@beberlei beberlei closed this Dec 20, 2013

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