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

ParadoxV5 · 2024-10-29T21:38:37Z

The Jira issue number for this PR is: MDEV-34614
Follow-up of MDEV-27037
- Resolves MDEV-27037 mysqlbinlog emits a warning when reaching EOF before stop-condition #3400 (comment)

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 warning implementation is in the Gtid_event_filter family through a new public polymorphic (virtual&override) method verify_completed_state, which provides (sub)filter statuses to stderr without exposing internals such as Domain_gtid_event_filter#m_stop_filters.
While it’s uncommon to commit a new public API targeting a non-latest version, this method is hierarchical and is extensible to say --start-position and even --ignore-server-ids (MDEV-20119).

This choice is superior to the previous Binlog_gtid_state_validator design.
Note that in --gtid-strict-mode (errors) or -vvv mode (warnings) (only), Binlog_gtid_state_validator is already prompting that all (rather than any) --start-positions are unreached. This may rather be an erroneous side effect (not certain; I’m new 👶).

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

Ideally, the Separation of Concerns would have:

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

Performance: The task of validating --stop-position (and --start-position too, if those’re to be implemented) GTIDs is O(n).

Release Notes

What does the Release Notes say for MDEV-27037?… 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.

ParadoxV5 · 2024-10-29T21:44:58Z

PR # 60²

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_gtid.test

sql/rpl_gtid.cc

bnestere

Hi @ParadoxV5

Thanks for a well done PR! Please see my notes.

client/mysqlbinlog.cc

sql/rpl_gtid.cc

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_gtid.test

sql/rpl_gtid.cc

ParadoxV5 · 2024-10-31T18:09:21Z

sql/rpl_gtid.cc

@@ -1,5 +1,5 @@
 /* Copyright (c) 2013, Kristian Nielsen and MariaDB Services Ab.
-   Copyright (c) 2020, 2022, MariaDB Corporation.
+   Copyright (c) 2020, 2024, MariaDB Corporation.


Hello seniors, new member question: Who, when, and what conventions are on the copyright years?

By the way, perhaps devs should ditch them altogether: https://hynek.me/til/copyright-years

That's probably a question for @vuvova (or maybe @dbart ?)

And FWIW, speaking for myself, I've only actually set a year when creating a new file. I never felt it necessary to update one in an existing file.

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_datetime.test

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_position.test

sql/rpl_gtid.cc

bnestere

Thanks @ParadoxV5 ! See my latest notes (though I can't help but feel I lead you in a circle earlier, to arrive back where you had already started looking 😅)

sql/rpl_gtid.cc

client/mysqlbinlog.cc

sql/rpl_gtid.cc

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_gtid.test

ParadoxV5 · 2024-11-11T01:58:00Z

sql/rpl_gtid.cc

@@ -4067,3 +4119,17 @@ my_bool Intersecting_gtid_event_filter::has_finished()
  }
  return FALSE;
 }
+
+my_bool Intersecting_gtid_event_filter::verify_completed_state()


This isn’t currently called, but I implemented the override anyway for potential future-proofing.
(position_gtid_filter is never an Intersecting_gtid_event_filter, but gtid_event_filter may be.)

ParadoxV5 · 2024-11-11T02:04:34Z

sql/rpl_gtid.cc

+  my_hash_iterate(&m_filters_by_id_hash,
+                  verify_subfilter_completed_state, &is_any_incomplete);


Instead of iterating the hash ~~(and segfault myself from the hash’s untypedness)~~, I could’ve directly iterated the Domain_gtid_event_filter::m_stop_filters.
Iterating Id_delegating_gtid_event_filter::m_filters_by_id_hash

future-proofs for --start-position warnings – if we want to.

is possible in the superclass, so Server_gtid_event_filter (used by --ignore-server-ids) can inherit the method override.

Any preference?

Mmm, now that I’m thinking about a larger-scale refactoring, I should use Domain_gtid_event_filter::m_stop_filters instead to reign in this patch’s scope.

ParadoxV5 · 2024-11-11T02:10:39Z

sql/rpl_gtid.cc

+{
+  my_bool is_any_incomplete= FALSE;
+  Gtid_event_filter *subfilter;
+  for (size_t i= 0; i < m_filters.elements; i++)


This method was based on ::has_finished().
Besides formatting, a difference is that this uses size_t to match the type of m_filters.elements.
The former had a ulong; it’s a potential bug, but I’m yet to review which earliest (supported) version this fix should apply on.

Or can I just leave this to [MDEV-26896] Enable -Wconversion globally?

sql/rpl_gtid.cc

client/mysqlbinlog.cc

ParadoxV5

Force-push: update to 10.11.11 (#3600 (comment))

client/mysqlbinlog.cc

ParadoxV5 · 2025-02-15T05:35:40Z

sql/rpl_gtid.cc

+  for (size_t i= 0; i < m_filters.elements; ++i)
+  {
+    subfilter=
+      *reinterpret_cast<Gtid_event_filter **>(dynamic_array_ptr(&m_filters, i));


I usually use

Suggested change

*reinterpret_cast<Gtid_event_filter **>(dynamic_array_ptr(&m_filters, i));

*static_cast<Gtid_event_filter **>(dynamic_array_ptr(&m_filters, i));

, but dynamic_array_ptr() appearently returns uchar* rather than void*.

ParadoxV5 · 2025-02-15T05:37:55Z

client/mysqlbinlog.cc

+  if (position_gtid_filter)
+    position_gtid_filter->verify_final_state();


When implemented,

Suggested change

if (position_gtid_filter)

position_gtid_filter->verify_final_state();

if (gtid_event_filter)

gtid_event_filter->verify_final_state();

will additionally warn if we finished processing before

reaching all --start-position GTID(s), and

touching all --ignore-domain/server-ids.

ParadoxV5 · 2025-02-15T05:38:33Z

sql/rpl_gtid.cc

+{
+  bool is_not_final= m_has_stop && !m_has_passed;
+  if (is_not_final)
+    Binlog_gtid_state_validator::warn(stderr,


mysqlbinlog.cc's static warning() family is all duplicate code of the Binlog_gtid_state_validator::warn() family, except the former specifies stderr and has a fflush(result_file) step.

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” `--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. The `Gtid_event_filter` family provides the the warning mechanism polymorphically and through the new public method `verify_completed_state`. This design is hierarchically extensible (e.g., to `--ignore-server-ids`). This commit also includes minor touchups: * `rpl_gtid.cc`: adjust cases when a `Window_gtid_event_filter` has only one of `--start-` and `--stop-position` (without intensive refactors) * `rpl_gtid.cc`: function docs improvements * `rpl_gtid.h`: Remove unimplemented, red-herring function prototype `Window_gtid_event_filter::verify_gtid_is_expected`

ParadoxV5 requested review from bnestere and DaveGosselin-MariaDB October 29, 2024 21:38

ParadoxV5 commented Oct 29, 2024

View reviewed changes

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_gtid.test Outdated Show resolved Hide resolved

sql/rpl_gtid.cc Outdated Show resolved Hide resolved

bnestere requested changes Oct 31, 2024

View reviewed changes

ParadoxV5 force-pushed the bb-MDEV-34614 branch from 2bf416c to ffec23d Compare October 31, 2024 17:55

ParadoxV5 commented Oct 31, 2024

View reviewed changes

DaveGosselin-MariaDB requested changes Nov 1, 2024

View reviewed changes

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_datetime.test Outdated Show resolved Hide resolved

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_position.test Outdated Show resolved Hide resolved

sql/rpl_gtid.cc Outdated Show resolved Hide resolved

bnestere self-assigned this Nov 2, 2024

bnestere added the MariaDB Corporation label Nov 2, 2024

bnestere requested changes Nov 2, 2024

View reviewed changes

sql/rpl_gtid.cc Outdated Show resolved Hide resolved

client/mysqlbinlog.cc Outdated Show resolved Hide resolved

sql/rpl_gtid.cc Outdated Show resolved Hide resolved

mysql-test/suite/binlog/t/binlog_mysqlbinlog_warn_stop_gtid.test Outdated Show resolved Hide resolved

ParadoxV5 marked this pull request as draft November 5, 2024 20:18

ParadoxV5 force-pushed the bb-MDEV-34614 branch from ffec23d to 4007ff0 Compare November 11, 2024 01:10

ParadoxV5 marked this pull request as ready for review November 11, 2024 01:14

ParadoxV5 requested a review from bnestere November 11, 2024 01:48

ParadoxV5 commented Nov 11, 2024

View reviewed changes

ParadoxV5 commented Dec 5, 2024

View reviewed changes

client/mysqlbinlog.cc Show resolved Hide resolved

ParadoxV5 marked this pull request as draft December 5, 2024 21:49

ParadoxV5 force-pushed the bb-MDEV-34614 branch from 4007ff0 to 8de9d95 Compare February 15, 2025 05:31

ParadoxV5 commented Feb 15, 2025

View reviewed changes

ParadoxV5 force-pushed the bb-MDEV-34614 branch from 8de9d95 to 1fc37e5 Compare February 17, 2025 01:53

ParadoxV5 marked this pull request as ready for review February 17, 2025 01:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ParadoxV5 commented Oct 29, 2024 •

edited

Loading

ParadoxV5 commented Oct 29, 2024

bnestere left a comment

ParadoxV5 Oct 31, 2024

bnestere Nov 2, 2024

bnestere left a comment

ParadoxV5 Nov 11, 2024

ParadoxV5 Nov 11, 2024

ParadoxV5 Feb 19, 2025

ParadoxV5 Nov 11, 2024

ParadoxV5 Dec 1, 2024

ParadoxV5 left a comment •

edited

Loading

ParadoxV5 Feb 15, 2025

ParadoxV5 Feb 15, 2025

ParadoxV5 Feb 15, 2025

	/*
	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

		my_hash_iterate(&m_filters_by_id_hash,
		verify_subfilter_completed_state, &is_any_incomplete);

	reinterpret_cast<Gtid_event_filter *>(dynamic_array_ptr(&m_filters, i));
	static_cast<Gtid_event_filter *>(dynamic_array_ptr(&m_filters, i));

		if (position_gtid_filter)
		position_gtid_filter->verify_final_state();

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

Are you sure you want to change the base?

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

Conversation

ParadoxV5 commented Oct 29, 2024 • edited Loading

Description

What problem is the patch trying to solve?

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

Release Notes

How can this PR be tested?

PR quality check

ParadoxV5 commented Oct 29, 2024

bnestere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnestere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 commented Oct 29, 2024 •

edited

Loading

ParadoxV5 left a comment •

edited

Loading