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

[MDEV-34614] mysqlbinlog warn on EOF before GTID in --stop-position #3600

Open
wants to merge 3 commits into
base: 10.11
Choose a base branch
from

Conversation

ParadoxV5
Copy link
Contributor

Description

This PR adds mariadb-binlog warnings for --stop-position GTIDs that were not reached at EOF, mainly as a follow-up to MDEV-27037 which added warnings for unreached --stop-position file indices and --stop-datetimes.

What problem is the patch trying to solve?

--stop-position warnings inform possible mistakes in the input; they’re helpful for both DBAs on CLI and scripts/wrappers that check/log the output status.

MDEV-34614 enchances MDEV-27037 with per-domain GTID validation.
MDEV-27037 was considered a bug fix and so built upon the oldest supported branch (at the time) for forwardporting, but GTID range selection wasn’t a feature of its ancient base version.

Unlike #3400, the code behind this feature is in the rpl_gtid.cc class Binlog_gtid_state_validator rather than directly in the mysqlbinlog.cc program itself via its filter inputs.
There are two limitations current situations that lead to this decision:

  • Binlog_gtid_state_validator is already prompting unreached --start-positions in --gtid-strict-mode (errors) or -vvv mode (warnings).
    This may rather be a side effect. (not certain; I’m new 👶)
  • Both MDEV-34614 and MDEV-27037 are considered bug fixes that will apply on older supported versions. Such tickets should have minimal changes and no refactorings.
    The GITD range filter doesn’t have an API for enumerating statuses of individual --stop-position GTIDs, whereas modifying Binlog_gtid_state_validator only requires one function to gain some optional parameters.

A “new feature” PR with unrestrained rewrites would have a more ideal Separation of Concerns, such as:

  • Filters preselect which entries (row or transaction depending on the format) are processed.
  • The GTID validator doesn’t care about ranges; it only validates entries (e.g., confirming GITD order) within the range given by the filters.
  • The program uses the GTID filters – not the GTID validator – to warn out-of-range GTIDs to match the logic and error message of position & time (MDEV-27037).
  • For parity, the program warns out-of-range --start-position/datetimes regardless of --gtid-strict-mode/-vvv, not just --stop-position/datetimes.

Do you think this patch might introduce side-effects in other parts of the server?

To enable the Binlog_gtid_state_validator-based design, I had to (temporarily?) remove the optimization that recycles the validator object at the first (encounter order) --start-position GTID outside of --gtid-strict-mode/-vvv.

server/client/mysqlbinlog.cc

Lines 1130 to 1151 in 0e8fb97

/*
Where we always ensure the initial binlog state is valid, we only
continually monitor the GTID stream for validity if we are in GTID
strict mode (for errors) or if three levels of verbosity is provided
(for warnings).
If we don't care about ensuring GTID validity, just delete the auditor
object to disable it for future checks.
*/
if (gtid_state_validator && print_event_info->is_event_group_active())
{
if (!(opt_gtid_strict_mode || verbose >= 3))
{
delete gtid_state_validator;
/*
Explicitly reset to NULL to simplify checks on if auditing is enabled
i.e. if it is defined, assume we want to use it
*/
gtid_state_validator= NULL;
}
else

Note that despite this, this latter (runtime order) code queries its states again (if it’s not deleted), specifically to confirm the expectation of reaching (start) GTID states.

server/client/mysqlbinlog.cc

Lines 3732 to 3741 in 0e8fb97

/*
Ensure the GTID state is correct. If not, end in error.
Note that in gtid strict mode, we will not report here if any invalid GTIDs
are processed because it immediately errors (i.e. retval will be
ERROR_STOP)
*/
if (retval != ERROR_STOP && gtid_state_validator &&
gtid_state_validator->report(stderr, opt_gtid_strict_mode))
retval= ERROR_STOP;

Consequently, outside of --gtid-strict-mode and -vvv, the only case that enters this line is when EOF is found before every --start-position GTID (rather than any).
(Recall that sequences of each domain have individual --start-positions, just that file logs are concurrent.)
One of these two designs has to be erroneous.

Optional parameters preserve the current (non-)behaviors of (non-)reporting.
Early exits from --gtid-strict-mode exceptions also do (should?) not prompt unreached --start-positions since these states may be natural for those. (We’re logging unreachability, not reachability.)

Release Notes

What does #3400 say for its Release Notes?… Nothing? 🤷

Here’s a summary that covers both this and the previous PR:

mariadb-binlog now warns about any --stop-positions and --stop-datetimes that didn’t apply after reading the entire log.

How can this PR be tested?

The new test binlog_mysqlbinlog_warn_stop_gtid covers positive expectations (but not negative testing yet), though QA should run the entire binlog suite to also confirm no regressions.

PR quality check

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.
  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

…e stop-condition

* `binlog_mysqlbinlog_warn_stop_position`:
  Name the temporary output redirect as ignored to match
  `binlog_mysqlbinlog_warn_stop_datetime`
  * #3400 (comment)
* `binlog_mysqlbinlog_warn_stop_datetime`:
  Remove duplicate case
Previously, if `--gtid-strict-mode` are `-vvv` are not active,
it’s recycled upon entering the first `--start-position` GTID range.
However, a latter code queries its states again (if it’s not deleted),
specifically to confirm the expectation of reaching (start) GTID states.

This commit (temporarily?) removes this *optimization* so that this
validator and its latest states are consistently accessible at the end.
It also adds an optional parameter to preserve the current
(non-)behavior of out-of-order (non-)reporting.
This commit adds warnings for `--stop-position` GTIDs that
were not reached at EOF, mainly as a follow-up to
MDEV-27037 Mysqlbinlog should output a warning if EOF is found before its stop condition
(#3400).

`--stop-position` warnings inform possible mistakes in the input,
especially for progress reporting of scripts/wrappers.
MDEV-34614 enhances MDEV-27037 with individualized GTID validation, for
GTID range selection weren’t in all versions that MDEV-27037 targeted.

Unlike #3400, `Binlog_gtid_state_validator` provides the code behind
this feature rather than `mysqlbinlog` itself.
Motivations behind this decision are:
* For a patch that’ll apply on older versions,
  this choice yields a smaller change footprint.
* `Binlog_gtid_state_validator` is already prompting unreached
  *`--start-position`s* for `--gtid-strict-mode`/`-vvv`.
@ParadoxV5
Copy link
Contributor Author

PR # 60²

--echo # MDEV-34614 mysqlbinlog warn on EOF before GTID in --stop-position
--echo

--source include/have_binlog_format_row_or_statement.inc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally had

Suggested change
--source include/have_binlog_format_row_or_statement.inc
--source include/have_log_bin.inc

But (at least for me,) the Mixed format would rotate 0-1-2 and latter to master-bin.000002.
mariadb-binlog path/to/master-bin.000002 somehow even alerted

ERROR: Found out of order GTID. Got 0-1-1 after 0-1-3

@@ -3343,6 +3344,7 @@ static my_bool report_audit_findings(void *entry, void *report_ctx_arg)
}

/* Report any out of order GTIDs */
if (report_ctx->report_out_of_order)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of stashing late_gtids until the end, Binlog_gtid_state_validator::record could instead warn/error out-of-order GTIDs itself live.

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

Successfully merging this pull request may close these issues.

1 participant