MDEV-30456 ALTER TABLE algorithm clause not replicated #2629
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dlenski
suggested changes
May 10, 2023
There was a problem hiding this comment.
It appears that this bug and this PR apply specifically to the case of Galera replication, and not to other types of replication that MariaDB supports.
I suggest modifying the PR title and description to clarify this.
Perhaps something like the following?
MDEV-30456 Galera replication ignores ALTER_ALGORITHM session variable
| ALTER TABLE t1 ADD COLUMN f2 INTEGER, ALGORITHM=INSTANT; | ||
| show binlog events limit 1 offset 4; | ||
| Log_name Pos Event_type Server_id End_log_pos Info | ||
| binlog.000001 367 Query 1 489 use `test`; ALTER TABLE t1 ADD COLUMN f2 INTEGER, ALGORITHM=INSTANT |
There was a problem hiding this comment.
Many of the fields here (Pos, End_log_pos, and maybe Log_name and Server_id?) will vary from run to run of this test.
The only thing the test result actually cares about (I think) is the Info column, right?
Consider adding this before each invocation of show binlog events … in the .test file:
--replace_column 1 LOG_NAME 2 POS 4 SERVER_ID 5 END_LOG_POS
See mysql-test/main/alter_table.test, for example, where this is used in other pre-existing tests.
| [mysqld.2] | ||
| log_slave_updates=ON | ||
| log_bin=binlog | ||
|
|
There was a problem hiding this comment.
Nit: extra blank line at end of file.
| @@ -108,6 +108,12 @@ class Alter_info | |||
| // Number of partitions. | |||
| uint num_parts; | |||
| private: | |||
| #ifdef WITH_WSREP | |||
| /* wsrep patch needs to peak the algorithm clause used in ALTER statement | |||
There was a problem hiding this comment.
- Typo: I believe you meant the word *
pick, notpeak, here. - Why not simply make the
algorithmmemberpublic:unconditionally?
If user has specified the desired alter algorithm by using session variable alter_algorithm, and not specifying the algorithm in the ALTER SQL statement, then the ALTER will be processed in sending node according the algorithm chosen by the alter_algorithm variable. But in receiving nodes, appliers cannot see this session variable, and will use the default algorithm when executing the ALTER. The fix in this commit, will check if user has set alter_algorithm variable and has not specified the algorithm clause in the ALTER statement, and in such case will rewrite the original ALTER statement appended by the algorithm clause. This rewritten ALTER statement will be used in replication. The commit has also new mtr test: galera.galera_alter_table_algorithm which verifies that ALTER query rewrite happens when needed
Fixing the regression happening with galera.galera_toi_ddl_nonconflicting The problem, surfaced by the test, was due to multi-statement used in the test. The test issues two queries, as multi-statement: --send ALTER TABLE t1 ADD COLUMN f3 INTEGER; INSERT INTO t1 (f1, f2) VALUES (DEFAULT, 123); With this, the session's THD structure has in query_string member the full multi-statement, and this was used as base query in TOI replication rewriting. The fix is to pass actual length of the ALTER statement part for TOI replication.
sjaakola
force-pushed
the
10.4-MDEV-30456
branch
from
285ff5d
to
fe7b984
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
If user has specified the desired alter algorithm by using session variable alter_algorithm, and not specifying the algorithm in the ALTER SQL statement, then the ALTER will be processed in sending node according the algorithm chosen by the alter_algorithm variable. But in receiving nodes, appliers cannot see this session variable, and will use the default algorithm when executing the ALTER.
The fix in this commit, will check if user has set alter_algorithm variable and has not explicitly specified the algorithm clause in the ALTER statement, and in such case will rewrite the original ALTER statement appended by the algorithm clause. This rewritten ALTER statement will be used in replication.
How can this PR be tested?
The commit has also new mtr test: galera.galera_alter_table_algorithm which verifies that ALTER query rewrite happens when needed
Basing the PR against the correct MariaDB version
Backward compatibility
PR quality check