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

release-23.1: sql: avoid stampede on synthetic privilege cache in vtable population #109735

Merged
merged 2 commits into from Aug 31, 2023

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Aug 30, 2023

Backport 2/2 commits from #109517.

/cc @cockroachdb/release

Release justification: fix to a performance regression


Accessing the synthetic privilege cache requires fetching the descriptor
for system.system_privileges, even in the case of a cache hit. This is
ok for the basic case of fetching a single privilege descriptor for one
virtual table, which is done when executing something like SELECT * FROM a_virtual_table.

However, for some virtual tables, like pg_class, populating the table
itself requires fetching a privilege descriptor for each row in the result.
If a query does a lot of joins, that can mean hundreds of thousands of
calls to the synthetic privilege cache and the descriptor catalog. This
leads to a lot of thrashing in the descriptor catalog, since the catalog
is also being upserted into at the same time, since populating virtual
tables also requires fetching many descriptors.

This can be avoided by some small code changes that short-circuit and
avoid hitting the synthetic privilege cache entirely.

Before:

BenchmarkORMQueries/prisma_column_descriptions-10         	       1	6824922583 ns/op

After:

BenchmarkORMQueries/prisma_column_descriptions-10         	       1	2601088125 ns/op

fixes #108781

Release note (performance improvement): Queries that access the
pg_catalog and information_schema that perform introspection on other
tables in those schemas are now signicantly faster.

Release note (sql change): Introspection queries will now show the
internal node user as the owner of tables in pg_catalog and
information_schema. Previously the owner was shown as admin, but that
was inaccurate since users with the admin role could not modify these
tables in any way.

Accessing the synthetic privilege cache requires fetching the descriptor
for system.system_privileges, even in the case of a cache hit. This is
ok for the basic case of fetching a single privilege descriptor for one
virtual table, which is done when executing something like `SELECT *
FROM a_virtual_table`.

However, for some virtual tables, like pg_class, populating the table
itself requires fetching a privilege descriptor for each row in the result.
If a query does a lot of joins, that can mean hundreds of thousands of
calls to the synthetic privilege cache and the descriptor catalog. This
leads to a lot of thrashing in the descriptor catalog, since the catalog
is also being upserted into at the same time, since populating virtual
tables also requires fetching many descriptors.

This can be avoided by some small code changes that short-circuit and
avoid hitting the synthetic privilege cache entirely.

Before:
```
BenchmarkORMQueries/prisma_column_descriptions-10         	       1	6824922583 ns/op
```
After:
```
BenchmarkORMQueries/prisma_column_descriptions-10         	       1	2601088125 ns/op
```

Release note (performance improvement): Queries that access the
pg_catalog and information_schema that perform introspection on other
tables in those schemas are now signicantly faster.

Release note (sql change): Introspection queries will now show the
internal `node` user as the owner of tables in pg_catalog and
information_schema. Previously the owner was shown as `admin`, but that
was inaccurate since users with the `admin` role could not modify these
tables in any way.
@rafiss rafiss requested a review from ecwall August 30, 2023 17:19
@rafiss rafiss requested review from a team as code owners August 30, 2023 17:19
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss changed the title sql: avoid stampede on synthetic privilege cache in vtable population release-23.1: sql: avoid stampede on synthetic privilege cache in vtable population Aug 30, 2023
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.

None yet

3 participants