Fix duplicate key value violation in setValues DB function #27576

mrow4a · 2017-04-05T23:18:41Z

This fixes #23213 issue when one changes the oc_preferences in OC10.

Unit tests
Backport - maybe fixes more?

mrow4a · 2017-04-06T01:03:02Z

@DeepDiver1975 is extremely hard to debug, OCA\Encryption\Tests\MigrationTest::testUpdateDBExistingNewConfig. How can I run this one test as single? I guess I need to setup encryption on local instance, sth else ?

It fails because I removed preconditions from setValue, and I need to adjust it to use updateValue instead.

mrow4a · 2017-04-06T09:09:01Z

lib/private/DB/Connection.php

 					}, array_merge($keys, $values))
 				);
 			return $insertQb->execute();
 		} catch (ConstraintViolationException $e) {


I am extremely unhappy with this line -> we have 3 options:

writing specialized queries everywhere instead of universal "set"

doing workaround as it is now

writing this properly, but this will require additional query, and we would need a transaction here to ensure ACID principles are preserved in case of concurent accesses and be sure the ConstraintViolationException never happens

And we already are doing transaction to oc_properties properly

core/apps/dav/lib/Files/CustomPropertiesBackend.php

Line 212 in 7f4a4e7

private function updateProperties($fileId, $properties) {

as transaction, shall I do it? @DeepDiver1975 @PVince81 ?

mrow4a · 2017-04-06T12:57:36Z

Current implementation will basicaly first perform select and insert if possible or knowing record does not exists (NOT EXISTS even wrapped by INSERT is very quick in that case), it will do update. Simple and easy. Problem here is that we need proper ACID handling maybe with transaction?

Tried also with ON DUPLICATE KEY UPDATE, but this is not working in SQLLite, so we cannot do it in one query and we need transaction 🔢

DeepDiver1975 · 2017-04-06T12:58:44Z

lib/private/DB/Connection.php


 	/**
-	 * Insert or update a row value
+	 * Insert or update a row value. For safety, user of the method should lock table and begin/end transaction


don't lock - lock is bad - doesn't work with galery cluster and breaks on mysql master replicatin

but transaction I is supported isnt it? Will erase it, but generaly I think better would be to have transaction in place there, what do you think @DeepDiver1975

mrow4a · 2017-04-06T18:22:19Z

@DeepDiver1975 Any decision regarding the comment there? Should I play with transaction there or leave as it is, since it solves the general issue?

jvillafanez · 2017-04-11T12:02:45Z

I'm not convinced with the solution. We should keep using the query builder if possible.

In addition, the insertIfNotExist might not be standard and we might be wrapping it for some DB providers (I might be wrong). If the DB doesn't provide a native method for this, I think it's better to use it just as a convenience function, otherwise we have a risk of different behaviour depending on the target DB.

mrow4a · 2017-04-11T12:08:00Z

Should I write another PR killing \OC\DB\Adapter @jvillafanez ? Can we lock table with QueryBuilder as in adapter and replicate other functions ? @DeepDiver1975 @PVince81 What was the reasoning behind the Adapter?

mrow4a · 2017-04-24T07:43:58Z

Any update and input here? In the issue #23213 it seems really problem to solve, and this fixes it.

jvillafanez · 2017-04-25T10:10:43Z

I guess we can go with this solution in order to fix the issue.

mrow4a · 2017-04-25T10:22:24Z

@PVince81 @DeepDiver1975 anything I did not think here and could be important? This is quick change actualy.

mrow4a · 2017-05-23T11:32:35Z

@PVince81 @DeepDiver1975 @jvillafanez Just retested everything, and this fixes the issue for PostgreSQL #23213. It would be good to have it, to avoid the issues and save time of e.g. @pako81 which customer changed that query so that it does not log warning.

PVince81 · 2017-05-30T11:51:19Z

insertIfNotExists is AFAIK already adjusted in the adapters for different database types.

So the change looks ok.

@mrow4a did you find out from old commit logs / PRs if there are clues why this wasn't directly done with insertIfNotExists ? Maybe the function didn't exist yet ? Am just wondering whether there was another legitimate reason for this choice.

mrow4a · 2017-06-20T21:35:29Z

@PVince81 what do we do with this PR?

PVince81 · 2017-06-21T06:38:07Z

@mrow4a did you find out from old commit logs / PRs if there are clues why this wasn't directly done with insertIfNotExists ? Maybe the function didn't exist yet ? Am just wondering whether there was another legitimate reason for this choice.

mrow4a · 2017-06-21T07:40:49Z

I dont know, my quick guess is that someone though it will just be faster or that function did not exist yet there. I can research if you want, but it needs to be done like it is now, or we drop the support for sqlite and write it properly with ON DUPLICATE KEY UPDATE (which I guess is no go :>)

PVince81 · 2017-06-21T07:57:45Z

@mrow4a I'm fine with your change. I just want to make sure there wasn't an explicit decision in the past to not use it. I hope you're right and it's simply that the method did not exist yet.

PVince81 · 2017-07-04T12:37:41Z

@mrow4a how was the research ?

tomneedham · 2017-07-25T13:48:07Z

Original implementation: #18531. Seems insertIfNotExists was considered but ignored for no real reason. We should use insertIfNotExists as this is polluting SQL error logs at the moment.

PVince81 · 2017-07-25T13:49:54Z

@tomneedham sounds good, then let's get this in. Thanks

PVince81 · 2017-07-25T14:03:17Z

lock · 2019-08-02T23:02:34Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mrow4a added the 3 - To Review label Apr 5, 2017

mrow4a added this to the 10.0 milestone Apr 5, 2017

mrow4a requested review from DeepDiver1975, PVince81 and jvillafanez April 5, 2017 23:18

mrow4a mentioned this pull request Apr 5, 2017

duplicate key value violates unique constraint "oc_preferences_pkey" on every login #23213

Closed

mrow4a force-pushed the fix_set_value_query branch 2 times, most recently from 0e674ce to 4302287 Compare April 6, 2017 00:53

mrow4a commented Apr 6, 2017

View reviewed changes

mrow4a force-pushed the fix_set_value_query branch from 4302287 to ee5b47a Compare April 6, 2017 11:59

DeepDiver1975 reviewed Apr 6, 2017

View reviewed changes

mrow4a force-pushed the fix_set_value_query branch from ee5b47a to 4993b96 Compare April 10, 2017 12:01

mrow4a force-pushed the fix_set_value_query branch from 4993b96 to 6ba2ae3 Compare April 24, 2017 07:47

PVince81 modified the milestones: 10.0.1, 10.0 Apr 26, 2017

PVince81 modified the milestones: 10.0.2, 10.0.1 May 19, 2017

Fix duplicate key value violation in setValues DB function

6bc82e2

mrow4a force-pushed the fix_set_value_query branch from 6ba2ae3 to 6bc82e2 Compare May 23, 2017 11:29

mrow4a assigned PVince81 May 24, 2017

PVince81 removed this from the 10.0.2 milestone May 26, 2017

PVince81 modified the milestones: 10.0.3, 10.0.2 May 26, 2017

PVince81 assigned mrow4a and unassigned PVince81 Jul 10, 2017

PVince81 added the backport-request label Jul 25, 2017

PVince81 merged commit 97d0db3 into master Jul 25, 2017

PVince81 deleted the fix_set_value_query branch July 25, 2017 13:50

This was referenced Jul 25, 2017

[stable9.1] Backport of setValues to use insertIfNotExists #28484

Merged

[stable9] Backport to make setValues use insertIfNotExists #28485

Merged

tomneedham mentioned this pull request Jul 25, 2017

[stable10] Backport of setvalues to use insertIfNotExists #28486

Merged

mrow4a mentioned this pull request Aug 3, 2017

race condition in insertOrUpdate queries #22563

Closed

PVince81 removed the backport-request label Aug 15, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Fix duplicate key value violation in setValues DB function #27576

Fix duplicate key value violation in setValues DB function #27576

Uh oh!

Conversation

mrow4a commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrow4a Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

mrow4a commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

mrow4a Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrow4a commented Apr 6, 2017

Uh oh!

jvillafanez commented Apr 11, 2017

Uh oh!

mrow4a commented Apr 11, 2017

Uh oh!

mrow4a commented Apr 24, 2017

Uh oh!

jvillafanez commented Apr 25, 2017

Uh oh!

mrow4a commented Apr 25, 2017

Uh oh!

mrow4a commented May 23, 2017

Uh oh!

PVince81 commented May 30, 2017

Uh oh!

mrow4a commented Jun 20, 2017

Uh oh!

PVince81 commented Jun 21, 2017

Uh oh!

mrow4a commented Jun 21, 2017

Uh oh!

PVince81 commented Jun 21, 2017

Uh oh!

PVince81 commented Jul 4, 2017

Uh oh!

tomneedham commented Jul 25, 2017

Uh oh!

PVince81 commented Jul 25, 2017

Uh oh!

PVince81 commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lock bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mrow4a commented Apr 5, 2017 •

edited

Loading

mrow4a commented Apr 6, 2017 •

edited

Loading

mrow4a Apr 6, 2017 •

edited

Loading

mrow4a commented Apr 6, 2017 •

edited

Loading

mrow4a Apr 6, 2017 •

edited

Loading

PVince81 commented Jul 25, 2017 •

edited

Loading