cockroachdb / cockroachdb/cockroach
sql: DROP PROVISIONED ROLES and SHOW USERS/ROLES apply inconsistent filters, and DROP LIMIT lacks ORDER BY
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Describe the problem**
`DROP PROVISIONED ROLES` and its companion `SHOW USERS|ROLES ... LAST LOGIN BEFORE` have two related defects. The docs recommend previewing a `DROP` with `SHOW` using "the same clauses," but the two statements do not compute the same set, and `DROP` does not choose a deterministic subset under `LIMIT`.
**1. DROP and SHOW apply different filters**
*(a) NULL handling on `LAST LOGIN BEFORE` differs.* `DROP` includes never-recorded-login users (`estimated_last_login_time IS NULL`):
```go
"(u.estimated_last_login_time IS NULL OR u.estimated_last_login_time < $%d)"
```
`SHOW` excludes them (bare `<`; a code comment there notes NULLs are dropped):
```go
"u.estimated_last_login_time < (%s)::TIMESTAMPTZ"
```
Under SQL three-valued logic `NULL < ts` is `UNKNOWN`, so `SHOW` omits exactly the never-logged-in users that `DROP` deletes.
*(b) Provisioning-scope filter differs.* `DROP` always restricts to provisioned users via `EXISTS (... system.role_options ... option = 'PROVISIONSRC')`. `SHOW` only adds that restriction when a `SOURCE` clause is supplied; otherwise it lists all users.
Net: `SHOW ... LAST LOGIN BEFORE ` under-reports (hides NULL/never-logged-in) and, without `SOURCE`, over-reports (includes non-provisioned users). It is not a faithful preview of `DROP` in either direction.
**2. DROP's LIMIT has no ORDER BY (nondeterministic selection)**
`buildProvisionedRolesQuery` emits `SELECT u.username FROM system.users AS u WHERE ... LIMIT $N` with no `ORDER BY`. When more than `N` users match, which `N` get dropped is nondeterministic and may vary run to run. `SHOW` uses `ORDER BY 1`, so even a NULL-consistent `SHOW` could not tell an operator which `N` rows a `LIMIT`-bounded `DROP` will remove.
**Impact**
The documented "preview with `SHOW` before you `DROP`" workflow is unreliable: a destructive statement's official preview systematically omits part of what it deletes (never-logged-in provisioned users) and cannot indicate which rows `LIMIT` selects. Note `estimated_last_login_time` is nullable, not backfilled on upgrade, and populated best-effort/async, so NULL spans "never logged in" and "no recorded login yet" (upgraded clusters, PCR/standby, non-recording auth paths, the async suppression window). DROP-includes-NULL can therefore remove provisioned users that are active-but-unrecorded. Blast radius is bounded by PROVISIONSRC-only, dependency-skipping, `LIMIT`, and re-provisioning on next login.
**Repro sketch**
1. Provision N+1 users from one IDP source; have some never authenticate (so `estimated_last_login_time IS NULL`).
2. `SHOW USERS WITH LAST LOGIN BEFORE ` -- never-logged-in users don't appear; non-provisioned users do.
3. `DROP PROVISIONED ROLES WITH LAST LOGIN BEFORE LIMIT ` -- never-logged-in users are dropped; the specific N dropped is not determined by any ordering.
**Code references** (pinned to `8812064`)
- DROP predicate (includes NULL): [drop_provisioned_roles.go#L310](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/drop_provisioned_roles.go#L310)
- DROP query (no `ORDER BY`): [drop_provisioned_roles.go#L322](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/drop_provisioned_roles.go#L322) (`buildProvisionedRolesQuery`)
- SHOW predicate (excludes NULL): [show_roles.go#L70](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/delegate/show_roles.go#L70)
- SHOW query (`ORDER BY 1`): [show_roles.go#L96](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/delegate/show_roles.go#L96)
**Suggested resolution**
- Have both statements share one predicate builder so the filters cannot drift, and decide the NULL policy explicitly:
- *Preferred:* make `SHOW` include NULL to match `DROP` (preserves dormant-cleanup intent, makes the preview truthful); correct the docs; consider surfacing the "no recorded login" cohort distinctly.
- *Alternative:* make `DROP` exclude NULL to match `SHOW` if "never delete on unknown last-login state" is preferred, requiring `SOURCE` for the NULL cohort.
- Add a deterministic `ORDER BY` to the DROP selection query (and document it) so `LIMIT` is stable and previewable.
Jira issue: CRDB-66640
Epic CRDB-54682
Contributor guide
Assessment
This issue has not been assessed yet.