sql: fix data race in MemberOfWithAdminOption by andyyang890 · Pull Request #98617 · cockroachdb/cockroach

andyyang890 · 2023-03-14T19:42:39Z

This patch fixes a race condition in MemberOfWithAdminOption
by using a fresh transaction within the singleflight call.

Release note: None

blathers-crl · 2023-03-14T19:42:43Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

cockroach-teamcity · 2023-03-14T19:43:01Z

This change is

andyyang890 · 2023-03-15T05:22:36Z

This PR is an implementation of the idea discussed in #98469, but it seems to be hitting an issue where the system.role_members table appears to be empty. Is the modification time perhaps not set for the permanent upgrade that creates the initial row for (admin, root)?

rafiss · 2023-03-15T15:40:28Z

table appears to be empty. Is the modification time perhaps not set for the permanent upgrade that creates the initial row for (admin, root)?

that seems likely. if the modification time is not set, then we should be able to safely not change the txn timestamp -- since this means that the system.role_members table is in it's default state.

ajwerner · 2023-03-15T16:08:46Z

Modification time will always be set -- we assert it deeply. The fact that that migration fails to update it and set the version is sad. Maybe one thing we can do is detect version 1 and use a hard-coded set of data, or just keep the transaction timestamp?

andyyang890 · 2023-03-15T17:07:24Z

or just keep the transaction timestamp?

Would you mind elaborating a bit more about what the advantages of the original suggestion to set a fixed timestamp were? Are those benefits not strictly required for correctness?

rafiss · 2023-03-15T17:29:42Z

If the version is 1, that means there have been no modifications to the table. So using a historical timestamp or the current timestamp would both return the same results.

andyyang890 · 2023-03-15T17:33:46Z

If the version is 1, that means there have been no modifications to the table. So using a historical timestamp or the current timestamp would both return the same results.

I think the problem right now is that the permanent upgrade below inserts rows into the system.role_members table without bumping the table descriptor version. I'm guessing it would be unsafe to add code to the permanent upgrade that does the version bumping since it won't have been run on older clusters.

cockroach/pkg/upgrade/upgrades/permanent_upgrades.go

Lines 59 to 87 in 08f2dc4

    
           func addRootToAdminRole(ctx context.Context, txn isql.Txn) error { 
        
           	var upsertStmt string 
        
           	var upsertVals []interface{} 
        
           	{ 
        
           		// We query the pg_attribute to determine whether the role_id and member_id 
        
           		// columns are present because we can't rely on version gates here. 
        
           		const pgAttributeStmt = ` 
        
           			SELECT * FROM system.pg_catalog.pg_attribute 
        
           			         WHERE attrelid = 'system.public.role_members'::REGCLASS 
        
           			         AND attname IN ('role_id', 'member_id') 
        
           			         LIMIT 1 
        
           			         ` 
        
           		if row, err := txn.QueryRow(ctx, "roleMembersColumnsGet", txn.KV(), pgAttributeStmt); err != nil { 
        
           			return err 
        
           		} else if row == nil { 
        
           			upsertStmt = ` 
        
                     UPSERT INTO system.role_members ("role", "member", "isAdmin") VALUES ($1, $2, true) 
        
                     ` 
        
           			upsertVals = []interface{}{username.AdminRole, username.RootUser} 
        
           		} else { 
        
           			upsertStmt = ` 
        
                     UPSERT INTO system.role_members ("role", "member", "isAdmin", role_id, member_id) VALUES ($1, $2, true, $3, $4) 
        
           	        ` 
        
           			upsertVals = []interface{}{username.AdminRole, username.RootUser, username.AdminRoleID, username.RootUserID} 
        
           		} 
        
           	} 
        
           	_, err := txn.Exec(ctx, "addRootToAdminRole", txn.KV(), upsertStmt, upsertVals...) 
        
           	return err 
        
           }

rafiss · 2023-03-15T17:38:39Z

Right, I'm saying that if the version is 1 then we know:

the permanent upgrade has run (since the cluster isn't available until permanent upgrades are done)
no modifications have been made to the table since the permanent upgrade ran

So we can work around this issue of the upgrade not bumping the version by just using the current timestamp for the txn timestamp.

Or to resurrect this thread #98469 (comment)

You don't want ReadTimestamp() I don't think. I think you want to take that descriptor you read for the role_memberships table and take its ModificationTime().

Maybe this is a case where we can use the outer txn's ReadTimestamp?

andyyang890 · 2023-03-15T18:30:49Z

Ah I see, thanks for the clarification. I've updated the PR to do that!

rafiss

great! this lgtm, but just wanted to discuss my comment from one of the other PRs:

if possible, could we add a more targeted test that catches this bug? it feels kind of lucky to me that TestBackendDialTLS is the one that caught this. i could imagine that test being refactored in the future, and no longer catching a future regression on this.

We could add a test to pkg/sql/user_test.go that surfaces the bug? It's possible that TestGetUserTimeout already would have revealed the bug, but I see that test is skipped under race

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

andyyang890

Looks like TestGetUserTimeout does catch the bug so I've unskipped it under race! I also removed the backport-22.2.x tag since this issue only started happening after the internal executor refactor, which was merged in 23.1.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rafiss)

This patch fixes a race condition in MemberOfWithAdminOption by using a fresh transaction within the singleflight call. Release note: None

This test is being unskipped under race to catch any regressions for a data race in MemberOfWithAdminOption that was discovered and subsequently patched. Release note: None

andyyang890 · 2023-03-16T22:55:52Z

TFTR!

bors r=rafiss

craig · 2023-03-16T23:28:34Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-17T01:30:54Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-17T02:22:46Z

Build failed:

Bazel Essential CI (Cockroach)

andyyang890 · 2023-03-17T02:52:59Z

bors retry

craig · 2023-03-17T05:53:20Z

Build succeeded:

Bazel Essential CI (Cockroach)

andyyang890 requested review from a team, ajwerner and rafiss March 15, 2023 05:14

andyyang890 mentioned this pull request Mar 15, 2023

sql: fix data race in MemberOfWithAdminOption #98469

Closed

andyyang890 force-pushed the member_fixed_timestamp branch from 4ad8802 to abf67c3 Compare March 15, 2023 18:23

andyyang890 marked this pull request as ready for review March 15, 2023 18:36

rafiss reviewed Mar 15, 2023

View reviewed changes

rafiss added the backport-22.2.x label Mar 15, 2023

andyyang890 force-pushed the member_fixed_timestamp branch 2 times, most recently from 5ab7028 to f55d945 Compare March 16, 2023 18:59

andyyang890 removed the backport-22.2.x label Mar 16, 2023

andyyang890 requested a review from rafiss March 16, 2023 19:04

andyyang890 commented Mar 16, 2023

View reviewed changes

rafiss approved these changes Mar 16, 2023

View reviewed changes

andyyang890 added 2 commits March 16, 2023 17:26

sql: fix data race in MemberOfWithAdminOption

87d29c8

This patch fixes a race condition in MemberOfWithAdminOption by using a fresh transaction within the singleflight call. Release note: None

sql: unskip TestGetUserTimeout under race

37e1066

This test is being unskipped under race to catch any regressions for a data race in MemberOfWithAdminOption that was discovered and subsequently patched. Release note: None

andyyang890 force-pushed the member_fixed_timestamp branch from f55d945 to 37e1066 Compare March 16, 2023 21:27

craig bot merged commit 6014497 into cockroachdb:master Mar 17, 2023

andyyang890 deleted the member_fixed_timestamp branch March 17, 2023 05:55

andyyang890 mentioned this pull request Mar 17, 2023

sql: fix data race in MemberOfWithAdminOption #98376

Closed

tbg mentioned this pull request Apr 27, 2023

sql: internal query to read role memberships encounters GC threshold error [breaks user logins, SHOW GRANTS, and more] #102385

Closed

Conversation

andyyang890 commented Mar 14, 2023

Uh oh!

blathers-crl bot commented Mar 14, 2023

Uh oh!

cockroach-teamcity commented Mar 14, 2023

Uh oh!

andyyang890 commented Mar 15, 2023

Uh oh!

rafiss commented Mar 15, 2023

Uh oh!

ajwerner commented Mar 15, 2023

Uh oh!

andyyang890 commented Mar 15, 2023

Uh oh!

rafiss commented Mar 15, 2023

Uh oh!

andyyang890 commented Mar 15, 2023

Uh oh!

rafiss commented Mar 15, 2023

Uh oh!

andyyang890 commented Mar 15, 2023

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 commented Mar 16, 2023

Uh oh!

craig bot commented Mar 16, 2023

Uh oh!

craig bot commented Mar 17, 2023

Uh oh!

craig bot commented Mar 17, 2023

Uh oh!

andyyang890 commented Mar 17, 2023

Uh oh!

craig bot commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants