sql: settings from ALTER ROLE ... SET apply on session init

[rafiss]

Fixes #21151

The default settings are applied when creating a new pgwire session. The
information is cached alongside authentication info to reduce the
latency of establishing a connection and minimize the chance of a login
availability hit.

Release note (sql change): Default session variable settings configured
by ALTER ROLE ... SET are now supported. The following order of
precedence is used for variable settings:

1. settings specified in the connection URL as a query parameter.
2. per-role and per-database setting configured by ALTER ROLE.
3. per-role and all-database setting configured by ALTER ROLE.
4. all-role and per-database setting configured by ALTER ROLE.
5. all-role and all-database setting configured by ALTER ROLE.

RESET does not validate the setting name.
SET validates both the name and the proposed default value.

Note that the default settings for a role are not inherited if one role
is a member of another role that has default settings. Also, the
defaults only apply during session initialization. Using SET DATABASE to change databases does not apply default settings for that
database.

The public, admin, and root roles cannot have default session
variables configured. The root role also will never use the "all-role"
default settings. This is so that root has fewer dependencies during
session initialization and to make it less likely for root
authentication to become unavailable during the loss of a node.

Changing the default settings for a role requires the role running the
ALTER command to either be an ADMIN or to have the CREATEROLE role
option. Only ADMINs can edit the default settings for another admin.
Futhermore, changing the default settings for ALL roles is
only allowed for ADMINs. Roles without ADMIN or CREATEROLE cannot
change the default settings for themselves.

[cockroach-teamcity]

This change is 

[RichardJCai]

Reviewed 2 of 13 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/user.go, line 65 at r3 (raw file):

//   used without a KV lookup.
//
func GetUserAuthInfo(

I think this function is starting to become a bit overloaded, would it be easy to split out the settings resolution into a separate function without adding round trips?

---

pkg/sql/vars.go, line 1601 at r3 (raw file):

}

// CheckSessionVariableValueValid returns an error iff the value is not valid

nit: sorry for being super super pedantic but it isn't iff since theres another condition it returns an error (in the comment below) 😛

---

pkg/sql/authentication/cache.go, line 94 at r3 (raw file):

// SettingsCacheEntry for the provided username and databaseName. If the
// information is not in the cache, or if the underlying tables have changed
// since the cache was populated, then the readFromStore callback is used to

nit: Can we rename readFromStore to readFromSystemTables for clarity

Actually why are we passing readFromStore as a function to Get I think it would be more clear if we defined it as a separate helper.

---

pkg/sql/authentication/cache.go, line 282 at r3 (raw file):

// writeBackToCache tries to put the fetched data into the cache, and returns
// true if it succeeded.

Add comment that this also checks if the data is up to date.

[yuzefovich]

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

---

pkg/sql/alter_role.go, line 495 at r4 (raw file):

				newSettings,
			)
			if internalExecErr != nil {

nit: seems like this if could be pulled out outside of the outer if.

---

pkg/sql/drop_database.go, line 331 at r4 (raw file):

		return nil
	}
	rowsDeleted, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(

nit: this error is not checked right away now.

---

pkg/sql/pgwire/auth.go, line 141 at r4 (raw file):

	// Add all the defaults to this session's defaults. If there is an
	// error (e.g., a setting that no longer exists, or bad input,

nit: missing closing parenthesis.

[rafiss]

this is ready for another look

[RichardJCai]

Reviewed 4 of 13 files at r2, 1 of 1 files at r4, 2 of 8 files at r5, 16 of 16 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @yuzefovich)

---

pkg/sql/user.go, line 65 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think this function is starting to become a bit overloaded, would it be easy to split out the settings resolution into a separate function without adding round trips?

Add comment here that GetUserAuthInfo also checks for default session settings and the flow here.

---

pkg/sql/sessioninit/cache.go, line 101 at r10 (raw file):

	f *descs.CollectionFactory,
	username security.SQLUsername,
	readFromSystemTables func(

Is there a reason we have to pass readFromSystemTables as an argument instead of just calling retrieveAuthInfo?

---

pkg/sql/sessioninit/cache.go, line 361 at r10 (raw file):

			break
		}
		sEntries = append(sEntries, SettingsCacheEntry{k, s})

If multiple entries are found for a setting, are we appending them all here?
I'm thinking if its defined for default user and for the user, both entries would be added to this return right?

I think we should note that this is handled when applying the session settings or maybe dont add duplicates here.

---

pkg/sql/sessioninit/cache.go, line 416 at r10 (raw file):

}

func (a *Cache) checkStaleness(

nit: rename to something like clearCacheIfStale

[rafiss]

oops i forgot to publish my comments from the round of reviews last week

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @yuzefovich)

---

pkg/sql/alter_role.go, line 495 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: seems like this if could be pulled out outside of the outer if.

done

---

pkg/sql/user.go, line 65 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Add comment here that GetUserAuthInfo also checks for default session settings and the flow here.

done

---

pkg/sql/vars.go, line 1601 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: sorry for being super super pedantic but it isn't iff since theres another condition it returns an error (in the comment below) 😛

LOL good catch; i think this was actually a typo, i didn't mean to put iff :D

---

pkg/sql/sessioninit/cache.go, line 101 at r10 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Is there a reason we have to pass readFromSystemTables as an argument instead of just calling retrieveAuthInfo?

the reason it's passed as a parameter is because this cache lives in a different package (authentication), which cannot depend on the sql package.

---

pkg/sql/sessioninit/cache.go, line 361 at r10 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

If multiple entries are found for a setting, are we appending them all here?
I'm thinking if its defined for default user and for the user, both entries would be added to this return right?

I think we should note that this is handled when applying the session settings or maybe dont add duplicates here.

yeah, both of those would be added.

there's a comment about this in the retrieveDefaultSettings function of user.go that explains

	// Add an empty slice for all the keys so that something gets cached and
	// prevents a lookup for the same key from happening later.

i'll add a similar note here

---

pkg/sql/sessioninit/cache.go, line 416 at r10 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: rename to something like clearCacheIfStale

done

---

pkg/sql/authentication/cache.go, line 94 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: Can we rename readFromStore to readFromSystemTables for clarity

Actually why are we passing readFromStore as a function to Get I think it would be more clear if we defined it as a separate helper.

the reason it's passed as a parameter is because this cache lives in a different package (authentication), which cannot depend on the sql package.

---

pkg/sql/authentication/cache.go, line 282 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Add comment that this also checks if the data is up to date.

done

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @yuzefovich)

---

pkg/sql/authentication/cache.go, line 282 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

done

oh actually, it's done in the last commit of this PR already

[RichardJCai]

Looks almost good to me just some final questions / comments

Reviewed 7 of 17 files at r11, 7 of 19 files at r13.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @yuzefovich)

---

pkg/sql/user.go, line 146 at r13 (raw file):

			return retErr
		}
		// Avoid lookingup default settings for root and non-existent users.

nit: space

---

pkg/sql/sessioninit/cache.go, line 171 at r13 (raw file):

			}

			finishedLoop := a.writeAuthInfoBackToCache(

Should we be calling writeAuthInfoBackToCache if !CacheEnabled?

Or readAuthInfoFromCache?

---

pkg/sql/sessioninit/cache.go, line 290 at r13 (raw file):

		}

		if dbRoleSettingsTableDesc.IsUncommittedVersion() || !CacheEnabled.Get(&settings.SV) {

Does it make sense to check the !CacheEnabled case outside of this function and return readFromSystemTables outside this function?

---

pkg/sql/sessioninit/cache.go, line 298 at r13 (raw file):

				databaseID,
			)
			if err != nil || !CacheEnabled.Get(&settings.SV) {

nit: I'm confused

---

pkg/sql/authentication/cache.go, line 94 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the reason it's passed as a parameter is because this cache lives in a different package (authentication), which cannot depend on the sql package.

Gotcha

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @yuzefovich)

---

pkg/sql/sessioninit/cache.go, line 171 at r13 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Should we be calling writeAuthInfoBackToCache if !CacheEnabled?

Or readAuthInfoFromCache?

we would never reach this since the first line of GetAuthInfo() checks if the cache is enabled

---

pkg/sql/sessioninit/cache.go, line 290 at r13 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Does it make sense to check the !CacheEnabled case outside of this function and return readFromSystemTables outside this function?

i would have preferred to (that's how GetAuthInfo works). but unfortunately we do need to look up the DB descriptor ID so we have to start a descFactor.Txn to do that

---

pkg/sql/sessioninit/cache.go, line 298 at r13 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: I'm confused

this was a mistake. nice catch. we actually should just always end the Txn at this point (i also messed up GetAuthInfo)

[RichardJCai]

minor nit / question Do we test the caching functionality?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @yuzefovich)

---

pkg/sql/sessioninit/cache.go, line 298 at r13 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this was a mistake. nice catch. we actually should just always end the Txn at this point (i also messed up GetAuthInfo)

LOL I accidentally cut my comment short

[rafiss]

there's not a separate test for it -- the idea was that correctness would be checked by existing tests, and performance could be seen in the connection latency test. but as this review has caught there's definitely enough subtle logic in the cache that probably deserves it's own test

i will give some unit tests a shot. thanks for the nudge. i'll do it in a follow up PR so that Vy has a chance to do some acceptance testing on this before her PTO

bors r=RichardJCai

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @yuzefovich)

---

pkg/sql/sessioninit/cache.go, line 298 at r13 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

LOL I accidentally cut my comment short

:D

[craig]

Build succeeded:

• GitHub CI (Cockroach)