Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VReplication: ignore generated columns in workflows #8129

Merged
merged 9 commits into from
Jun 10, 2021

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented May 14, 2021

Description

Currently generated columns are not supported during a resharding workflow. MySQL does not allow values to be provided for generated columns either for insert or update. Today, generated columns are treated as regular columns and hence they attempt to populate such columns.

This PR identifies these columns by looking at the Extra column in information_schema.columns and drops them from insert/update statements on the target.

Note: We continue to send these columns as part of the vstreamer events so the downstream consumers will see the actual values. We need to do this since vstreams might be consumed by CDC pipelines which should be expected to recompute the generated column data.

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

@rohit-nayak-ps rohit-nayak-ps force-pushed the rn-vr-gen-columns branch 2 times, most recently from 342dc21 to f4b0dc2 Compare May 15, 2021 09:59
@rohit-nayak-ps rohit-nayak-ps added Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature) labels May 19, 2021
…oMap as a precursor to adding Extra info for detecting generated columns

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
…sts to test generated columns in source as well as target for reshard and materialize

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

initial comments, not thorough review.

go/vt/vttablet/tabletmanager/vreplication/vreplicator.go Outdated Show resolved Hide resolved
DataType string
ColumnType string
IsPK bool
IsGenerated bool
Copy link
Contributor

Choose a reason for hiding this comment

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

this becomes very parallel to

const (
UnknownColumnType ColumnType = iota
TimestampColumnType
DateTimeColumnType
EnumColumnType
MediumIntColumnType
JSONColumnType
FloatColumnType
BinaryColumnType
)
// Column represents a table column
type Column struct {
Name string
IsUnsigned bool
Charset string
Type ColumnType
EnumValues string
EnumToTextConversion bool
// add Octet length for binary type, fix bytes with suffix "00" get clipped in mysql binlog.
// https://github.com/github/gh-ost/issues/909
BinaryOctetLength uint64
}
. I see we are solving the same kind of problems in different places. I'll defer to yours anyway.

go/vt/vttablet/tabletmanager/vreplication/vreplicator.go Outdated Show resolved Hide resolved
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps marked this pull request as ready for review June 8, 2021 15:21
mysqld mysqlctl.MysqlDaemon
pkInfoMap map[string][]*PrimaryKeyInfo
mysqld mysqlctl.MysqlDaemon
colInfoMap map[string][]*ColumnInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

map[string][]*ColumnInfo confuses me. Is the key a table-name? Let's consider making a new type to simplify the syntax.

Copy link
Collaborator

@systay systay Jun 10, 2021

Choose a reason for hiding this comment

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

I sometimes use type aliases for this.

type tableNameStr = string
colInfoMap := map[tableNameStr][]*ColumnInfo{}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great pattern. There are quite a few places where such maps are used it this will increase code clarity. I will fix it up when I touch this module next since likely it will affect quite a few lines of code and also tests might need to be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants