Skip to content

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

Open
DrashtiChhatralia wants to merge 1 commit intotransferwise:masterfrom
DrashtiChhatralia:1269_MySQL_compatibility_issue
Open

TAP-MYSQL: Fix MySQL 8.4 compatibility for binlog status retrieval#1273
DrashtiChhatralia wants to merge 1 commit intotransferwise:masterfrom
DrashtiChhatralia:1269_MySQL_compatibility_issue

Conversation

@DrashtiChhatralia
Copy link
Copy Markdown

Context

MySQL 8.4 removes support for the SHOW MASTER STATUS command, which is currently used in tap-mysql to fetch binlog file and position. This causes failures when running against newer MySQL versions.

This PR updates the implementation to first attempt SHOW BINARY LOG STATUS (supported in MySQL 8.4+) and fall back to SHOW MASTER STATUS for backward compatibility. This ensures the tap works across both newer and older MySQL versions without breaking existing behavior.

Types of changes

What types of changes does your code introduce to PipelineWise?

  • Bugfix (non-breaking change which fixes an issue) [optional]

Checklist

  • I have read the CONTRIBUTING doc
  • Description above provides context of the change
  • I have added tests that prove my fix is effective or that my feature works
  • Unit tests for changes (not needed for documentation changes)
  • CI checks pass with my changes

@DrashtiChhatralia DrashtiChhatralia marked this pull request as ready for review April 17, 2026 16:47
@DrashtiChhatralia DrashtiChhatralia requested a review from a team as a code owner April 17, 2026 16:47
Copilot AI review requested due to automatic review settings April 17, 2026 16:47
@platon-github-app-production
Copy link
Copy Markdown

Comment /request-review to automatically request reviews from the following teams:

You can also request review from a specific team by commenting /request-review team-name, or you can add a description with --notes "<message>"

💡 If you see something that doesn't look right, check the configuration guide.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Changes: Bugfix (1), Test improvement (1)

Updates tap-mysql binlog status retrieval to work with MySQL 8.4+ (where SHOW MASTER STATUS is removed) while preserving backward compatibility with older MySQL versions.

Changes:

  • Prefer SHOW BINARY LOG STATUS when retrieving current binlog file/position, with fallback to SHOW MASTER STATUS.
  • Add unit tests covering both the primary path and the fallback path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
singer-connectors/tap-mysql/tap_mysql/sync_strategies/binlog.py Switch binlog status query to SHOW BINARY LOG STATUS with fallback logic.
singer-connectors/tap-mysql/tests/unit/sync_strategies/test_binlog.py Add/adjust unit tests for the new primary query and fallback behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1827 to +1831
cur_mock.__enter__.return_value.execute.assert_has_calls(
[
call('SHOW BINARY LOG STATUS'),
]
)
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.
Comment on lines 1881 to +1885
cur_mock.__enter__.return_value.execute.assert_has_calls(
[
call('SHOW BINARY LOG STATUS'),
]
)
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.
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.
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.

2 participants