WIP:MySQL Directory Format #84

shivansh02 · 2024-08-08T11:45:16Z

Adds support for directory format for MySQL databases.

To Do:

Write new/modify unit tests
Modify E2E tests

shivansh02 · 2024-08-20T08:49:24Z

To test the feature, I tried running mysqldump on a database but got the following error:
The MySQL server is running with the --secure-file-priv option so it cannot execute this statement](https://stackoverflow.com/questions/74932524/mysql-error-code-1290-the-mysql-server-is-running-with-the-secure-file-priv)

To fix that, I added the following lines to mysql config files:

[mysqld]
secure-file-priv = ""

Now, the error I'm getting is
unknown variable 'secure-file-priv='
I removed the variable from all the config files I tried setting it in, the error still remains.
Tried to allow AppArmor according to this answer, but to no avail:
https://stackoverflow.com/a/57502632/20831469

witten · 2024-08-20T22:51:03Z

Maybe try secure_file_priv with underscores instead of dashes? Although these docs say an empty value has "no effect" which is not reassuring. You could also try setting it to the actual directory that you'll be dumping to.

witten

What do you think of making these changes to the MariaDB database hook as well, as it's pretty identical to the MySQL hook right now? The main difference is that the specific binaries it runs have different names.

witten · 2024-08-20T23:13:19Z

borgmatic/hooks/mysql.py

@@ -86,6 +86,10 @@ def execute_dump_command(
    mysql_dump_command = tuple(
        shlex.quote(part) for part in shlex.split(database.get('mysql_dump_command') or 'mysqldump')
    )
+
+    dump_format = database.get('format')


Style nit: This variable could easily inlined given that it's not used anywhere else besides the next line.

witten · 2024-08-20T23:16:01Z

borgmatic/hooks/mysql.py

+            extra_environment=extra_environment,
+        )
+        return None
+    else:


You could lose the else: given that the other code path returns early. (Do not feel strongly.)

witten · 2024-08-20T23:31:09Z

borgmatic/hooks/mysql.py

                    log_prefix,
                    dump_path,
-                    dump_database_names,
+                    (dump_name,),


I'm not quite following this changed and the earlier deletions. Don't you still need some of that logic if the database name is all?

witten · 2024-08-20T23:34:07Z

borgmatic/hooks/mysql.py

-            if 'restore_options' in data_source
-            else ()
+
+    dump_format = data_source.get('format')


Similar here; could be inlined.

witten · 2024-08-20T23:36:54Z

borgmatic/hooks/mysql.py

+            + (('--port', str(port)) if port else ())
+            + (('--protocol', 'tcp') if hostname or port else ())
+            + (('--user', username) if username else ())
+        )


I think I'm missing something.. These two commands look identical to me. Maybe it's just still WIP.

witten · 2024-08-20T23:46:26Z

borgmatic/hooks/mysql.py

+        )
+        for file in os.listdir(dump_directory):
+            file_path = os.path.join(dump_directory, file)
+            if file.endswith('.sql'):


The docs say these are .txt files...?

shivansh02 · 2024-08-21T13:56:47Z

What do you think of making these changes to the MariaDB database hook as well, as it's pretty identical to the MySQL hook right now? The main difference is that the specific binaries it runs have different names.

Yes I proposed making similar changes to MariaDB hook as well. I'll get on it as soon as I can get this PR approved, so it saves time.

shivansh02 · 2024-08-24T08:45:15Z

@witten I made changes that hopefully resolve all the issues above. Please take a look.

witten

Finally got around to giving this a look. Thank you so much for your patience! I'll try to be a little faster on the next round.

witten · 2024-08-29T16:22:17Z

borgmatic/hooks/mysql.py

@@ -61,27 +61,39 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run):
    )


+def ensure_directory_exists(path):
+    if not os.path.exists(path):
+        os.makedirs(path)


You can actually replace this whole function with a call to:

os.makedirs(path, exist_ok=True)

And that should have the same effect! More info in the docs: https://docs.python.org/3/library/os.html#os.makedirs

witten · 2024-08-29T16:32:38Z

borgmatic/hooks/mysql.py


-    if os.path.exists(dump_filename):
-        logger.warning(
-            f'{log_prefix}: Skipping duplicate dump of MySQL database "{database_name}" to {dump_filename}'


Should this duplicate-skipping logic also apply to directory-style dumps? They don't have a single filename, but they do have a dump dir that may or may not already exist. (Do not feel strongly.)

witten · 2024-08-29T16:38:34Z

borgmatic/hooks/mysql.py

+
+    if use_tab:
+        ensure_directory_exists(dump_path)
+        dump_dir = os.path.join(dump_path, database_name)


Is there a reason that dump.make_data_source_dump_filename() doesn't work for this purpose? That would have the benefit of using the standard directory layout that other dump formats use (~/.borgmatic/mysql_databases/[hostname]/[dbname]).

witten · 2024-08-29T16:50:31Z

borgmatic/hooks/mysql.py

@@ -96,16 +108,17 @@ def execute_dump_command(
        + (('--user', database['username']) if 'username' in database else ())
        + ('--databases',)
        + database_names
-        + ('--result-file', dump_filename)
+        + result_file_option


Makes sense!

witten · 2024-08-29T16:52:59Z

borgmatic/hooks/mysql.py

-    if os.path.exists(dump_filename):
-        logger.warning(
-            f'{log_prefix}: Skipping duplicate dump of MySQL database "{database_name}" to {dump_filename}'
+    use_tab = '--tab' in (database.get('options') or '')


I would expect this to look at the database's configured format instead. e.g. database.get('format') == 'directory' or whatever.

Maybe I'm not understanding the schema.

witten · 2024-08-29T17:45:38Z

borgmatic/hooks/mysql.py

    '''
-    return any(databases)
+    return not any('--tab' in (db.get('options') or '') for db in databases)


Similar question here as above. Shouldn't this use format instead of options? And again, let me know if I'm just missing something.

witten · 2024-08-29T17:54:34Z

borgmatic/hooks/mysql.py


-    Return a sequence of subprocess.Popen instances for the dump processes ready to spew to a named
-    pipe. But if this is a dry run, then don't actually dump anything and return an empty sequence.
+    Return a sequence of subprocess.Popen instances for the dump processes.


I wouldn't expect the directory format dumps to return any streaming Popen instances since presumably they won't stream! So that logic might need to change in this function for that case.

The existing PostgreSQL hook does something similar for the directory format.

witten · 2024-08-29T17:55:41Z

borgmatic/hooks/mysql.py

    restore_command = (
        mysql_restore_command
-        + ('--batch',)


Isn't this still needed/beneficial? We certainly don't want interactive behavior from mysql when it's invoked from borgmatic.

witten · 2024-08-29T17:57:10Z

borgmatic/hooks/mysql.py

+            return
+
+        for filename in os.listdir(dump_directory):
+            if filename.endswith('.sql'):


Again, I might just be reading this wrong, but shouldn't these be .txt files, not .sql files? https://dev.mysql.com/doc/refman/8.4/en/mysqldump.html#option_mysqldump_tab

witten · 2024-08-29T18:03:11Z

borgmatic/hooks/mysql.py

+                file_path = os.path.join(dump_directory, filename)
+                logger.debug(f"{log_prefix}: Restoring from {file_path}")
+                with open(file_path, 'r') as sql_file:
+                    execute_command_with_processes(


I don't think you want the _with_processes() variant for this use case. That's intended for passing in, say, an extract_process so as to extract a single file into a restore command via background streaming. But that's not what you're doing here; this is presumably a synchronous call (... it's not streaming from borg extract). So, for that, maybe try just execute_command()?

mysql directory format

c5ef93c

shivansh02 marked this pull request as draft August 8, 2024 11:45

shivansh02 added 2 commits August 9, 2024 20:39

mysql unit tests

e7b34ae

mysql file variable clarified

8b2647e

witten reviewed Aug 20, 2024

View reviewed changes

shivansh02 closed this Aug 21, 2024

shivansh02 reopened this Aug 21, 2024

shivansh02 added 2 commits August 21, 2024 19:32

restore logic updated and variables inlined

78267ab

Skipping duplicate dump error fixed

171e9eb

shivansh02 requested a review from witten August 22, 2024 19:44

witten requested changes Aug 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP:MySQL Directory Format #84

WIP:MySQL Directory Format #84

shivansh02 commented Aug 8, 2024

shivansh02 commented Aug 20, 2024

witten commented Aug 20, 2024

witten left a comment

witten Aug 20, 2024

witten Aug 20, 2024

witten Aug 20, 2024

witten Aug 20, 2024

witten Aug 20, 2024

witten Aug 20, 2024 •

edited

Loading

shivansh02 commented Aug 21, 2024

shivansh02 commented Aug 24, 2024

witten left a comment

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

witten Aug 29, 2024

WIP:MySQL Directory Format #84

Are you sure you want to change the base?

WIP:MySQL Directory Format #84

Conversation

shivansh02 commented Aug 8, 2024

shivansh02 commented Aug 20, 2024

witten commented Aug 20, 2024

witten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witten Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

shivansh02 commented Aug 21, 2024

shivansh02 commented Aug 24, 2024

witten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witten Aug 20, 2024 •

edited

Loading