embulk / embulk-output-jdbc

MySQL, PostgreSQL, Redshift and generic JDBC output plugins for Embulk
Other
88 stars 89 forks source link

Swap name + drop instead of drop + rename in MySQL replace mode #337

Closed rajyan closed 9 months ago

rajyan commented 9 months ago

Note

This is a re-implementation of https://github.com/embulk/embulk-output-jdbc/pull/336

with fixed based on the reviews by @hiroyuki-sato in https://github.com/embulk/embulk-output-jdbc/pull/336 applied (Thanks for the review.)

Description

The current implementation for replace mode

For databases that have transactional DDL queries, there are no problems with this operation.

Although, MySQL implicitly commits DROP TABLE and RENAME TABLE which can inadvertently drop the target table, if the RENAME TABLE fails, or the operation is aborted in the middle. https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html

This pull request aims to make the replace mode safer for MySQL by changing the operation to

With this change, even in case of failure, the target table would never be dropped.

Changes

https://dev.mysql.com/doc/refman/5.7/en/rename-table.html https://dev.mysql.com/doc/refman/8.0/en/rename-table.html

hiroyuki-sato commented 9 months ago

Basically LGTM👍 I'm asking co-maintainers about this PR. Please wait.

rajyan commented 9 months ago

@dmikurube

Do you have any more ideas that could be much better to guarantee the uniqueness? If we could use TEMPORARY TABLE in this context, it could be much more robust, but we may not be able to use it here...

This unique logic is basically copied from here https://github.com/rajyan/embulk-output-jdbc/blob/bf0448dae36ebb655424f85f250aaa0051cee99c/embulk-output-jdbc/src/main/java/org/embulk/output/jdbc/AbstractJdbcOutputPlugin.java#L789 , but I think we can think for a MySQL specific solution.

TEMPORARY TABLE doesn't work as you explained, and also requires additional CREATE TEMPORARY TABLES privilege, which I thought it would be a breaking change.

Also, we don't need to "create" a unique table, but only need to have a unique identifier.

One idea I have is using prepare statement with MySQL UUID() function to use it as a table name.

ex.

mysql> create table test1(col1 INT);
Query OK, 0 rows affected (0.03 sec)

mysql> create table test2(col2 INT);
Query OK, 0 rows affected (0.16 sec)

mysql> set @tmp_name = uuid();
Query OK, 0 rows affected (0.00 sec)

mysql> set @stmt = concat('RENAME TABLE `test1` to `', @tmp_name, '`, `test2` to `test1`, `', @tmp_name, '` to `
test2`')
    -> ;
Query OK, 0 rows affected (0.00 sec)

mysql> select @stmt;
+---------------------------------------------------------------------------------------------------------------------------------------+
| @stmt                                                                                                                                 |
+---------------------------------------------------------------------------------------------------------------------------------------+
| RENAME TABLE `test1` to `2bf3b5fb-cc0d-11ee-abc8-0242ac110002`, `test2` to `test1`, `2bf3b5fb-cc0d-11ee-abc8-0242ac110002` to `test2` |
+---------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> prepare stmt from @stmt;
Query OK, 0 rows affected (0.00 sec)
Statement prepared

mysql> execute stmt;
Query OK, 0 rows affected (0.03 sec)

mysql> desc test1;
+-------+---------+------+-----+---------+-------+
| Field | Type    | Null | Key | Default | Extra |
+-------+---------+------+-----+---------+-------+
| col2  | int(11) | YES  |     | NULL    |       |
+-------+---------+------+-----+---------+-------+
1 row in set (0.01 sec)

mysql> desc test2;
+-------+---------+------+-----+---------+-------+
| Field | Type    | Null | Key | Default | Extra |
+-------+---------+------+-----+---------+-------+
| col1  | int(11) | YES  |     | NULL    |       |
+-------+---------+------+-----+---------+-------+
1 row in set (0.01 sec)

wdyt about this?

dmikurube commented 9 months ago

I think we'll release v0.10.5 with this sooner, maybe at some time next week. Please let us know if you're in a hurry.

rajyan commented 9 months ago

Thank you! I'm not in a hurry, looking forward to the next release.