Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dbal): insert ignore conflict method for MySQL and SQLite #45655

Merged
merged 2 commits into from
Jun 27, 2024

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, Fenn-CS 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
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

Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
@Altahrim Altahrim merged commit 2482688 into master Jun 27, 2024
165 checks passed
@Altahrim Altahrim deleted the feat/mysql_ignore_conflics branch June 27, 2024 09:19
@blizzz blizzz mentioned this pull request Jul 24, 2024
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.

5 participants