sql: add user_id column to system.web_sessions table #96896
Conversation
|
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. |
|
This change is |
andyyang890
force-pushed
the
migrate_web_sessions
branch
11 times, most recently
from
79b3032 to
49040c2
Compare
andyyang890
force-pushed
the
migrate_web_sessions
branch
2 times, most recently
from
db5a381 to
7a9962e
Compare
There was a problem hiding this comment.
nice, looks good overall! just had a few nits
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andyyang890)
pkg/cli/democluster/session_persistence.go line 211 at r1 (raw file):
} // TODO(yang): Update this to insert new user_id column.
can we do this right now?
pkg/server/authentication.go line 493 at r1 (raw file):
ctx context.Context, userName username.SQLUsername, ) (int64, []byte, error) { privilegesTableHasUserIDCol := s.sqlServer.execCfg.Settings.Version.IsActive(ctx,
should this be using WebSessionsTableHasUserIDColumn?
pkg/upgrade/upgrades/web_sessions_table_user_id_migration_test.go line 100 at r1 (raw file):
} txRunner.Exec(t, fmt.Sprintf("CREATE USER testuser%d", i)) txRunner.Exec(t, fmt.Sprintf(`
nit: can you add a comment saying that this INSERT is meant to mimic what the real auth code does?
andyyang890
force-pushed
the
migrate_web_sessions
branch
from
7a9962e to
72a4b69
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cli/democluster/session_persistence.go line 211 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
can we do this right now?
Done.
pkg/server/authentication.go line 493 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
should this be using
WebSessionsTableHasUserIDColumn?
Oops, yeah. Good catch!
pkg/upgrade/upgrades/web_sessions_table_user_id_migration_test.go line 100 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: can you add a comment saying that this INSERT is meant to mimic what the real auth code does?
Done.
andyyang890
force-pushed
the
migrate_web_sessions
branch
from
72a4b69 to
8ab47c5
Compare
This patch adds a new `user_id` column to the `system.web_sessions` table, which corresponds to the existing `username` column. Migrations are also added to alter and backfill the table in older clusters. Release note: None
andyyang890
force-pushed
the
migrate_web_sessions
branch
from
8ab47c5 to
e2af9d1
Compare
|
TFTR! bors r=rafiss |
|
seems like bors got stuck bors r=rafiss |
|
Build failed: |
|
bors r=rafiss |
|
Build succeeded: |
97226: Add Inject Retry Errors On Commit variable r=rafiss a=lyang24 fixes #86370 Release note (sql change): The session variable inject_retry_errors_on_commit_enabled has been added. When this is true, any COMMIT statement will return a transaction retry error if it is run inside of an explicit transaction. If the client retries the transaction using the special cockroach_restart SAVEPOINT, then after the 3rd error, the transaction will proceed as normal. Otherwise, the errors will continue until inject_retry_errors_on_commit_enabled is set to false. The purpose of this setting is to allow developers to test their transaction retry logic. 97490: cli: use placeholder in SQL query r=rafiss a=knz Amends #96896. Release note: None Epic: CRDB-19134 Co-authored-by: Eric.Yang <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
This patch adds a new
user_idcolumn to thesystem.web_sessionstable,which corresponds to the existing
usernamecolumn. Migrations arealso added to alter and backfill the table in older clusters.
Part of #87079
Release note: None