refactor: Migrate away from \OCP\IDBConnection::insertIfNotExist#57124
refactor: Migrate away from \OCP\IDBConnection::insertIfNotExist#57124provokateurin wants to merge 3 commits intomasterfrom
Conversation
provokateurin
requested review from
Altahrim,
CarlSchwan,
icewind1991 and
leftybournes
and removed request for
a team
provokateurin
force-pushed
the
refactor/insert-if-not-exists
branch
from
d9b3fde to
d161459
Compare
Signed-off-by: provokateurin <[email protected]>
Signed-off-by: provokateurin <[email protected]>
Signed-off-by: provokateurin <[email protected]>
provokateurin
force-pushed
the
refactor/insert-if-not-exists
branch
from
d161459 to
7eb9092
Compare
| $this->db->insertIfNotExist('*PREFIX*dav_shares', [ | ||
| 'principaluri' => 'principal:unknown', | ||
| 'type' => 'calendar', | ||
| 'access' => 2, | ||
| 'resourceid' => 666, | ||
| ]); | ||
| $this->db->insertIfNotExist('*PREFIX*dav_shares', [ | ||
| 'principaluri' => 'principals/remote-users/foobar', | ||
| 'type' => 'calendar', | ||
| 'access' => 2, | ||
| 'resourceid' => 666, | ||
| ]); | ||
| $qb = $this->db->getQueryBuilder(); | ||
| $qb->insert('dav_shares'); | ||
|
|
||
| foreach (['principal:unknown', 'principals/remote-users/foobar'] as $principaluri) { | ||
| $qb->values([ | ||
| 'principaluri' => $qb->createNamedParameter($principaluri), | ||
| 'type' => $qb->createNamedParameter('calendar'), | ||
| 'access' => $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT), | ||
| 'resourceid' => $qb->createNamedParameter(666, IQueryBuilder::PARAM_INT), | ||
| ]); |
There was a problem hiding this comment.
The previous insertIfNotExists compared all input fields, while the unique index is create unique index dav_shares_index on oc_dav_shares (principaluri, resourceid, type, publicuri);. This seems to result in different numbers of rows, but only on sharding for some reason. I can't fully judge this, but I think the tests are incorrect in some way.
There was a problem hiding this comment.
I can't really wrap my head around the unique index vs the semantics of the insertIfNotExists. But the index definitely is not helping getting an atomic operation here.
How does the logic change with sharding?
There was a problem hiding this comment.
But the index definitely is not helping getting an atomic operation here.
How so? Before it was a SELECT + INSERT which was not atomic at all, leading to the duplicated oc_mounts issue. Now its UNIQUE INDEX + INSERT and this definitely is atomic.
How does the logic change with sharding?
This is what confuses me the most, it's not failing without sharding. I feel like this could be a bug in the sharding code? CC @icewind1991
There was a problem hiding this comment.
The index will only ensure that the (principaluri, resourceid, type, null) is unique. It doesn't cover the access column. So if you have an existing row (principaluri, resourceid, type, anything but null) the insert still happens without a unique constraint violation.
The old insertIfNotExist was not guaranteed to be atomic, but in the case of no concurrent modifications of the table (like in the sequentially executed phpunit tests) it still prevented duplicates with identical (principaluri, resourceid, type, access).
But I think we are on the same page anyway :)
The test outcome makes no sense. I also think that the sharding somehow doesn't send the deletions in the command to the same db instance as the select for the insertion.
4 participants
A unique index must be used to actually guarantee no duplicates, as this method is not even transaction safe (with READ_COMMITTED)