Skip to content

Conversation

@Altahrim
Copy link
Collaborator

@Altahrim Altahrim commented Jun 4, 2024

Summary

Add ignore conflicts method for MySQL
Similar to PgSQL: https://github.com/nextcloud/server/blob/master/lib/private/DB/AdapterPgSql.php#L26-L37

Checklist

@Altahrim Altahrim added the 3. to review Waiting for reviews label Jun 4, 2024
@Altahrim Altahrim added this to the Nextcloud 30 milestone Jun 4, 2024
@Altahrim Altahrim requested a review from a team June 4, 2024 08:17
@Altahrim Altahrim self-assigned this Jun 4, 2024
@Altahrim Altahrim requested review from ArtificialOwl, nfebe and yemkareems and removed request for a team June 4, 2024 08:17
@Altahrim
Copy link
Collaborator Author

Altahrim commented Jun 4, 2024

Actually, there is a different behaviour with PostgreSQL…

  • MySQL will update the duplicated rows with values from the insert
  • PostgreSQL will ignore the new values for duplicated rows.

I see two solutions:

  • We can mimic the behaviour of PostgreSQL in MySQL by updating only the primary key (but at this point we don't know the primary key). It will do nothing.
  • Or we may want to create a new method instead which will update the duplicate rows with new values for both storage engines, but I don't know if it's possible to do the same with Oracle

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On conflict this ones updates the row while pgsql implementation will not touch it, no?

@Altahrim
Copy link
Collaborator Author

Altahrim commented Jun 4, 2024

On conflict this ones updates the row while pgsql implementation will not touch it, no?

Yep, see above comment :)

@solracsf
Copy link
Member

solracsf commented Jun 4, 2024

On conflict this ones updates the row while pgsql implementation will not touch it, no?

Can this be an option?
https://mariadb.com/kb/en/insert-ignore/

@Altahrim
Copy link
Collaborator Author

Altahrim commented Jun 4, 2024

Can this be an option?
mariadb.com/kb/en/insert-ignore

For me it ignores too many errors. I would prefer to avoid this.

@Altahrim Altahrim force-pushed the feat/mysql_ignore_conflics branch from 5894248 to 115abb5 Compare June 5, 2024 09:21
@Altahrim Altahrim changed the title feat(dbal): add proper insert ignore conflict method for MySQL feat(dbal): insert ignore conflict method for MySQL and SQLite Jun 5, 2024
@Altahrim Altahrim force-pushed the feat/mysql_ignore_conflics branch from bc95813 to 330d35b Compare June 12, 2024 09:24
@Altahrim
Copy link
Collaborator Author

Finally, it seems INSERT IGNORE is the only way to achieve this with our current setup of MySQL.

From MySQL documentation

With ON DUPLICATE KEY UPDATE, the affected-rows value per row is 1 if the row is inserted as a new row, 2 if an existing row is updated, and 0 if an existing row is set to its current values. If you specify the CLIENT_FOUND_ROWS flag to the mysql_real_connect() C API function when connecting to mysqld, the affected-rows value is 1 (not 0) if an existing row is set to its current values.

The other way is to update all fields instead of ignore them but I failed to achieve this with Oracle. It's sad because it could make sense when we launch an update after the insertIgnoreConflict

@Altahrim
Copy link
Collaborator Author

@nickvergessen Do you think it is a correct solution given the risks of ignoring other errors?

@Altahrim Altahrim requested a review from come-nc June 19, 2024 13:02
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A unit test would be nice

@Altahrim Altahrim force-pushed the feat/mysql_ignore_conflics branch from 330d35b to b724368 Compare June 25, 2024 14:21
@Altahrim Altahrim enabled auto-merge June 26, 2024 07:42
@Altahrim Altahrim merged commit 2482688 into master Jun 27, 2024
@Altahrim Altahrim deleted the feat/mysql_ignore_conflics branch June 27, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants