MDEV-33782 Implement WRITE_TO_BINLOG_ENABLE option for events #3174

andremralves · 2024-04-02T23:03:38Z

The Jira issue number for this PR is: MDEV-33782

Description

Implement a new option for events, WRITE_TO_BINLOG_ENABLE. If this option is set, events will be replicated with Event:status = ENABLE, otherwise, the event status will be DISABLED. Thanks @andrelkin for the idea.

Here is a table showing some examples:

Sql command	Event Status on Master	Event Status on Slave
create event .. ENABLE ..	Enabled	Disabled
create event .. DISABLE ..	Disabled	Disabled
create event .. ENABLE WRITE_TO_BINLOG_ENABLE ..	Enabled	Enabled
create event .. DISABLE WRITE_TO_BINLOG_ENABLE ..	Disabled	Enabled

Previously, events were replicated with the SLAVESIDE_DISABLED status. With this patch they will be set either to DISABLED or ENABLED --if WRITE_TO_BINLOG_ENABLE is defined.

Release Notes

Implement a new option for events, WRITE_TO_BINLOG_ENABLE. If this option is set, events will be replicated with Event:status = ENABLE, otherwise, the event status will be DISABLED. Replicated events will not be set to SLAVESIDE_DISABLED anymore.

How can this PR be tested?

./mtr rpl.rpl_create_drop_event

It can also be tested with manual tests setting up a replication.

Basing the PR against the correct MariaDB version

This is a new feature and the PR is based against the latest MariaDB development branch.
This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

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.

HugoWenTD

In my opinion, DISABLE ON SLAVE and ENABLE ON SLAVE seem to suggest that they are specific options for the replica (formerly known as the slave) only. However, the existing DISABLE ON SLAVE command actually disables the feature on both the source (formerly known as the master) and the replica. MySQL actually has a bug report for it. https://bugs.mysql.com/bug.php?id=31643

On the other hand, if based on current behavior, new ENABLE ON SLAVE seems to do the right thing as the opposite to DISABLE ON SLAVE.

By the way, better to add some descriptions in the commit message for example why the change is needed.

LinuxJedi

Nice work!

It appears this is failing a test when embedded server is built, funcs_1.is_columns_mysql_embedded, could you please fix this too?

sql/event_data_objects.cc

andremralves · 2024-04-11T18:19:49Z

It appears this is failing a test when embedded server is built, funcs_1.is_columns_mysql_embedded, could you please fix this too?

I'm working on it.

Edit: It's fixed now

LinuxJedi

Many thanks for fixing that. I'll try to get someone from the replication team to give it the next stage of review.

andrelkin · 2024-08-07T13:10:04Z

@andremralves salute!
I started reviewing the top-level of this work, to have noticed something interesting I believe.
Please read on that on Jira and preferably reply on there too (that seems easier to engage the feature reporter).

Thanks for this work for far!

Andrei

andrelkin · 2024-08-09T15:07:48Z

sql/sql_yacc.yy

            Lex->event_parse_data->status_changed= true;
            $$= 1;
          }
+        | ENABLE_SYM ON SLAVE


Please read a discussion on Jira about how to express in SQL way the desired effect.

This patch does address the FR, but it complicates earlier dubious design decision still. As the FR requires to achieve on slave Event::status as ENABLE a straightforward way to help it is to encode such intent as a new option which would be merely for logging on master and unlogging on slave (that is slave should be able to deduce which of the binary status the replicated Event should be created with). That is it need not to expand Event::status, rather contrary.

We could even leave the current name of SLAVESIDE_ENABLED but not regard it the way it's done in the current patch.
Also consider WRITE_TO_BINLOG_ENABLE (somewhat this naming would follow an existing NO_WRITE_TO_BINLOG) instead.

When it's specified ...

andrelkin · 2024-08-09T15:20:50Z

sql/sql_yacc.yy

          }
+        | ENABLE_SYM ON SLAVE
+          {
+            Lex->event_parse_data->status= Event_parse_data::SLAVESIDE_ENABLED;


... status would be set on slave straight to ENABLE unless the event is CREATE-ed with DISABLE status.
Logics inside Event_parse_data::check_originator_id() would need some adjustment, which is to disable the event on slave

status= Event_parse_data::DISABLED

without the new logging option.

The status of event on any server then would flip between just two ENABLE and DISABLE.
I think the slave should preserve the new option in its binlog when one is configured on the slave.

Hi @andrelkin, you also mentioned create event .. log_as [DISABLE|ENABLE] in Jira, which I believe have the same purpose of WRITE_TO_BINLOG_ENABLE, but the latter one is just a more polished idea, right?

So basically, if WRITE_TO_BINLOG_ENABLE is set we replicate as ENABLED otherwise DISABLED. I made a table to make it easier to visualize:

Options Master Status Slave Status

status: ENABLED Enabled Disabled

status: DISABLED Disabled Disabled

status: ENABLED + WRITE_TO_BINLOG_ENABLE Enabled Enabled

status: DISABLED + WRITE_TO_BINLOG_ENABLE Disabled Enabled

Now I see that Event:status would be reduced to only ENABLED/DISABLED, but what should we do to SLAVESIDE_DISABLED? Remove it? Change it's purpose?

the latter one is just a more polished idea, right?

Indeed, and more precisely the new option to the command's syntax does not force a new state.

SLAVESIDE_DISABLED? Remove it?

Correct.

Howdy @andremralves !

To make it clear your option's syntax - just a single word WRITE_TO_BINLOG_ENABLE looks fine to me.
The patch therefore only lacks SLAVESIDE_DISABLED removal.

andrelkin

Hello Andre!

Thanks for a good effort to satisfy to the feature request.
I am explaining why we could and should do it better though.

Specifically I think we should treat the new syntax as a sub-clause to control logging/unlogging.
Inevitably we would eradicate the old Event_parse_data::SLAVESIDE_DISABLED from Event::status.

I hope the measures will make sense for you as well.

Cheers,
Andrei

andremralves · 2024-08-09T17:00:19Z

Hi Andrei,

Thanks for your feedback.

I will read more carefully the discussion on jira and then respond here and there.

andrelkin · 2025-01-27T15:49:43Z

Salve @andremralves!

I wonder about your plan and timetable to complete this PR?
Could you please share with us if any help is needed for that.

Happy NY on few calendars :-)!

Andrei

andremralves · 2025-01-31T15:10:55Z

Salve @andrelkin !

I got caught up with other things and honestly forgot about this issue—sorry for the delay! I’ll try to finish it over the weekend, and if I can’t, I’ll update you all on how I plan to proceed.

Thanks for checking in, and happy NY on those calendars too!

Cheers,
André

CLAassistant · 2025-02-03T02:00:26Z

All committers have signed the CLA.

bnestere · 2025-04-04T17:58:26Z

@andremralves, hi! It looks like @andrelkin made one final suggestion (link) to your patch before it is ready to go. Are you still planning on working on this?

andremralves force-pushed the MDEV-33782 branch 3 times, most recently from 1b36c60 to 63c54e4 Compare April 3, 2024 03:11

HugoWenTD reviewed Apr 9, 2024

View reviewed changes

LinuxJedi suggested changes Apr 11, 2024

View reviewed changes

sql/event_data_objects.cc Outdated Show resolved Hide resolved

andremralves force-pushed the MDEV-33782 branch from 63c54e4 to a2b0553 Compare April 11, 2024 17:56

andremralves force-pushed the MDEV-33782 branch from a2b0553 to 3e65c97 Compare April 11, 2024 21:14

andremralves requested a review from LinuxJedi April 12, 2024 00:35

LinuxJedi approved these changes Apr 22, 2024

View reviewed changes

andrelkin self-requested a review August 6, 2024 18:19

andrelkin reviewed Aug 9, 2024

View reviewed changes

andrelkin requested changes Aug 9, 2024

View reviewed changes

cvicentiu added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Nov 26, 2024

andremralves force-pushed the MDEV-33782 branch from 3e65c97 to c74e22e Compare February 3, 2025 02:00

andremralves changed the base branch from 11.5 to main February 3, 2025 02:01

andremralves changed the title ~~MDEV-33782 Implement ENABLE ON SLAVE option for events~~ MDEV-33782 Implement WRITE_TO_BINLOG_ENABLE option for events Feb 3, 2025

andremralves force-pushed the MDEV-33782 branch 2 times, most recently from af5a058 to 072bb89 Compare February 3, 2025 04:44

MDEV-33782 Implement WRITE_TO_BINLOG_ENABLE option for events

b1a0100

andremralves force-pushed the MDEV-33782 branch from 072bb89 to b1a0100 Compare February 3, 2025 04:45

bnestere added the Replication Patches involved in replication label Mar 25, 2025

Options	Master Status	Slave Status
status: ENABLED	Enabled	Disabled
status: DISABLED	Disabled	Disabled
status: ENABLED + WRITE_TO_BINLOG_ENABLE	Enabled	Enabled
status: DISABLED + WRITE_TO_BINLOG_ENABLE	Disabled	Enabled

Uh oh!

MDEV-33782 Implement WRITE_TO_BINLOG_ENABLE option for events #3174

Are you sure you want to change the base?

MDEV-33782 Implement WRITE_TO_BINLOG_ENABLE option for events #3174

Uh oh!

Conversation

andremralves commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

Uh oh!

HugoWenTD left a comment

Choose a reason for hiding this comment

Uh oh!

LinuxJedi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andremralves commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinuxJedi left a comment

Choose a reason for hiding this comment

Uh oh!

andrelkin commented Aug 7, 2024

Uh oh!

andrelkin Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrelkin Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andremralves Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrelkin Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

andrelkin Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

andrelkin left a comment

Choose a reason for hiding this comment

Uh oh!

andremralves commented Aug 9, 2024

Uh oh!

andrelkin commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andremralves commented Jan 31, 2025

Uh oh!

CLAassistant commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnestere commented Apr 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

andremralves commented Apr 2, 2024 •

edited

Loading

andremralves commented Apr 11, 2024 •

edited

Loading

andrelkin Aug 9, 2024 •

edited

Loading

andrelkin Aug 9, 2024 •

edited

Loading

andremralves Feb 2, 2025 •

edited

Loading

andrelkin commented Jan 27, 2025 •

edited

Loading

CLAassistant commented Feb 3, 2025 •

edited

Loading