syntheticprivilegecache: scan all privileges at startup

[ajwerner]

syntheticprivilegecache: move caching logic out of sql

This is a pure refactor to move the logic for caching synthetic privileges
from the sql package. This will make it easier to add features later.

syntheticprivilegecache: scan all privileges at startup

Fixes #93182

This commit attempts to alleviate the pain caused by synthetic virtual table
privileges introduced in 22.2. We need to fetch privileges for virtual tables
to determine whether the user has access. This is done lazily. However, when a
user attempts to read a virtual table like pg_class or run SHOW TABLES it will
force the privileges to be determined for each virtual table (of which there
are 290 at the time of writing). This sequential process can be somewhat slow
in a single region cluster and will be very slow in an MR cluster.

This patch attempts to somewhat alleviate this pain by scanning the table
eagerly during server startup.

Release note (performance improvement): In 22.2 we introduced privileges on
virtual tables (system catalogs like pg_catalog, information_schema, and
crdb_internal). A problem with this new feature is that we now must fetch those
privileges into a cache before we can use those tables or determine their
visibility in other system catalogs. This process used to occur on-demand, when
the privilege was needed. Now we'll fetch these privileges eagerly during
startup to mitigate the latency when accessing pg_catalog right after the
server boots up.

[cockroach-teamcity]

This change is 

[rafiss]

feel free to merge if you do any of the nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/sql/syntheticprivilegecache/accumulator.go line 21 at r1 (raw file):

)

// Accumulator accumulates different rows of system.privileges

nit: should be lower case

---

pkg/sql/syntheticprivilegecache/cache.go line 130 at r1 (raw file):

		catconstants.SystemPrivilegeTableName,
	)
	// TODO(ajwerner): Use an internal executor bound to the transaction.

could we add a sqlutil.InternalExecutor as a parameter and pass initInternalExecutor(ctx, p) as the argument? i guess that's uglier?

---

pkg/sql/syntheticprivilegecache/cache.go line 98 at r2 (raw file):

	// Before we launch a goroutine to go fetch this descriptor, make sure that
	// the logic to fetch all descriptors at startup has completed.
	if err := c.waitForWarmed(ctx); err != nil {

should this wait be before we call getFromCache?

[rafiss]

is the performance impact bad enough that we need to backport?

[ajwerner]

is the performance impact bad enough that we need to backport?

Yes. Added the tag.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/syntheticprivilegecache/accumulator.go line 21 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: should be lower case

Done.

---

pkg/sql/syntheticprivilegecache/cache.go line 130 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could we add a sqlutil.InternalExecutor as a parameter and pass initInternalExecutor(ctx, p) as the argument? i guess that's uglier?

All of this is going away very soon with #93218 which is why I left it.

---

pkg/sql/syntheticprivilegecache/cache.go line 98 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should this wait be before we call getFromCache?

It could, but I don't think it matters. If we found something in the cache, then it was warmed. I'd rather not have to check the channel in the common case.

[ajwerner]

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[ajwerner]

bors r-

[craig]

Canceled.

[ajwerner]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

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 13c07ce to blathers/backport-release-22.2-93557: 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.2.x failed. See errors above.

---

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