docker-library / mysql

Docker Official Image packaging for MySQL Community Server
https://dev.mysql.com/
GNU General Public License v2.0
2.46k stars 2.19k forks source link

Support running server with autocommit #1065

Closed LaurentGoderre closed 3 months ago

LaurentGoderre commented 4 months ago

Forces initiation queries to run in autocommit then use the provided mode

Fixed #816

LaurentGoderre commented 4 months ago

After the fix

> docker build -t mysql:8 -f Dockerfile.oracle --platform linux/amd64 . > /dev/null 2>&1                          
> docker run -d --name test --rm -it -e MYSQL_RANDOM_ROOT_PASSWORD=1 --platform linux/amd64 mysql:8 --autocommit=0
> docker logs test                                                                                                
[...]
2024-05-30 20:23:20+00:00 [Note] [Entrypoint]: MySQL init process done. Ready for start up.
> docker exec -it test mysql -p
Enter password: 
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 8
Server version: 8.0.37 MySQL Community Server - GPL

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> SHOW VARIABLES LIKE '%autocommit%';
+---------------+-------+
| Variable_name | Value |
+---------------+-------+
| autocommit    | OFF   |
+---------------+-------+
1 row in set (0.04 sec)
tianon commented 3 months ago

Doh, sorry, this one got lost -- I worry that this "leaks" to users since their scripts get run in this environment too, so I think I'd prefer if we change our one or two scripts to instead enable autocommit (since IIRC there's a trivial one-line way to do so for a single session). It might also be worth trying to make sure we minimize the number of queries we actually make, but maybe we've already done that. :see_no_evil:

LaurentGoderre commented 3 months ago

@tianon sadly no you can't use that one liner in a session (or I didn't get it working after trying many ways). I tested the change and posted the result and it respected the flag that was passed.

tianon commented 3 months ago

Wow, that's bizarre -- I've confirmed, every way I can find or think of to SET autocommit = 1 (with or without SESSION qualifiers) and even adding an explicit COMMIT still ends up with Cannot modify @@session.sql_log_bin inside a transaction. :sob:

That's wild, and seems unintentional? Effectively this means it must be impossible to set autocommit to off by default, but turn it back on for a single session, which isn't a limitation that's documented anywhere I can find. :thinking:

tianon commented 3 months ago

To clarify though, my concern is not that the final runtime environment is correct (that much is clear from your change), but that the intermediate environment in which the user-supplied initdb scripts get invoked is also disabling autocommit by default (because I think that's part of the "user intent" expressed by passing this flag by default globally).

tianon commented 3 months ago

Now I'm questioning my testing methodology entirely though, because even deleting the SQL_LOG_BIN bit from the script completely still gives the exact same Cannot modify @@session.sql_log_bin inside a transaction. error. :thinking:

LaurentGoderre commented 3 months ago

@tianon glad it's not just me! I spent a while trying to get it work that way

tianon commented 3 months ago

Hahahahahahahahahahaha: https://github.com/mysql/mysql-server/commit/7dbf4f80ed15f3c925cfb2b834142f23a2de719a / https://bugs.mysql.com/bug.php?id=110535

Turns out this was a bug in --initialize-insecure that still exists in MySQL 8.0 but was fixed in 8.2+ (@yosifkit was trying to gaslight me saying it works fine but he was testing 8.4 and I was testing 8.0, but that provided the clue that helped find the upstream code).

tianon commented 3 months ago
diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh
index 8cb17c4..e635e07 100755
--- a/docker-entrypoint.sh
+++ b/docker-entrypoint.sh
@@ -214,7 +214,8 @@ docker_create_db_directories() {
 # initializes the database directory
 docker_init_database_dir() {
    mysql_note "Initializing database files"
-   "$@" --initialize-insecure --default-time-zone=SYSTEM
+   "$@" --initialize-insecure --default-time-zone=SYSTEM --autocommit=1
+   # explicitly enable autocommit to combat https://bugs.mysql.com/bug.php?id=110535 (TODO remove this when 8.0 is EOL; see https://github.com/mysql/mysql-server/commit/7dbf4f80ed15f3c925cfb2b834142f23a2de719a)
    mysql_note "Database files initialized"
 }

@@ -292,6 +293,9 @@ docker_setup_db() {

    # tell docker_process_sql to not use MYSQL_ROOT_PASSWORD since it is just now being set
    docker_process_sql --dont-use-mysql-root-password --database=mysql <<-EOSQL
+       -- enable autocommit explicitly (in case it was disabled globally)
+       SET autocommit = 1;
+
        -- What's done in this file shouldn't be replicated
        --  or products like mysql-fabric won't work
        SET @@SESSION.SQL_LOG_BIN=0;
LaurentGoderre commented 3 months ago

Done!

yosifkit commented 3 months ago

I thought we might've missed the other docker_process_sql statements, but CREATE and GRANT statements already implicitly commit and mysql_tzinfo_to_sql ends its output with an explicit COMMIT;.

https://github.com/docker-library/mysql/blob/1a703318803fa85bf69cb5d2e5b3f1c4087627b5/docker-entrypoint.sh#L266-L269 https://github.com/docker-library/mysql/blob/1a703318803fa85bf69cb5d2e5b3f1c4087627b5/docker-entrypoint.sh#L311-L324