-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
98370: sql: add session variable to not hold up role membership changes r=ajwerner a=ajwerner Epic: CRDB-22371 Release note (sql change): A new session variable has been introduced which can be used to make the granting and revoking of role memberships faster at the cost of some isolation claims. When granting or revoking a role from another role, the system waits until all transactions which are consulting the current set of role memberships to complete. This means, by default, that by the time the transaction which performed the grant or revoke operation returns successfully, the user has a proof that no ongoing transaction is relying on the state that existed prior to the change. The downside of this waiting is that it means that grant and revoke will take longer than the longest currently executing transaction. In some cases, users do not care about whether concurrent transactions will immediately see the side-effects of the operation, but would instead prefer that the grant or revoke finish rapidly. In order to aid in those cases, a new session variable `allow_role_memberships_to_change_during_transaction` has been added. Now, the grant or revoke will only need to wait for the completion of statements in sessions which do not have that option set. One can set the option as enabled by default in all sessions in order to accelerate and grant and revoke role operations. Co-authored-by: ajwerner <[email protected]>
- Loading branch information
Showing
9 changed files
with
185 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
134 changes: 134 additions & 0 deletions
134
pkg/sql/tests/allow_role_memberships_to_change_during_transaction_test.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| // Copyright 2023 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package tests | ||
|
|
||
| import ( | ||
| "context" | ||
| gosql "database/sql" | ||
| "net/url" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/cockroachdb/cockroach/pkg/base" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" | ||
| "github.com/cockroachdb/cockroach/pkg/util/leaktest" | ||
| "github.com/cockroachdb/cockroach/pkg/util/log" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestAllowRoleMembershipsToChangeDuringTransaction ensures that when the | ||
| // corresponding session variable is set for all sessions using cached | ||
| // role memberships, performing new grants do not need to wait for open | ||
| // transactions to complete. | ||
| func TestAllowRoleMembershipsToChangeDuringTransaction(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
| defer log.Scope(t).Close(t) | ||
|
|
||
| ctx := context.Background() | ||
| s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) | ||
| defer s.Stopper().Stop(ctx) | ||
|
|
||
| openUser := func(username, dbName string) (_ *gosql.DB, cleanup func()) { | ||
| pgURL, testuserCleanupFunc := sqlutils.PGUrlWithOptionalClientCerts( | ||
| t, s.ServingSQLAddr(), username, | ||
| url.UserPassword(username, username), | ||
| false, /* withClientCerts */ | ||
| ) | ||
| pgURL.Path = dbName | ||
| db, err := gosql.Open("postgres", pgURL.String()) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| return db, func() { | ||
| require.NoError(t, db.Close()) | ||
| testuserCleanupFunc() | ||
| } | ||
| } | ||
|
|
||
| // Create three users: foo, bar, and baz. | ||
| // Use one of these users to hold open a transaction which uses a lease | ||
| // on the role_memberships table. Ensure that initially granting does | ||
| // wait on that transaction. Then set the session variable and ensure | ||
| // that it does not. | ||
| tdb := sqlutils.MakeSQLRunner(sqlDB) | ||
| tdb.Exec(t, "CREATE USER foo PASSWORD 'foo'") | ||
| tdb.Exec(t, "CREATE USER bar") | ||
| tdb.Exec(t, "CREATE USER baz") | ||
| tdb.Exec(t, "GRANT admin TO foo") | ||
| tdb.Exec(t, "CREATE DATABASE db2") | ||
| tdb.Exec(t, "CREATE TABLE db2.public.t (i int primary key)") | ||
|
|
||
| fooDB, cleanupFoo := openUser("foo", "db2") | ||
| defer cleanupFoo() | ||
|
|
||
| // In this first test, we want to make sure that the query cannot proceed | ||
| // until the outer transaction commits. We launch the grant while the | ||
| // transaction is open, wait a tad, make sure it hasn't made progress, | ||
| // then we commit the relevant transaction and ensure that it does finish. | ||
| t.Run("normal path waits", func(t *testing.T) { | ||
| fooTx, err := fooDB.BeginTx(ctx, nil) | ||
| require.NoError(t, err) | ||
| var count int | ||
| require.NoError(t, | ||
| fooTx.QueryRow("SELECT count(*) FROM t").Scan(&count)) | ||
| require.Equal(t, 0, count) | ||
| errCh := make(chan error, 1) | ||
| go func() { | ||
| _, err := sqlDB.Exec("GRANT bar TO baz;") | ||
| errCh <- err | ||
| }() | ||
| select { | ||
| case <-time.After(10 * time.Millisecond): | ||
| // Wait a tad and make sure nothing happens. This test will be | ||
| // flaky if we mess up the leasing. | ||
| case err := <-errCh: | ||
| t.Fatalf("expected transaction to block, got err: %v", err) | ||
| } | ||
| require.NoError(t, fooTx.Commit()) | ||
| require.NoError(t, <-errCh) | ||
| }) | ||
| // In this test we ensure that we can perform role grant and revoke | ||
| // operations while the transaction which uses the relevant roles | ||
| // remains open. We ensure that the transaction still succeeds and | ||
| // that the operations occur in a timely manner. | ||
| t.Run("session variable prevents waiting", func(t *testing.T) { | ||
| fooConn, err := fooDB.Conn(ctx) | ||
| require.NoError(t, err) | ||
| defer func() { _ = fooConn.Close() }() | ||
| _, err = fooConn.ExecContext(ctx, "SET allow_role_memberships_to_change_during_transaction = true;") | ||
| require.NoError(t, err) | ||
| fooTx, err := fooConn.BeginTx(ctx, nil) | ||
| require.NoError(t, err) | ||
| var count int | ||
| require.NoError(t, | ||
| fooTx.QueryRow("SELECT count(*) FROM t").Scan(&count)) | ||
| require.Equal(t, 0, count) | ||
| conn, err := sqlDB.Conn(ctx) | ||
| require.NoError(t, err) | ||
| defer func() { _ = conn.Close() }() | ||
| // Set a timeout on the SQL operations to ensure that they both | ||
| // happen in a timely manner. | ||
| grantRevokeTimeout, cancel := context.WithTimeout( | ||
| ctx, testutils.DefaultSucceedsSoonDuration, | ||
| ) | ||
| defer cancel() | ||
|
|
||
| _, err = conn.ExecContext(grantRevokeTimeout, "GRANT foo TO baz;") | ||
| require.NoError(t, err) | ||
| _, err = conn.ExecContext(grantRevokeTimeout, "REVOKE bar FROM baz;") | ||
| require.NoError(t, err) | ||
|
|
||
| // Ensure the transaction we held open commits without issue. | ||
| require.NoError(t, fooTx.Commit()) | ||
| }) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters