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

logical: disallow replicating into a table with non-nullable column i… #130127

Closed
wants to merge 1 commit into from

Conversation

navsetlur
Copy link
Contributor

…n a column family

LDR streams currently process incoming data via a row processor. In order to apply column family updates from the rangefeed which don't contain a full row, columns not in the family will have to be nullable in order to write the entire row. This code verifies that when starting the LDR stream.

Release note: none
Informs: #129892

@navsetlur navsetlur requested a review from a team as a code owner September 5, 2024 03:20
@navsetlur navsetlur requested review from stevendanna and removed request for a team September 5, 2024 03:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@msbutler
Copy link
Collaborator

msbutler commented Sep 5, 2024

@navsetlur can i ask you to review the test flakes on this before i review?

@navsetlur navsetlur force-pushed the ldr_column_family branch 2 times, most recently from 8f7fead to 7034c43 Compare September 5, 2024 21:09
@@ -154,6 +157,10 @@ func createLogicalReplicationStreamPlanHook(
}
break
}
// The rowid column is always NOT NULL and is never explicitly set by the row processor, so ignore it
if requireNotNull && !col.Nullable && col.Name != "rowid" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the col.Name != "rowid" isn't the right check, as it assumes the pk is one column named "rowid". I think the right check is

requireNotNull && !col.Nullable && colNotInPrimaryKey(col.ID)

I just made up that last function, but you have the info required to create it. I would post in -eng about this, as I'm not totally confident this is the right check.

…n a column family

LDR streams currently process incoming data via a row processor. In order to apply
column family updates from the rangefeed which don't contain a full row, columns not
in the family will have to be nullable in order to write the entire row. This code
verifies that when starting the LDR stream.

Release note: none
Informs: cockroachdb#129892
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants