Correct DDL generation for various CHANGE STREAM alterations#79
Correct DDL generation for various CHANGE STREAM alterations#79toga4 wants to merge 7 commits intodaichirata:masterfrom
Conversation
|
Thanks for the PR! It looks like the changes now conflict due to the merge of #78. Could you review the updates? |
- Use ALTER ... DROP FOR ALL instead of DROP + CREATE when changing from TABLES to NONE watch type - Use ALTER ... SET FOR instead of DROP + CREATE when moving watch target to another table - Set options to null instead of hardcoded defaults when removing CHANGE STREAM options - Properly handle table-bound CHANGE STREAM deletion when the table is deleted but the CHANGE STREAM still exists in target schema - Add comprehensive tests for all 7 CHANGE STREAM options
toga4
force-pushed
the
fix-changestream-drop
branch
from
885149a to
7dd548f
Compare
|
@daichirata Conflict now resolved! Could you please review? |
daichirata
left a comment
daichirata
left a comment
There was a problem hiding this comment.
@toga4 Thanks for resolving the conflict! Could you please check a few test cases?
CHANGE STREAM operations (ALTER/DROP) must be executed before dropping the tables they reference. This fixes issues where dropping a table before handling its associated CHANGE STREAM would cause errors.
toga4
force-pushed
the
fix-changestream-drop
branch
from
e34adf1 to
cfebec0
Compare
|
@daichirata Thanks for catching these cases! I've added both test cases and fixed the issues. Additionally, I've fixed an incorrect expected value in one test case. Please take a look when you have a chance! |
| // drop change streams | ||
| for _, fromChangeStream := range g.from.changeStreams { | ||
| if _, exists := g.findChangeStreamByName(g.to, identsToComparable(fromChangeStream.Name)); !exists { | ||
| ddl.AppendDDL(g.generateDDLForDropChangeStream(fromChangeStream)) |
There was a problem hiding this comment.
Since the original implementation is structured this way, this comment may be slightly out of scope for this PR, but for the sake of implementation consistency, would it make sense to also add it to g.dropedChangeStream here?
There was a problem hiding this comment.
You're right, thanks for catching this!
This loop only processes FOR ALL change streams, which are mutually exclusive from table-specific ones, so duplicate drops can't currently occur here. But I agree it's better to add both the g.dropedChangeStream append and the isDropedChangeStream() check for consistency with other loops.
internal/hammer/diff.go
Outdated
| continue | ||
| } | ||
| } | ||
| ddl.Append(&ast.DropChangeStream{Name: cs.Name}) |
There was a problem hiding this comment.
While looking into this, I also found a related bug at L534: it uses ddl.Append(&ast.DropChangeStream{...}) directly instead of g.generateDDLForDropChangeStream(), which causes an unnecessary REVOKE to be generated when dropping a table with an associated change stream that has a GRANT. I'll fix that too.
Add isDropedChangeStream check and dropedChangeStream append to the loop handling database-level change streams (FOR ALL) for consistency with table-level change stream handling.
Use generateDDLForDropChangeStream instead of directly appending DropChangeStream to ensure GRANT is recorded in droppedGrant. Previously, when dropping a table that has an associated change stream with a GRANT, an unnecessary REVOKE statement was generated because the GRANT was not tracked in droppedGrant.
| g.dropedChangeStream = append(g.dropedChangeStream, identsToComparable(cs.Name)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When adding a test like the following, ALTER CHANGE STREAM gets emitted multiple times.
{
from: `
CREATE TABLE t1 (
id INT64 NOT NULL,
) PRIMARY KEY(id);
CREATE TABLE t2 (
id INT64 NOT NULL,
) PRIMARY KEY(id);
CREATE CHANGE STREAM SomeStream FOR t1, t2;
`,
to: `
CREATE TABLE t2 (
id INT64 NOT NULL,
) PRIMARY KEY(id);
CREATE CHANGE STREAM SomeStream FOR t2;
`,
ignoreAlterDatabase: true,
expected: []string{
"ALTER CHANGE STREAM SomeStream SET FOR t2",
"DROP TABLE t1",
},
},=== Failed
=== FAIL: internal/hammer TestDiff/avoid_duplicate_alter_change_stream_set_for_on_drop_table (0.00s)
diff_test.go:2420: (-want, +got)
[]string{
+ "ALTER CHANGE STREAM SomeStream SET FOR t2",
"ALTER CHANGE STREAM SomeStream SET FOR t2",
"DROP TABLE t1",
}
There was a problem hiding this comment.
Fixed! c7a66e8
The duplicate occurred because generateDDLForDropConstraintIndexAndTable and the main change stream diff loop both emitted ALTER for the same change stream. Added a willCreateOrAlterChangeStreamIDs check to skip it when already scheduled.
Also fixed a related bug where unnecessary ALTER was emitted before DROP CHANGE STREAM (7d0c5aa), and added some edge case tests (f11fb05).
When a table is dropped and a change stream watching multiple tables needs to be altered, the ALTER CHANGE STREAM statement was being emitted twice: once in generateDDLForDropConstraintIndexAndTable and again in GenerateDDL's L313-323 loop. This fix checks willCreateOrAlterChangeStreamIDs before emitting the ALTER statement in generateDDLForDropConstraintIndexAndTable, skipping the output if the change stream is already scheduled for alteration.
2 participants
This PR fixes several issues in DDL generation for CHANGE STREAM alterations to generate more optimal and correct DDL statements.
Issues Fixed:
ALTER CHANGE STREAM ... DROP FOR ALLALTER CHANGE STREAM ... SET FORretention_period = '1d'), now correctly sets options to null to reset to server defaults