Skip to content

Fix read_only scan error on MariaDB via CASE-WHEN string check #121

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.

Sign up for GitHub

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

Merged
renecannao merged 2 commits into from
Aug 11, 2026
Merged

Fix read_only scan error on MariaDB via CASE-WHEN string check #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2b0e5f
Fix read_only scan error on MariaDB via CASE-WHEN string check
ahmetsoguksu Aug 7, 2026
00cf254
test(inst): cover MariaDB read_only modes
renecannao Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .github/workflows/functional.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ jobs:
strategy:
fail-fast: false
matrix:
mariadb_version: ['10.6', '10.11']
mariadb_version: ['10.6', '10.11', '12.3']

steps:
- uses: actions/checkout@v4
Expand Down Expand Up @@ -274,6 +274,12 @@ jobs:
COMPOSE: docker compose -f tests/functional/docker-compose.yml -f tests/functional/docker-compose.mariadb.yml
run: bash tests/functional/test-smoke.sh

- name: Run read_only discovery tests
env:
MYSQL_IMAGE: mariadb:${{ matrix.mariadb_version }}
COMPOSE: docker compose -f tests/functional/docker-compose.yml -f tests/functional/docker-compose.mariadb.yml
run: bash tests/functional/test-mariadb-read-only.sh

- name: Run relay drain / SQL-stopped failover tests
env:
MYSQL_IMAGE: mariadb:${{ matrix.mariadb_version }}
Expand Down
2 changes: 1 addition & 1 deletion go/inst/instance_dao.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,7 @@ func ReadTopologyInstanceBufferable(instanceKey *InstanceKey, bufferWrites bool,

// Synchronously query for some params needed in following go routines
var mysqlHostname, mysqlReportHost string
err = db.QueryRow("select @@global.hostname, ifnull(@@global.report_host, ''), @@global.server_id, @@global.version_comment, @@global.read_only, @@global.binlog_format, @@global.log_bin, @@global."+instance.QSP.log_slave_updates()).Scan(
err = db.QueryRow("select @@global.hostname, ifnull(@@global.report_host, ''), @@global.server_id, @@global.version_comment, CASE WHEN UPPER(CAST(@@global.read_only AS CHAR)) IN ('0','OFF') THEN 0 ELSE 1 END, @@global.binlog_format, @@global.log_bin, @@global."+instance.QSP.log_slave_updates()).Scan(

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 'read_only|NO_LOCK|NO_LOCK_NO_ADMIN|ReadOnly' go tests .github

Repository: ProxySQL/orchestrator

Length of output: 50378

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- target query ---'
sed -n '650,695p' go/inst/instance_dao.go

printf '%s\n' '--- focused functional test ---'
sed -n '1,240p' tests/functional/test-mariadb-read-only.sh

printf '%s\n' '--- relevant repository diff summary ---'
git diff --stat
git status --short

printf '%s\n' '--- focused references ---'
rg -n -C 5 'read_only|NO_LOCK|NO_LOCK_NO_ADMIN|@@global.read_only' \
  tests/functional go/inst/instance_dao.go go/inst/instance_dao_test.go

Repository: ProxySQL/orchestrator

Length of output: 50377

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail
sed -n '650,695p' go/inst/instance_dao.go
printf '\n--- test names and read-only assertions ---\n'
rg -n -C 8 'read.only|NO_LOCK|NO_LOCK_NO_ADMIN|ReadOnly' tests/functional go/inst/instance_dao_test.go
printf '\n--- diff summary ---\n'
git diff --stat
git status --short

Repository: ProxySQL/orchestrator

Length of output: 50377

🌐 Web query:

MariaDB read_only NO_LOCK NO_LOCK_NO_ADMIN supported versions official documentation

💡 Result:

The options READ_ONLY_NO_LOCK and READ_ONLY_NO_LOCK_NO_ADMIN are part of an extension to the MariaDB read_only system variable introduced in MDEV-36425, which is associated with upcoming MariaDB 12.x versions [1][2]. These options provide more granular control over the read_only behavior: 1. READ_ONLY (or 1): Blocks changes for users without the 'READ ONLY ADMIN' privilege [1]. 2. READ_ONLY_NO_LOCK (or 2): In addition to blocking changes for non-admin users, it also blocks LOCK TABLES and SELECT IN SHARE MODE [1][3]. 3. READ_ONLY_NO_LOCK_NO_ADMIN (or 3): In addition to the above, it further restricts these operations even for users holding the 'READ ONLY ADMIN' privilege [1]. These settings were developed to allow for more robust restriction of operations on replicas to ensure they remain identical to the primary server by preventing unexpected blocking or modifications [1][4]. As these are recent additions for MariaDB 12.x, they may not be fully documented in older stable version manuals [2].

Citations:

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- MariaDB versions in CI ---'
rg -n -C 3 'mariadb_version|MYSQL_IMAGE: mariadb' .github tests
printf '%s\n' '--- focused test references ---'
rg -n -C 6 'read.only|NO_LOCK|NO_LOCK_NO_ADMIN|ReadOnly' tests/functional go/inst/instance_dao_test.go go/inst/instance_dao.go

Repository: ProxySQL/orchestrator

Length of output: 50377

Make the fallback explicit.

The current CASE treats every value except 0 and OFF as read-only. Use an allowlist so unknown values default to writable:

Suggested CASE expression 
- CASE WHEN UPPER(CAST(@@global.read_only AS CHAR)) IN ('0','OFF') THEN 0 ELSE 1 END,
+ CASE WHEN UPPER(CAST(@@global.read_only AS CHAR)) IN ('1','ON','NO_LOCK','NO_LOCK_NO_ADMIN') THEN 1 ELSE 0 END,

The existing functional test already covers OFF, ON, and MariaDB 12+ NO_LOCK modes.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
err = db.QueryRow("select @@global.hostname, ifnull(@@global.report_host, ''), @@global.server_id, @@global.version_comment, CASE WHEN UPPER(CAST(@@global.read_only AS CHAR)) IN ('0','OFF') THEN 0 ELSE 1 END, @@global.binlog_format, @@global.log_bin, @@global."+instance.QSP.log_slave_updates()).Scan(
err = db.QueryRow("select @@global.hostname, ifnull(@@global.report_host, ''), @@global.server_id, @@global.version_comment, CASE WHEN UPPER(CAST(@@global.read_only AS CHAR)) IN ('1','ON','NO_LOCK','NO_LOCK_NO_ADMIN') THEN 1 ELSE 0 END, @@global.binlog_format, @@global.log_bin, @@global."+instance.QSP.log_slave_updates()).Scan(
🧰 Tools
🪛 OpenGrep (1.26.0)

[ERROR] 676-676: SQL query built via fmt.Sprintf or string concatenation passed to a database method. Use parameterized queries with placeholder arguments.

(coderabbit.sql-injection.go-query-format)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@go/inst/instance_dao.go` at line 676, Update the CASE expression in the
instance DAO query to use an explicit allowlist for read-only values, treating
only recognized read-only modes as read-only and defaulting unknown values to
writable. Preserve the existing handling for OFF, ON, and MariaDB 12+ NO_LOCK
modes in the QueryRow scan.

&mysqlHostname, &mysqlReportHost, &instance.ServerID, &instance.VersionComment, &instance.ReadOnly, &instance.Binlog_format, &instance.LogBinEnabled, &instance.LogReplicationUpdatesEnabled)
if err != nil {
goto Cleanup
Expand Down
14 changes: 14 additions & 0 deletions tests/functional/docker-compose.mariadb.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,31 @@
# Override for MariaDB functional tests (use with docker-compose.yml)
# MYSQL_IMAGE=mariadb:10.11 docker compose -f docker-compose.yml -f docker-compose.mariadb.yml up -d
x-mariadb-client-compat: &mariadb-client-compat
command: mariadbd
entrypoint:
- /bin/sh
- -c
- |
ln -sf /usr/bin/mariadb /usr/local/bin/mysql
ln -sf /usr/bin/mariadb-admin /usr/local/bin/mysqladmin
exec docker-entrypoint.sh "$@"
- --

services:
mysql1:
<<: *mariadb-client-compat
image: ${MYSQL_IMAGE:-mariadb:10.11}
volumes:
- ./mysql/mariadb-master.cnf:/etc/mysql/conf.d/repl.cnf
- ./mysql/init-master.sql:/docker-entrypoint-initdb.d/init.sql
mysql2:
<<: *mariadb-client-compat
image: ${MYSQL_IMAGE:-mariadb:10.11}
volumes:
- ./mysql/mariadb-replica.cnf:/etc/mysql/conf.d/repl.cnf
- ./mysql/init-replica.sql:/docker-entrypoint-initdb.d/init.sql
mysql3:
<<: *mariadb-client-compat
image: ${MYSQL_IMAGE:-mariadb:10.11}
volumes:
- ./mysql/mariadb-replica2.cnf:/etc/mysql/conf.d/repl.cnf
Expand Down
65 changes: 65 additions & 0 deletions tests/functional/test-mariadb-read-only.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#!/bin/bash
# Verify MariaDB read_only enum values are normalized correctly by discovery.
set -uo pipefail
cd "$(dirname "$0")/../.." || exit 1
source tests/functional/lib.sh

echo "=== MARIADB READ_ONLY DISCOVERY TESTS ==="

COMPOSE="${COMPOSE:-docker compose -f tests/functional/docker-compose.yml -f tests/functional/docker-compose.mariadb.yml}"
TEST_INSTANCE="mysql2"

restore_read_only() {
$COMPOSE exec -T "$TEST_INSTANCE" mysql -uroot -ptestpass \
-e "SET GLOBAL read_only=ON" >/dev/null 2>&1 || true
curl -s --max-time 10 "$ORC_URL/api/discover/$TEST_INSTANCE/3306" >/dev/null 2>&1 || true
}
trap restore_read_only EXIT

wait_for_orchestrator || { echo "FATAL: Orchestrator not reachable"; exit 1; }
discover_topology "mysql1" || { echo "FATAL: Topology not discovered"; exit 1; }

wait_for_api_read_only() {
local EXPECTED="$1"
local ACTUAL=""
for _ in $(seq 1 20); do
curl -s --max-time 10 "$ORC_URL/api/discover/$TEST_INSTANCE/3306" >/dev/null 2>&1
ACTUAL=$(curl -s --max-time 10 "$ORC_URL/api/instance/$TEST_INSTANCE/3306" 2>/dev/null | python3 -c \
"import json,sys; print(str(json.load(sys.stdin).get('ReadOnly')).lower())" 2>/dev/null || echo "")
if [ "$ACTUAL" = "$EXPECTED" ]; then
return 0
fi
sleep 1
done
echo "last API ReadOnly value: ${ACTUAL:-unavailable}"
return 1
}

check_mode() {
local MODE="$1"
local EXPECTED="$2"
if ! $COMPOSE exec -T "$TEST_INSTANCE" mysql -uroot -ptestpass \
-e "SET GLOBAL read_only=$MODE" >/dev/null 2>&1; then
fail "MariaDB rejected read_only=$MODE"
return
fi
if wait_for_api_read_only "$EXPECTED"; then
pass "read_only=$MODE is reported as ReadOnly=$EXPECTED"
else
fail "read_only=$MODE was not reported as ReadOnly=$EXPECTED"
fi
}

check_mode OFF false
check_mode ON true

MARIADB_MAJOR="$(mysql_version)"
MARIADB_MAJOR="${MARIADB_MAJOR%%.*}"
if [ "$MARIADB_MAJOR" -ge 12 ]; then
check_mode NO_LOCK true
check_mode NO_LOCK_NO_ADMIN true
else
skip "NO_LOCK modes require MariaDB 12 or newer"
fi

summary
Loading