Skip to content

TAP-MYSQL: Fix MySQL 8.4 compatibility for binlog status retrieval #1273

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

Open
DrashtiChhatralia wants to merge 1 commit into
base: master
Choose a base branch
from
Open

TAP-MYSQL: Fix MySQL 8.4 compatibility for binlog status retrieval #1273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion singer-connectors/tap-mysql/tap_mysql/sync_strategies/binlog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ def verify_gtid_config(mysql_conn: MySQLConnection):
def fetch_current_log_file_and_pos(mysql_conn):
with connect_with_backoff(mysql_conn) as open_conn:
with open_conn.cursor() as cur:
cur.execute("SHOW MASTER STATUS")
try:
cur.execute("SHOW BINARY LOG STATUS")
except pymysql.err.ProgrammingError:
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

The fallback currently triggers on any pymysql.err.ProgrammingError, which can hide real problems (e.g., permission/privilege errors, server-side SQL mode issues) by logging a misleading “not supported” warning and then attempting a different statement. Consider catching the exception as ex and only falling back for the specific “syntax/unsupported” case(s) (e.g., MySQL error 1064 or the specific message), otherwise re-raise so genuine configuration/auth issues fail fast with the original error.

Suggested change
except pymysql.err.ProgrammingError:
except pymysql.err.ProgrammingError as ex:
error_code = ex.args[0] if ex.args else None
error_message = str(ex.args[1]).lower() if len(ex.args) > 1 else str(ex).lower()
is_unsupported_syntax = error_code == 1064 or (
"you have an error in your sql syntax" in error_message
or "not supported" in error_message
)
if not is_unsupported_syntax:
raise

Copilot uses AI. Check for mistakes.
LOGGER.warning("SHOW BINARY LOG STATUS not supported, falling back to SHOW MASTER STATUS")
cur.execute("SHOW MASTER STATUS")

result = cur.fetchone()

Expand Down
63 changes: 62 additions & 1 deletion singer-connectors/tap-mysql/tests/unit/sync_strategies/test_binlog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from unittest import TestCase
from unittest.mock import patch, Mock, call, MagicMock

from pymysql import InternalError
from pymysql import InternalError, ProgrammingError
from pymysql.cursors import Cursor
from pymysqlreplication.constants import FIELD_TYPE
from pymysqlreplication.event import RotateEvent, MariadbGtidEvent, GtidEvent
Expand Down Expand Up @@ -1826,6 +1826,36 @@ def test_fetch_current_log_file_and_pos_success(self, connect_with_backoff):
connect_with_backoff.assert_called_with(mysql_con)
cur_mock.__enter__.return_value.execute.assert_has_calls(
[
call('SHOW BINARY LOG STATUS'),
]
)
Comment on lines 1827 to +1831
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

This assertion only checks that SHOW BINARY LOG STATUS was called at least once; it doesn’t fail if the implementation unexpectedly falls back to SHOW MASTER STATUS as well (extra calls are allowed by assert_has_calls). To make the test protect the non-fallback path, assert the execute call list/call count matches exactly one call (or use assert_called_once_with).

Suggested change
cur_mock.__enter__.return_value.execute.assert_has_calls(
[
call('SHOW BINARY LOG STATUS'),
]
)
cur_mock.__enter__.return_value.execute.assert_called_once_with('SHOW BINARY LOG STATUS')

Copilot uses AI. Check for mistakes.

@patch('tap_mysql.sync_strategies.binlog.connect_with_backoff')
def test_fetch_current_log_file_and_pos_success_fallback(self, connect_with_backoff):
mysql_con = MagicMock(spec_set=MySQLConnection).return_value
cur_mock = MagicMock(spec_set=Cursor).return_value

def execute_side_effect(query):
if query == 'SHOW BINARY LOG STATUS':
raise ProgrammingError(1064, "You have an error in your SQL syntax...")

cur_mock.__enter__.return_value.execute.side_effect = execute_side_effect
cur_mock.__enter__.return_value.fetchone.side_effect = [
['binlog.000033', 345, ''],
]

mysql_con.__enter__.return_value.cursor.return_value = cur_mock

connect_with_backoff.return_value = mysql_con

result = binlog.fetch_current_log_file_and_pos(mysql_con)

self.assertEqual(result, ('binlog.000033', 345))

connect_with_backoff.assert_called_with(mysql_con)
cur_mock.__enter__.return_value.execute.assert_has_calls(
[
call('SHOW BINARY LOG STATUS'),
call('SHOW MASTER STATUS'),
]
)
Expand All @@ -1850,6 +1880,37 @@ def test_fetch_current_log_file_and_pos_fail_if_no_result(self, connect_with_bac
connect_with_backoff.assert_called_with(mysql_con)
cur_mock.__enter__.return_value.execute.assert_has_calls(
[
call('SHOW BINARY LOG STATUS'),
]
)
Comment on lines 1881 to +1885
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

Same as the success case: this assert_has_calls allows additional execute calls (including an unintended fallback to SHOW MASTER STATUS) without failing. Tighten the assertion to ensure the non-fallback path executes exactly the expected statement(s) so the test can catch regressions in fallback behavior.

Copilot uses AI. Check for mistakes.

@patch('tap_mysql.sync_strategies.binlog.connect_with_backoff')
def test_fetch_current_log_file_and_pos_fail_if_no_result_fallback(self, connect_with_backoff):
mysql_con = MagicMock(spec_set=MySQLConnection).return_value
cur_mock = MagicMock(spec_set=Cursor).return_value

def execute_side_effect(query):
if query == 'SHOW BINARY LOG STATUS':
raise ProgrammingError(1064, "You have an error in your SQL syntax...")

cur_mock.__enter__.return_value.execute.side_effect = execute_side_effect
cur_mock.__enter__.return_value.fetchone.side_effect = [
None
]

mysql_con.__enter__.return_value.cursor.return_value = cur_mock

connect_with_backoff.return_value = mysql_con

with self.assertRaises(Exception) as context:
binlog.fetch_current_log_file_and_pos(mysql_con)

self.assertEqual('MySQL binary logging is not enabled.', str(context.exception))

connect_with_backoff.assert_called_with(mysql_con)
cur_mock.__enter__.return_value.execute.assert_has_calls(
[
call('SHOW BINARY LOG STATUS'),
call('SHOW MASTER STATUS'),
]
)
Expand Down
Loading