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

Support running server with autocommit #1065

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

LaurentGoderre
Copy link
Member

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

Fixed #816

@LaurentGoderre
Copy link
Member Author

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)

@LaurentGoderre LaurentGoderre requested a review from tianon June 17, 2024 15:23
@tianon
Copy link
Member

tianon commented Jun 17, 2024

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. 🙈

@LaurentGoderre
Copy link
Member Author

@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
Copy link
Member

tianon commented Jun 17, 2024

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. 😭

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. 🤔

@tianon
Copy link
Member

tianon commented Jun 17, 2024

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
Copy link
Member

tianon commented Jun 17, 2024

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. 🤔

@LaurentGoderre
Copy link
Member Author

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

@tianon
Copy link
Member

tianon commented Jun 17, 2024

Hahahahahahahahahahaha: mysql/mysql-server@7dbf4f8 / 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
Copy link
Member

tianon commented Jun 17, 2024

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;

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

Fixed docker-library#816
@LaurentGoderre
Copy link
Member Author

Done!

@tianon tianon merged commit 1a70331 into docker-library:master Jun 18, 2024
5 checks passed
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Jun 18, 2024
Changes:

- docker-library/mysql@1a70331: Merge pull request docker-library/mysql#1065 from LaurentGoderre/fix-816
- docker-library/mysql@319db56: Support running server with autocommit
@yosifkit
Copy link
Member

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;.

mysql/docker-entrypoint.sh

Lines 266 to 269 in 1a70331

mysql_tzinfo_to_sql /usr/share/zoneinfo \
| sed 's/Local time zone must be set--see zic manual page/FCTY/' \
| docker_process_sql --dont-use-mysql-root-password --database=mysql
# tell docker_process_sql to not use MYSQL_ROOT_PASSWORD since it is not set yet

mysql/docker-entrypoint.sh

Lines 311 to 324 in 1a70331

if [ -n "$MYSQL_DATABASE" ]; then
mysql_note "Creating database ${MYSQL_DATABASE}"
docker_process_sql --database=mysql <<<"CREATE DATABASE IF NOT EXISTS \`$MYSQL_DATABASE\` ;"
fi
if [ -n "$MYSQL_USER" ] && [ -n "$MYSQL_PASSWORD" ]; then
mysql_note "Creating user ${MYSQL_USER}"
docker_process_sql --database=mysql <<<"CREATE USER '$MYSQL_USER'@'%' IDENTIFIED BY '$MYSQL_PASSWORD' ;"
if [ -n "$MYSQL_DATABASE" ]; then
mysql_note "Giving user ${MYSQL_USER} access to schema ${MYSQL_DATABASE}"
docker_process_sql --database=mysql <<<"GRANT ALL ON \`${MYSQL_DATABASE//_/\\_}\`.* TO '$MYSQL_USER'@'%' ;"
fi
fi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mysql:8.0.28: cannot boot server with autocommit=0
3 participants