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

changefeedccl: protect system.role_members using protected timestamps #131027

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

asg0451
Copy link
Contributor

@asg0451 asg0451 commented Sep 19, 2024

Adds system.role_members to the list of system
tables that are protected by changefeeds.

Informs: #128806
See also: #130622

Release note (bug fix): Fix a bug which could
result in changefeeds using cdc queries failing
due to a system table being GC'd.

@asg0451 asg0451 requested a review from a team as a code owner September 19, 2024 15:24
@asg0451 asg0451 requested review from andyyang890 and rharding6373 and removed request for a team September 19, 2024 15:24
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@asg0451 asg0451 added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Sep 19, 2024
@asg0451 asg0451 requested review from stevendanna and removed request for andyyang890, rharding6373 and stevendanna September 19, 2024 15:27
@asg0451 asg0451 marked this pull request as draft September 19, 2024 15:30
@asg0451 asg0451 force-pushed the protect-role-members branch from 3ab267d to 7acc4d4 Compare September 19, 2024 15:46
@asg0451 asg0451 requested review from stevendanna and rharding6373 and removed request for stevendanna and rharding6373 September 19, 2024 15:46
Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @asg0451 and @stevendanna)


pkg/ccl/changefeedccl/protected_timestamps.go line 53 at r1 (raw file):

	keys.ZonesTableID,
	// Required for CDC Queries.
	// TODO: do we need any other tables, such as `keys.UsersTableID`?

Please add an issue number for this TODO so it doesn't get lost. I'd be in favor of keeping #128806 open to investigate other required tables.


pkg/ccl/changefeedccl/protected_timestamps_test.go line 547 at r1 (raw file):

Previously, stevendanna (Steven Danna) wrote…

I wonder if we could make this test less synthetic. Could we instead start a changefeed at a cursor with a cdc expression that should trigger the failure?

The purpose of this test is to validate the effectiveness of the PTS record, so I think using AOST to validate is ok here that it's not GC'd and therefore usable. It seems like there should be a way to turn it into a table-driven test with a case for each protected table, which will make this extendable if we need to protect more tables in the future.

@asg0451 asg0451 force-pushed the protect-role-members branch from 7acc4d4 to 438f1e1 Compare September 25, 2024 18:25
@asg0451 asg0451 marked this pull request as ready for review September 25, 2024 18:44
@asg0451 asg0451 force-pushed the protect-role-members branch 5 times, most recently from 4f123bb to 1ce2b84 Compare September 25, 2024 19:05
@asg0451 asg0451 force-pushed the protect-role-members branch from 1ce2b84 to 4e6e04b Compare September 25, 2024 19:56
Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

:lgtm: Thank you for adding this!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @asg0451 and @stevendanna)


-- commits line 7 at r2:
Change this to Informs so the issue doesn't close. Also update the PR's first comment in github.

Adds `system.role_members` to the list of system
tables that are protected by changefeeds.

Informs: cockroachdb#128806
See also: cockroachdb#130622

Release note (bug fix): Fix a bug which could
result in changefeeds using cdc queries failing
due to a system table being GC'd.
@asg0451 asg0451 force-pushed the protect-role-members branch from 4e6e04b to f6472f3 Compare September 27, 2024 14:51
@asg0451
Copy link
Contributor Author

asg0451 commented Sep 27, 2024

bors r=rharding6373

@craig craig bot merged commit 5b0371b into cockroachdb:master Sep 27, 2024
22 of 23 checks passed
Copy link

blathers-crl bot commented Sep 27, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from f6472f3 to blathers/backport-release-23.2-131027: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.


error creating merge commit from f6472f3 to blathers/backport-release-24.1-131027: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1.x failed. See errors above.


error creating merge commit from f6472f3 to blathers/backport-release-24.2-131027: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@stevendanna
Copy link
Collaborator

blathers backport 24.1 24.2 23.2

Copy link

blathers-crl bot commented Oct 1, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from f6472f3 to blathers/backport-release-23.2-131027: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants