Skip to content

VReplication: _vt.vreplication source column VARBINARY->BLOB#6421

Merged
sougou merged 2 commits intovitessio:masterfrom
planetscale:tj-vrep-source-blob
Aug 27, 2020
Merged

VReplication: _vt.vreplication source column VARBINARY->BLOB#6421
sougou merged 2 commits intovitessio:masterfrom
planetscale:tj-vrep-source-blob

Conversation

@teejae
Copy link
Copy Markdown
Contributor

@teejae teejae commented Jul 8, 2020 •
edited
Loading

Signed-off-by: Toliver Jue toliver@planetscale.com

Change the source column to have unlimited size (4gb), to allow for larger vreplication source content, such as many tables.

  • Should cause no issues with current code.
  • Ideally has no performance impact for same-size source column data.

@teejae teejae requested a review from sougou as a code owner July 8, 2020 06:05
sougou
sougou previously requested changes Jul 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We now have a framework (and a policy) for upgrades. They should always be alter statements on top of the existing ones. The withddl package should be used to for managing the upgrades.

Having said that, there are more problems with this table than just this blob. I'd prefer that we design and work on a single consolidated fix to minimize the number of alters one has to apply for upgrades.

@teejae
Copy link
Copy Markdown
Contributor Author

teejae commented Jul 9, 2020

@sougou ok, do we have some consolidated idea of what a redesign of this table might look like?

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Jul 9, 2020

@sougou ok, do we have some consolidated idea of what a redesign of this table might look like?

@rohit-nayak-ps and I discussed some ideas, and some of the thoughts are in @shlomi-noach issue #6352. Essentially, we need to sit down and hash this out. But there have been other priorities.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Aug 2, 2020

This particular change itself is harmless because the change is backward compatible. But I'm worried about different versions of the table floating around, which can cause uncertainty during troubleshooting. Instead, we should just prioritize the refactoring of this table and fix all the other problems that exist with it.

So, I propose that we close this PR for now.

@teejae
Copy link
Copy Markdown
Contributor Author

teejae commented Aug 25, 2020 •
edited
Loading

@sougou
If we go with the ALTER statement strategy, how often would we check? we shouldn't be doing this on every write of the table right?

@shlomi-noach
Copy link
Copy Markdown
Contributor

shlomi-noach commented Aug 25, 2020

If we go with the ALTER statement strategy, how often would we check?

@teejae use WithDDL.Exec(), like so:

give or take

@teejae teejae force-pushed the tj-vrep-source-blob branch from 8acecd2 to 5515a02 Compare August 26, 2020 05:55
VReplication: _vt.vreplication source column VARBINARY->BLOB
0717e81 
Signed-off-by: Toliver Jue <toliver@planetscale.com>
@teejae teejae force-pushed the tj-vrep-source-blob branch from 5515a02 to 0717e81 Compare August 26, 2020 10:11
@teejae teejae requested a review from sougou August 26, 2020 10:38
@teejae
Copy link
Copy Markdown
Contributor Author

teejae commented Aug 26, 2020

@shlomi-noach thanks for the pointers. @sougou back to you.

shlomi-noach
shlomi-noach requested changes Aug 26, 2020
View reviewed changes
go/vt/binlog/binlogplayer/binlog_player.go

// AlterVReplicationTableSourceToBlob changes source column of vreplication table to BLOB
var AlterVReplicationTableSourceToBlob = []string{
"ALTER TABLE _vt.vreplication MODIFY source BLOB NOT NULL",
Copy link
Copy Markdown
Contributor

@shlomi-noach shlomi-noach Aug 26, 2020

Choose a reason for hiding this comment

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

I think this should just be part of AlterVReplicationTable. The comment // AlterVReplicationTable adds new columns to vreplication table is misleading: AlterVReplicationTable is about all transformation post-CREATE.

Copy link
Copy Markdown
Contributor Author

@teejae teejae Aug 26, 2020

Choose a reason for hiding this comment

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

yea, i debated whether it belongs there. i figured we wanted to keep order of all migration steps...just in case

Copy link
Copy Markdown
Contributor

@shlomi-noach shlomi-noach Aug 26, 2020

Choose a reason for hiding this comment

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

If anything, putting all statements into the same array is the best guarantee for order.

Copy link
Copy Markdown
Contributor Author

@teejae teejae Aug 26, 2020

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor Author

@teejae teejae Aug 26, 2020

Choose a reason for hiding this comment

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

i was wondering whether the Alter and Create should just be combined. i don't see any reason not to.

Copy link
Copy Markdown
Contributor

@shlomi-noach shlomi-noach Aug 26, 2020

Choose a reason for hiding this comment

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

Do you mean like the CREATE should be part of the array itself? I think that makes sense.

go/vt/vttablet/tabletmanager/vreplication/engine.go Outdated
allddls := append([]string{}, binlogplayer.CreateVReplicationTable()...)
allddls = append(allddls, binlogplayer.AlterVReplicationTable...)
allddls = append(allddls, createReshardingJournalTable, createCopyState)
allddls = append(allddls, binlogplayer.AlterVReplicationTableSourceToBlob...)
Copy link
Copy Markdown
Contributor

@shlomi-noach shlomi-noach Aug 26, 2020

Choose a reason for hiding this comment

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

This can be removed once query is moved into AlterVReplicationTable

Copy link
Copy Markdown
Contributor Author

@teejae teejae Aug 26, 2020

Choose a reason for hiding this comment

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

done

combine ALTER statements
92eace9 
Signed-off-by: Toliver Jue <toliver@planetscale.com>
@teejae teejae requested a review from shlomi-noach August 26, 2020 10:58
@teejae teejae force-pushed the tj-vrep-source-blob branch from ee108bf to 92eace9 Compare August 26, 2020 12:00
rohit-nayak-ps
rohit-nayak-ps approved these changes Aug 26, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

lgtm

@sougou sougou merged commit f388025 into vitessio:master Aug 27, 2020
@deepthi deepthi added this to the v8.0 milestone Sep 22, 2020
@setassociative setassociative mentioned this pull request Mar 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shlomi-noach shlomi-noach shlomi-noach approved these changes

@rohit-nayak-ps rohit-nayak-ps rohit-nayak-ps approved these changes

@morgo morgo Awaiting requested review from morgo

@sougou sougou Awaiting requested review from sougou

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v8.0

Development

Successfully merging this pull request may close these issues.

5 participants

@teejae @sougou @shlomi-noach @rohit-nayak-ps @deepthi