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

sql: don't no-op SET SESSION AUTHORIZATION DEFAULT #86485

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Aug 19, 2022

Release note (bug fix): Previously, SET SESSION AUTHORIZATION DEFAULT
would have no effect. Now, it causes the current role to be reset to the
original user who logged into the session.

Release justification: low risk bug fix to existing functionality

Also includes:

  • sql: refactor DISCARD so it runs during execution
  • sql: consolidate logic for RESET ALL

@rafiss rafiss force-pushed the fix-discard-all-reset branch from e68d544 to 80158a5 Compare August 19, 2022 20:48
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Release note (bug fix): Previously, SET SESSION AUTHORIZATION DEFAULT
would have no effect. Now, it causes the current role to be reset to the
original user who logged into the session.

Release justification: low risk bug fix to existing functionality
@rafiss rafiss force-pushed the fix-discard-all-reset branch 2 times, most recently from f69cb98 to 60cee36 Compare August 20, 2022 21:41
rafiss added 2 commits August 21, 2022 01:48
Previously, it would run during planning, which is not correct, or at
least, makes it harder to reason about what's going on.

Release note: None

Release justification: low risk refactor
Release note: None

Release justification: low risk refactor
@rafiss rafiss force-pushed the fix-discard-all-reset branch from 60cee36 to 3887560 Compare August 21, 2022 05:48
@rafiss rafiss marked this pull request as ready for review August 22, 2022 14:46
@rafiss rafiss requested a review from a team August 22, 2022 14:46
@rafiss rafiss requested review from a team as code owners August 22, 2022 14:46
@rafiss rafiss requested a review from e-mbrown August 22, 2022 14:46
Copy link
Contributor

@e-mbrown e-mbrown left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, 4 of 4 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

@rafiss
Copy link
Collaborator Author

rafiss commented Aug 22, 2022

tftr!

bors r=e-mbrown

@craig
Copy link
Contributor

craig bot commented Aug 22, 2022

Build succeeded:

@craig craig bot merged commit 31bd044 into cockroachdb:master Aug 22, 2022
@blathers-crl
Copy link

blathers-crl bot commented Aug 22, 2022

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 creating merge commit from 42b67ff to blathers/backport-release-22.1-86485: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

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


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@rafiss
Copy link
Collaborator Author

rafiss commented Aug 22, 2022

I'm not going to backport this, since there are a lot of conflicts with special handling of intervalstyle_enabled and I feel like the risk isn't worth it.

@rafiss rafiss deleted the fix-discard-all-reset branch August 22, 2022 21:01
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Jan 25, 2023
This is an uncached round-trip. It makes `DISCARD` expensive.
In cockroachdb#86485 we implemented
`SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to
`DISCARD ALL` to check if the role we'd become exists. This is almost
certainly unintentional in the case where the role we'd become is the
current session role. I think this just happened because of code
consolidation.

We now no longer check if the current session role exists. This removes
the last round-trip from DISCARD ALL.

Release note (performance improvement): In 22.2, logic was added to make
`SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used
more general code for setting the role for a session which made sure that
the role exists. The check for whether a role exists is currently uncached.
We don't need to check if the role we already are exists. This improves the
performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION
DEFAULT`.
craig bot pushed a commit that referenced this pull request Jan 26, 2023
94153: sql: Remove type annotations from `column_default` in `information_schema.columns` r=ZhouXing19 a=e-mbrown

Informs: #87774

The type annotation on `column_default` caused confusion in postgres compatible apps. This change formats the `column_default` without the type annotation. It does not address the incorrect type annotation.

Release note (bug fix): The content of `column_default` in `information_schema.columns` no longer have type annotations.

95855: backupccl: run restore/tpce/32tb/aws/nodes=15/cpus=16 once a week r=benbardin a=msbutler

This patch adds the restore/tpce/32TB/aws/nodes=15/cpus=16 test to our weekly
test suite. The backup fixture was generated by initing a tpce cluster with 2
Million customers, and running this workload while 48 incremental backups were
taken at 15 minute increments. The roachtest restores from a system time
captured in the 24th backup.

The cluster used to restore the backup will run on aws with 15 nodes each with
16 vcpus.

To verify this test exists, run:
`roachtest list tag:weekly`

Release note: None

Epic: none

95861: kv: remove lastToReplica and lastFromReplica from raftMu r=nvanbenschoten a=nvanbenschoten

Extracted from #94165 without modification.

----

In 410ef29, we moved these fields from under the Replica.mu to under the Replica.raftMu. This was done to avoid lock contention.

In this commit, we move these fields under their own mutex so that they can be accessed without holding the raftMu. This allows us to send Raft messages from other goroutines.

The commit also switches from calling RawNode.ReportUnreachable directly in sendRaftMessage to using the more flexible unreachablesMu set, which defers the call to ReportUnreachable until the next Raft tick. As a result, the commit closes #84246.

Release note: None
Epic: None

95876: sql: remove round-trips from common DISCARD ALL r=ajwerner a=ajwerner

#### sql: avoid checking if a role exists when setting to the current role

This is an uncached round-trip. It makes `DISCARD` expensive.
In #86485 we implemented
`SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to
`DISCARD ALL` to check if the role we'd become exists. This is almost
certainly unintentional in the case where the role we'd become is the
current session role. I think this just happened because of code
consolidation.

We now no longer check if the current session role exists. This removes
the last round-trip from DISCARD ALL.

#### sql: use in-memory session data to decide what to do in DISCARD

In #86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive.

Fixes: #95864

Release note (performance improvement): In 22.2, logic was added to make
`SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used
more general code for setting the role for a session which made sure that
the role exists. The check for whether a role exists is currently uncached.
We don't need to check if the role we already are exists. This improves the
performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION
DEFAULT`.


Release note (performance improvement): In 22.2, we introduced support for `DISCARD TEMP` and made `DISCARD ALL` actually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result, `DISCARD ALL`, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved.

95997: ui: hide list of statement for non-admins r=maryliag a=maryliag

To get the list of fingerprints used by an index, we use the `system.statement_statistics` directly, but non-admins don't have access to system table.
Until #95756 or #95770 are done, we need to hide
this feature for non-admins.

Part Of #93087

Release note (ui change): Hide list of used fingerprint per index on Index Details page, for non-admin users.

95998: roachtest: update activerecord blocklist r=ZhouXing19 a=andyyang890

This patch removes a few tests from the blocklist that do not
exist in the test suite for rails 7.0.3, which were being run
prior to the fix for version pinning.

Informs #94211

Release note: None

96003: kv: add MVCC local timestamp details to ReadWithinUncertaintyIntervalError r=nvanbenschoten a=nvanbenschoten

This PR adds details about MVCC local timestamps to ReadWithinUncertaintyIntervalErrors. This provides more information about the cause of uncertainty errors.

The PR also includes a series of minor refactors to clean up this code.

Release note: None
Epic: None

Co-authored-by: e-mbrown <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Jan 27, 2023
This is an uncached round-trip. It makes `DISCARD` expensive.
In cockroachdb#86485 we implemented
`SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to
`DISCARD ALL` to check if the role we'd become exists. This is almost
certainly unintentional in the case where the role we'd become is the
current session role. I think this just happened because of code
consolidation.

We now no longer check if the current session role exists. This removes
the last round-trip from DISCARD ALL.

Release note (performance improvement): In 22.2, logic was added to make
`SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used
more general code for setting the role for a session which made sure that
the role exists. The check for whether a role exists is currently uncached.
We don't need to check if the role we already are exists. This improves the
performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION
DEFAULT`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants