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

upgrades: update system.web_sessions migration to handle orphaned rows #100017

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

andyyang890
Copy link
Collaborator

@andyyang890 andyyang890 commented Mar 30, 2023

This patch updates the system.web_sessions user ID migration to delete
orphaned rows (i.e. rows corresponding to users that have been dropped)
after backfilling user IDs. They need to be deleted since they block
the addition of the NOT NULL constraint on the column.

Part of #87079

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andyyang890 andyyang890 added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 30, 2023
@andyyang890 andyyang890 requested review from rafiss and a team March 30, 2023 05:36
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

just one comment, and everything else lgtm!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890)


pkg/upgrade/upgrades/web_sessions_table_user_id_migration.go line 48 at r1 (raw file):

}

const deleteRowsForDroppedUsersWebSessionsTableStmt = `

i'm more in favor of doing this after the backfill, and changing this query to

	DELETE FROM system.web_sessions
	WHERE user_id IS NULL

(it seems less expensive)

This patch updates the system.web_sessions user ID migration to delete
orphaned rows (i.e. rows corresponding to users that have been dropped)
after backfilling user IDs. They need to be deleted since they block
the addition of the NOT NULL constraint on the column.

Release note: None
@andyyang890 andyyang890 force-pushed the web_sessions_dropped branch from 39f0435 to 1229975 Compare March 30, 2023 20:44
@andyyang890 andyyang890 marked this pull request as ready for review March 30, 2023 20:49
@andyyang890 andyyang890 requested review from a team and rafiss March 30, 2023 20:49
Copy link
Collaborator Author

@andyyang890 andyyang890 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 @rafiss)


pkg/upgrade/upgrades/web_sessions_table_user_id_migration.go line 48 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i'm more in favor of doing this after the backfill, and changing this query to

	DELETE FROM system.web_sessions
	WHERE user_id IS NULL

(it seems less expensive)

Done.

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm!

const deleteRowsForDroppedUsersWebSessionsTableStmt = `
DELETE FROM system.web_sessions
WHERE user_id IS NULL
LIMIT 1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nice, i forgot that we could have a limit here.

@andyyang890
Copy link
Collaborator Author

TFTR!

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 31, 2023

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Mar 31, 2023

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 setting reviewers, but backport branch blathers/backport-release-23.1-100017 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/100244/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

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


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

@andyyang890 andyyang890 deleted the web_sessions_dropped branch March 31, 2023 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants