sql, *: allow table scoping under pg_temp_<session_id> physical schema

[arulajmani]

Previously, CRDB only supported the public physical schema. All table
entries in system.namespace assumed an implied public schema, so
name resolution for tables only required a databaseID and table name to
uniquely identify a table.

As highlighted in the temp tables RFC, temp tables will be scoped under
a session specific schema of the form pg_temp_<session_id>. This
motivated adding support for additional physical schemas.

This PR involves the following changes to system.namespace:

• A new system.namespace table is constructed for cluster versions

= 20.1, which has an additional primary key column publicSchemaID.

• The older system.namespace table is marked deprecated. All
  system.namespace read accesses default to the new system.namespace.
  If a match isn't found, the deprecated system.namespace is checked.

• All system.namespace write accesses for key deletions remove entries
  from both versions of the table. This ensures the fallback code doesn't
  read an old key that was deleted.

• All system.namespace write accesses that involve creating new entries
  are added to the system.namespace table according to the cluster version.

• Selecting from system.namespace in mixed version clusters actually
  selects from system.namespace_deprecated, ensuring that the change is
  invisible to users.

• The new system.namespace table is moved out of the SystemConfig
  range. This means system.namespace is no longer gossiped for cluster
  versions >= 20.1 .

• Every new database creation adds the public schema to
  system.namespace by default.

As a result of the above changes to system.namespace, there is a
new interface that all accesses should go through.

• Lookups
  Previously: Keys were constructed and directly used to perform KV
  lookups.
  Now: Use LookupObjectID, or another specialized lookup method
  provided. This ensures correct fallback semantics for mixed-version
  19.2/20.1 clusters.

• Removals
  Previously: Keys were constructed and directly used to perform KV
  deletes.
  Now: Use RemoveObjectNamespaceEntry or another specialized method
  provided. This ensures correct behavior for mixed-version 19.2/20.1
  clusters.

• Additions
  Previously: Keys were constructed and directly used to perform CPuts
  with the appropriate value.
  Now: Use MakeObjectNameKey or another specialized method provided to
  construct the key. This ensures that the key created is for the
  appropriate cluster version. These methods should only be used to
  create keys for adding entries -- removals/lookups should go through
  the appropriate interfaces.

The search_path is responsible for establishing the order in which
schemas are searched during name resolution. This PR involves the
following changes to the search_path.go:

• The search semantics are updated to match those described in the temp
  tables RFC.

• The search path is now aware of the session specific temporary schema,
  which can be used during name resolution.

• The distSQL api proto had to be updated to pass the temporary schema to
  other nodes in addition to the list of paths.

Benchmarks:

TPC-C with 3 nodes/16CPUs:

• max warehouses: 1565

Microbenchmarks for system.namespace access:

name	master time/op	new approach time/op	delta
NameResolution/Cockroach-8	163µs ± 0%	252µs ± 0%	~
NameResolution/MultinodeCockroach-8	419µs ± 0%	797µs ± 0%	~

name	master time/op	new approach time/op	delta
NameResolutionTempTablesExist/Cockroach-8	175µs ± 0%	337µs ± 0%	~
NameResolutionTempTablesExist/MultinodeCockroach-8	1.06ms ± 0%	1.07ms ± 0%	~

Follow-up work:

• The TableCollection cache needs to be updated to have knowledge about
  schemaIDs. Once this is done, there is a TODO in the code that allows the
  CachedPhysicalAccessor to work correctly.

• Migration for clusters upgrading to 20.1. The new system.namespace
  table needs to be populated from the deprecated table and a public
  schema needs to be added for every database during migration.

Release note (sql change): CREATE TABLE pg_temp.abc(a int) now creates
a temporary table. See temp tables RFC (guide-level explanation) for
more details about the search path semantics.

[cockroach-teamcity]

This change is 

[solongordon]

Looking good in general! I left some thoughts, mostly minor. I need to come back and review search_path.go and metadata.go in detail after I free up some brain space.

Reviewed 41 of 44 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @solongordon)

---

pkg/keys/constants.go, line 299 at r1 (raw file):

	// NOTE: IDs must be <= MaxSystemConfigDescID.
	SystemDatabaseID           = 1
	DeprecatedNamespaceTableID = 2

I know this is still a rough draft but make sure to add a nice comment here when you polish it up.

---

pkg/sql/create_table.go, line 66 at r1 (raw file):

func (n *createTableNode) startExec(params runParams) error {
	temporary := false

Nit: The ensuing code might read better if you say temporary = n.n.Temporary || n.n.Table.SchemaName == sessiondata.PgTempSchemaName

---

pkg/sql/create_table.go, line 71 at r1 (raw file):

	// pg_temp schema name is merely an alias for a temporary table. If the table
	// name is qualified under pg_temp, even without the TEMP/TEMPORARY keyword,
	// it implies a temporary table must be created.

Something about the "merely an alias" wording confuses me. I would recommend just saying this: "If a user specifies the pg_temp schema, even without the TEMPORARY keyword, a temporary table should be created."

---

pkg/sql/create_table.go, line 74 at r1 (raw file):

	if n.n.Temporary || n.n.Table.SchemaName == sessiondata.PgTempSchemaName {
		// An explicit schema can only be provided in the CREATE TEMP TABLE statement
		// iff it is pg_temp

Nit: end comments with a period.

---

pkg/sql/descriptor.go, line 85 at r1 (raw file):

	}

	// Every database must be initialized with the public schema

Nit: End comments with a period.

---

pkg/sql/opt_catalog.go, line 146 at r1 (raw file):

	//  need to be replaced then.
	isTempScoped := false
	if oc.tn.Schema() == sessiondata.PgTempSchemaName {

Hmm, to be honest I don't understand what is going on here. How would oc.tn.SchemaName have gotten set to the temp schema in the first place? It seems like oc.tn is basically just used in the context of this function. Isn't the schema name specified by the name argument here?

---

pkg/sql/physical_schema_accessors.go, line 100 at r1 (raw file):

) (TableNames, error) {
	ok, schemaID, err := a.IsValidSchema(ctx, txn, dbDesc.ID, scName)
	if !ok || err == nil {

Why err == nil?

---

pkg/sql/physical_schema_accessors.go, line 141 at r1 (raw file):

	}

	// Try to use the system name resolution bypass. Avoids a by explicitly

"Avoids a by"?

---

pkg/sql/physical_schema_accessors.go, line 259 at r1 (raw file):

		if obj == nil {
			return nil, err
		}

For readability I would move the below code changes into this if block too.

---

pkg/sql/select_name_resolution.go, line 12 at r1 (raw file):

//
// This file implements the select code that deals with column references
// and resolving column nmes in expressions.

bye bye a

---

pkg/sql/temporary_schema.go, line 46 at r1 (raw file):

	b.CPut(schemaNameKey, schemaID, nil)

	if err := p.txn.Run(ctx, b); err != nil {

Nit: You can just return p.txn.Run(ctx, b)

---

pkg/sql/sessiondata/search_path.go, line 32 at r1 (raw file):

const PgCatalogName = "pg_catalog"

const PgTempSchemaName = "pg_temp"

Could use a comment.

---

pkg/sql/sessiondata/search_path.go, line 40 at r1 (raw file):

	containsPgCatalog    bool
	containsPgTempSchema bool
	TempScName           string

Nit: I would just spell out "schema" here and elsewhere in this file for readability. Not worth saving the four characters here.

---

pkg/sql/sqlbase/structured.go, line 3805 at r1 (raw file):

}

// NewPublicTableKey returns a new TableKey.

Needs an update.

[arulajmani]

Thanks for the initial look over.

Seems like a bunch of small nits you had were fixed in my 2nd and 3rd commit. Should've squashed them before putting this PR out, but I wasn't sure if we need to break this up into 2 PRs or not, so I kept it this way for the initial review.

I'll add some tests for the changes I've added, write a proper commit message, and squash the commits down to one. I'll ping you when this is ready for a full review, but if you have comments about search_path.go or metadata.go, let me know and I'll incorporate them into my ready for review PR.

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

---

pkg/keys/constants.go, line 299 at r1 (raw file):

Previously, solongordon (Solon) wrote…

I know this is still a rough draft but make sure to add a nice comment here when you polish it up.

Done.

---

pkg/sql/create_table.go, line 71 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Something about the "merely an alias" wording confuses me. I would recommend just saying this: "If a user specifies the pg_temp schema, even without the TEMPORARY keyword, a temporary table should be created."

Done.

---

pkg/sql/opt_catalog.go, line 146 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Hmm, to be honest I don't understand what is going on here. How would oc.tn.SchemaName have gotten set to the temp schema in the first place? It seems like oc.tn is basically just used in the context of this function. Isn't the schema name specified by the name argument here?

This is for queries like CREATE [TEMP] TABLE pg_temp.abc(a int);

The reason for this special case if block is:

• If the first temporary table created is qualified with pg_temp, the optimizer will error out here as the temporary schema does not exist. This should not happen though, so by checking public exists, we make sure the database exists and then CREATE TABLE can handle the temporary schema creation.

---

pkg/sql/physical_schema_accessors.go, line 100 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Why err == nil?

This was wrong, I corrected it in the second commit. Will squash all 3 commits and write an actual commit message :)

---

pkg/sql/physical_schema_accessors.go, line 141 at r1 (raw file):

Previously, solongordon (Solon) wrote…

"Avoids a by"?

Hotspot! I fixed this in the second commit :(

---

pkg/sql/physical_schema_accessors.go, line 259 at r1 (raw file):

Previously, solongordon (Solon) wrote…

For readability I would move the below code changes into this if block too.

Done.

---

pkg/sql/select_name_resolution.go, line 12 at r1 (raw file):

Previously, solongordon (Solon) wrote…

bye bye a

ooops

---

pkg/sql/sessiondata/search_path.go, line 32 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Could use a comment.

3rd commit :(

---

pkg/sql/sessiondata/search_path.go, line 40 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Nit: I would just spell out "schema" here and elsewhere in this file for readability. Not worth saving the four characters here.

I think I got into writing ScName because name_resolution.go used the shorthand and I updated this file after. Will change to schema.

---

pkg/sql/sqlbase/structured.go, line 3805 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Needs an update.

The comment?

[solongordon]

Added some thoughts on search_path.go.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @solongordon)

---

pkg/sql/sessiondata/search_path.go, line 41 at r2 (raw file):

	containsPgCatalog    bool
	containsPgTempSchema bool
	TempSchemaName       string

Is there a reason this needs to be exported when the other fields are not?

---

pkg/sql/sessiondata/search_path.go, line 67 at r2 (raw file):

// string. This should be called every time a session creates a temporary schema
// for the first time.
func (s *SearchPath) UpdateTemporarySchemaName(tempSchemaName string) {

It makes me a little nervous that SearchPath used to be immutable and now we have these update methods. Maybe safer to call this WithTempSchemaName and have it return a new SearchPath instance.

---

pkg/sql/sessiondata/search_path.go, line 73 at r2 (raw file):

// UpdatePaths sets the updates the paths of the SearchPath struct whithout
// changing the session specific temporary schema.
func (s *SearchPath) UpdatePaths(paths []string) {

Same thought here about immutability.

---

pkg/sql/sessiondata/search_path.go, line 99 at r2 (raw file):

// searched in the specified order. If pg_catalog is not in the path then it
// will be searched before searching any of the path items."
// - https://www.postgresql.org/docs/9.1/static/runtime-config-client.html

Looks like this comment could use an update.

---

pkg/sql/sessiondata/search_path.go, line 109 at r2 (raw file):

	}

	return SearchPathIter{paths: append(paths, s.paths...), tempSchemaName: s.TempSchemaName, i: 0}

Super nit: Don't need to specify i: 0.

---

pkg/sql/sessiondata/search_path.go, line 132 at r2 (raw file):

// Equals returns true if two SearchPaths are the same.
func (s SearchPath) Equals(other *SearchPath) bool {
	if s.containsPgCatalog != other.containsPgCatalog || s.containsPgTempSchema != other.containsPgTempSchema {

Nit: Clearer to put the new check in a separate if statement.

---

pkg/sql/sessiondata/search_path.go, line 171 at r2 (raw file):

		schemaName := iter.paths[iter.i]
		iter.i++
		if schemaName == PgTempSchemaName && iter.tempSchemaName != DefaultTemporarySchema {

Can you do this logic when you construct SearchPathIter? Like add tempSchemaName to the paths if containsPgTempSchema is true and iter.tempSchemaName != DefaultTemporarySchema? I think that would be clearer.

[arulajmani]

TFTR. Addressed comments about search_path.go and added some tests for name resolution.

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

---

pkg/sql/sessiondata/search_path.go, line 41 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Is there a reason this needs to be exported when the other fields are not?

Remnant of an earlier implementation attempt -- changed!

---

pkg/sql/sessiondata/search_path.go, line 67 at r2 (raw file):

Previously, solongordon (Solon) wrote…

It makes me a little nervous that SearchPath used to be immutable and now we have these update methods. Maybe safer to call this WithTempSchemaName and have it return a new SearchPath instance.

Done.

---

pkg/sql/sessiondata/search_path.go, line 73 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Same thought here about immutability.

Done.

---

pkg/sql/sessiondata/search_path.go, line 99 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Looks like this comment could use an update.

Done.

---

pkg/sql/sessiondata/search_path.go, line 109 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Super nit: Don't need to specify i: 0.

Done.

---

pkg/sql/sessiondata/search_path.go, line 171 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Can you do this logic when you construct SearchPathIter? Like add tempSchemaName to the paths if containsPgTempSchema is true and iter.tempSchemaName != DefaultTemporarySchema? I think that would be clearer.

As discussed offline, I've changed this implementation to not do any extra allocations (and preserve iterator invariant)

[knz]

I did a casual review on this, here are a few findings:

1. search_path logic + name resolution general algorithm LGTM - nice work and nice tests 💯

2. statement traces reveal 2 mandatory KV lookups on every query. This needs more explaining in the commit message, and possibly follow-up work.

3. mostly because of point 2, but I'd have asked anyways, I would recommend you run a before/after TPC-C and KV benchmarks and paste the results in the commit message. I'm not expecting significant change but you do need the evidence.

4. can you explain in the commit message (and preferably also in the RFC) what happens when the cluster setting is changed while there are some SQL clients connected. What happens with clients that were running queries before the flip over to the new naming scheme?

5. you're passing a *cluster.Settings parameter in several places (I noticed first in sqlbase but that's also specifically why there's movement in sql) to mean "passing this argument marks the difference between pre- and post-20.1. I don't think this is a good idea: it will break down violently if a later PR in the release cycle makes the function need *cluster.Settings for an unrelated reason.

   Instead, I recommend you create a 2-valued enum singleSchemaNamespace/multiSchemaNamespace and pass that as argument alongside the *cluster.Settings to decide what to do. (And then pass the *cluster.Settings always when possible, regardless of the cluster version)

6. for each part of the code that needs this new enum value, and/or *cluster.Settings, please try harder to see if you can pre-compute it and store it "closer" to where it's needed, so as to avoid passing it along on every call. For example, the TableCollection can get it this way, you don't need to pass it along via the SQL executor. Also you definitely do not need to pass it as argument through the schema resolver interfaces, the resolver object should contain all the data you need already.

   In fact, if you think about points 4 and 5 together, you'll notice this will be the area of the design where you need a narrative (in the RFC and this commit message) about a) which bit of information decides whether to use system.namespace and system.deprecated_namespace b) where is this bit stored c) until when does this bit remain valid d) what do you do about overlapping requests from clients from the two worlds.

7. if you think twice about it, the cluster version is really known during bootstrapping, see comments below

8. nit: in the commit message write "search_path" not "searchPath"

Reviewed 6 of 44 files at r1, 58 of 78 files at r2, 25 of 25 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @solongordon)

---

pkg/server/admin_test.go, line 448 at r3 (raw file):

	// TODO(celia): We're missing 1 range -- where is it?
	// TODO(arul): We're missing 2 ranges after moving system.namespace out from
	//  the gossip range -- where are they?

Yeah indeed where are they? 😅

---

pkg/sql/conn_executor.go, line 2044 at r3 (raw file):

		// Wait for the cache to reflect the dropped databases if any.
		ex.extraTxnState.tables.waitForCacheToDropDatabases(ex.Ctx(), ex.server.cfg.Settings)

If you need the settings, store the reference to the settings in the tablecollection. This way you don't need to pass them as argument every time.

---

pkg/sql/create_table.go, line 66 at r3 (raw file):

func (n *createTableNode) startExec(params runParams) error {
	temporary := n.n.Temporary || n.n.Table.SchemaName == sessiondata.PgTempSchemaName

Make a method IsTemporary on TableName that takes a sessiondata argument, and checks the schema name. then call it here. It will make the code more readable.
(The method will also need to fail if the schema name has not been computed/resolved yet. You want that defensive programming to prevent control from arriving here with non-resolved names.)

also the variable should be called isTemporary.

---

pkg/sql/create_table.go, line 71 at r3 (raw file):

	// If a user specifies the pg_temp schema, even without the TEMPORARY keyword,
	// a temporary table should be created.
	if n.n.Temporary || n.n.Table.SchemaName == sessiondata.PgTempSchemaName {

if isTemporary { ...

---

pkg/sql/create_test.go, line 104 at r3 (raw file):

		// a migration associated with it yet. There are 3 databases: defaultdb,
		// system, and postgres.
		e := len(descriptorIDs) + 3

I think you have this now?

Also this test is only run with the new cluster version, so this should be fine.

---

pkg/sql/database.go, line 79 at r3 (raw file):

// the zone, name and descriptor of a database.
func getKeysForDatabaseDescriptor(
	dbDesc *sqlbase.DatabaseDescriptor, settings *cluster.Settings,

see my comments above

---

pkg/sql/database.go, line 137 at r3 (raw file):

// cache.
func (dc *databaseCache) getCachedDatabaseDesc(
	name string, settings *cluster.Settings,

should be stored in cache, no argument needed

---

pkg/sql/database.go, line 182 at r3 (raw file):

func (dc *databaseCache) getDatabaseDesc(
	ctx context.Context,
	settings *cluster.Settings,

ditto

---

pkg/sql/database.go, line 231 at r3 (raw file):

func (dc *databaseCache) getDatabaseID(
	ctx context.Context,
	settings *cluster.Settings,

ditto

---

pkg/sql/database.go, line 280 at r3 (raw file):

// renameDatabase implements the DatabaseDescEditor interface.
func (p *planner) renameDatabase(
	ctx context.Context,

ditto

---

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

	}

	tbNames, err := GetObjectNames(ctx, p.txn, p.ExecCfg().Settings, p, dbDesc, tree.PublicSchema, true /*explicitPrefix*/)

No need for the extra argument. The resolver (p) contains all the state needed already.

---

pkg/sql/logical_schema_accessors.go, line 52 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

see comments elsewhere

---

pkg/sql/logical_schema_accessors.go, line 77 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/opt_catalog.go, line 146 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This is for queries like CREATE [TEMP] TABLE pg_temp.abc(a int);

The reason for this special case if block is:

• If the first temporary table created is qualified with pg_temp, the optimizer will error out here as the temporary schema does not exist. This should not happen though, so by checking public exists, we make sure the database exists and then CREATE TABLE can handle the temporary schema creation.

It sounds to me that your approach here is perverting the very definition / meaning of this method's name, ResolveSchema.

If the temp schema is specified, but does not exist yet, this method should fail!
Since it's on the read-only data path, a failure here is a true failure, meaning the temp schema name was mistakenly provided (or the requested table does not exist in the temp namespace).

---

pkg/sql/opt_catalog.go, line 109 at r3 (raw file):

	return GetObjectNames(
		ctx, os.planner.Txn(),
		os.planner.ExecCfg().Settings,

see comments elsewhere

---

pkg/sql/opt_catalog.go, line 141 at r3 (raw file):

	// Instead of searching if the temporary schema exists, we search for the
	// public schema, which is guaranteed to exist in every database.

Is it guaranteed to exist, really? What if the user mistakenly removes the namespace entry?

---

pkg/sql/physical_schema_accessors.go, line 52 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

see my comments elsewhere

---

pkg/sql/physical_schema_accessors.go, line 105 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/physical_schema_accessors.go, line 145 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/physical_schema_accessors.go, line 237 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/physical_schema_accessors.go, line 272 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/resolver.go, line 58 at r3 (raw file):

) (res *UncachedDatabaseDescriptor, err error) {
	p.runWithOptions(resolveFlags{skipCache: true}, func() {
		res, err = p.LogicalSchemaAccessor().GetDatabaseDesc(ctx, p.txn, p.ExecCfg().Settings, dbName,

No need for the extra argument. The schema accessors should contain all the state needed already.

---

pkg/sql/resolver.go, line 68 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

No need for the extra argument. The resolver (p) contains all the state needed already.

---

pkg/sql/resolver.go, line 382 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/resolver.go, line 453 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/schema_accessors.go, line 78 at r3 (raw file):

	// descriptor. If the database is not found and required is true,
	// an error is returned; otherwise a nil reference is returned.
	GetDatabaseDesc(ctx context.Context, txn *client.Txn, settings *cluster.Settings, dbName string, flags tree.DatabaseLookupFlags) (*DatabaseDescriptor, error)

No need for the extra argument. The accessor object implementing the interface should contains all the state needed already.

---

pkg/sql/schema_accessors.go, line 87 at r3 (raw file):

	// TODO(whomever): when separate schemas are supported, this
	// API should be extended to use schema descriptors.
	GetObjectNames(ctx context.Context, txn *client.Txn, settings *cluster.Settings, db *DatabaseDescriptor, scName string, flags tree.DatabaseListFlags) (TableNames, error)

ditto

---

pkg/sql/schema_accessors.go, line 93 at r3 (raw file):

	// found and flags.required is true, an error is returned, otherwise
	// a nil reference is returned.
	GetObjectDesc(ctx context.Context, txn *client.Txn, settings *cluster.Settings, name *ObjectName, flags tree.ObjectLookupFlags) (ObjectDescriptor, error)

ditto

---

pkg/sql/set_zone_config.go, line 328 at r3 (raw file):

		}
		// NamespaceTableID is not in the system gossip range, but users should not
		// be allowed to set zone configs on it.

Please explain? why?

---

pkg/sql/show_zone_config.go, line 128 at r3 (raw file):

	}

	targetID, err := resolveZone(ctx, p.txn, p.ExecCfg().Settings, &zoneSpecifier)

see my comment at top

---

pkg/sql/table.go, line 156 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

you don't need this if you have the info in TableCollection already

---

pkg/sql/table.go, line 179 at r3 (raw file):

		// Resolve the database from the database cache when the transaction
		// hasn't modified the database.
		dbID, err = tc.databaseCache.getDatabaseID(ctx, settings,

ditto

---

pkg/sql/table.go, line 195 at r3 (raw file):

	phyAccessor := UncachedPhysicalAccessor{}
	obj, err := phyAccessor.GetObjectDesc(ctx, txn, settings, tn, flags)

ditto the schema accessors should store all the session state they need to make this determination

---

pkg/sql/table.go, line 216 at r3 (raw file):

	ctx context.Context,
	txn *client.Txn,
	settings *cluster.Settings,

ditto

---

pkg/sql/table.go, line 460 at r3 (raw file):

// same gateway node observe the dropped databases.
func (tc *TableCollection) waitForCacheToDropDatabases(
	ctx context.Context, settings *cluster.Settings,

ditto

---

pkg/sql/table.go, line 657 at r3 (raw file):

// in the database cache, if necessary.
func (tc *TableCollection) getAllDatabaseDescriptors(
	ctx context.Context, txn *client.Txn, settings *cluster.Settings,

ditto

---

pkg/sql/execinfrapb/api.proto, line 71 at r3 (raw file):

  optional string database = 5 [(gogoproto.nullable) = false];
  repeated string searchPath = 6;
  optional string temporarySchema = 13 [(gogoproto.nullable) = false];

it's a name right? Then better name it temporarySchemaName.
Also, our name guidelines for protobufs recommend snake_case over camelCase. The name "searchPath" was mistakenly named, it should have been search_path (I think you can change it here it will compile to the same Go identifier in the name).

---

pkg/sql/logictest/testdata/logic_test/sequences, line 4 at r3 (raw file):

# The local-mixed-19.1-19.2 configuration is excluded for this file because system.namespace
# changed  in 20.1 .
# The only tests affected by this are the tests involving KV Trace.

The tests affected by this tell you, as they were intended to that there are now 2 extra KV lookups at the start of every query (including lookups for sequences) introduced by your change, which wasn't there before.

This alone should be explained in the git commit message, together with an analysis of impact (on performance).

---

pkg/sql/opt/optbuilder/util.go, line 437 at r3 (raw file):

	// Only allow creation of objects in the public schema or Temporary Schema.
	if resName.Schema() != tree.PublicSchema && resName.Schema() != sessiondata.PgTempSchemaName {

This really ought to be addressed via permissions/privileges instead of name matches. Can you file an issue for this?

---

pkg/sql/sem/tree/name_resolution_test.go, line 768 at r3 (raw file):

		// Cases where the temporary schema has not been created yet
		{`db3.pg_temp.foo`, `db3`, mpath("public"), false, ``, ``, ``, `prefix or object not found`},
	}

very nice tests 💯

---

pkg/sql/sessiondata/search_path.go, line 28 at r3 (raw file):

// schema if/when CRDB supports them. In PG, schema names starting with `pg_`
// are "reserved", so this can never clash with an actual physical schema.
const DefaultTemporarySchema = "pg_no_temp_schema"

DefaultTemporarySchemaName

---

pkg/sql/sessiondata/search_path.go, line 204 at r3 (raw file):

		// If pg_temp is explicitly present in the paths, it must be resolved to the
		// session specific temp schema (if one exists)
		if iter.paths[iter.i-1] == PgTempSchemaName && iter.tempSchemaName != DefaultTemporarySchema {

Can you explain (to me first) why you need to check iter.tempSchemaName != DefaultTemporarySchema?

---

pkg/sql/sessiondata/search_path_test.go, line 207 at r3 (raw file):

	sp = sp.UpdatePaths([]string{"x", "pg_temp"})
	assert.True(t, sp.GetTemporarySchema() == testTempSchemaName)
}

Nice tests 💯

---

pkg/sql/sqlbase/keys.go, line 25 at r3 (raw file):

// Pass name == "" in order to generate the prefix key to use to scan over all
// of the names for the specified parentID.
// Pass settings ==  nil to construct the key for cluster versions >= 20.1 .

1. can you expand the comment to explain why there is a difference between 20.1 and previous versions (and what is this fifference).
2. nit: space before period

---

pkg/sql/sqlbase/keys.go, line 44 at r3 (raw file):

// MakeDeprecatedNameMetadataKey returns the key for a name, as expected by
// versions < 20.1 . Pass name == "" in order to generate the prefix key to use

nit: space before period.

---

pkg/sql/sqlbase/keys_test.go, line 28 at r3 (raw file):

		{MakeDescMetadataKey(123)},
		{MakeDescMetadataKey(124)},
		{NewPublicTableKey(0, "BAR", nil /* settings */).Key()},

I would have left the 4 lines at the top in their original location in the file and only added the extra arg. It makes the diff more readable.

---

pkg/sql/sqlbase/metadata.go, line 150 at r3 (raw file):

			})
		} else {
			// Initializing the system database. The database must be initialized with

I don't understand the condition parentID == keys.RootNamespaceID to imply "the system database is being initialized".

To me this condition is true for any database descriptor, not just system.
Can you explain further?

---

pkg/sql/sqlbase/metadata.go, line 166 at r3 (raw file):

		}

		// This function is called during bootstrapping, and the cluster settings are populated later.

If the cluster is being bootstrapped, then necessarily it is the new version. 😅

---

pkg/sql/sqlbase/structured.go, line 3781 at r3 (raw file):

type DatabaseKey struct {
	name     string
	settings *cluster.Settings

We don't store pointers to asynchronously mutable data structures in data structures that are meant to be functionally pure / immutable. This is not the right place to store this information.

(However this would be an OK place to store the schema style enum I describe in the top level comment)

---

pkg/sql/sqlbase/structured.go, line 3804 at r3 (raw file):

	parentSchemaID ID
	name           string
	settings       *cluster.Settings

ditto

---

pkg/sql/tests/system_table_test.go, line 36 at r3 (raw file):

	// value construction, we populate both the deprecated system.namespace table
	// and the new system.namespace table. This is done because during bootstrap,
	// cluster version isn't known.

see my comment elsewhere: we do know the version during bootstrap.

---

pkg/sql/tests/system_table_test.go, line 138 at r3 (raw file):

		}

		fmt.Println(gen)

If you keep this print, make it a t.Logf(...) instead.

---

pkg/sqlmigrations/migrations.go, line 582 at r3 (raw file):

	err := r.db.Txn(ctx, func(ctx context.Context, txn *client.Txn) error {
		b := txn.NewBatch()
		// No access to the cluster version here, so we populate both the newer

See my comments elsewhere. You do know the cluster version here.

[solongordon]

Few more files I haven't looked at yet, but I see @knz beat me to them so I'll just submit my comments for now and come back to them. :)

Reviewed 53 of 78 files at r2, 24 of 25 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @solongordon)

---

pkg/ccl/partitionccl/partition_test.go, line 1098 at r3 (raw file):

		traceLines = append(traceLines, traceLine.String)
		if strings.Contains(traceLine.String, "read completed") {
			if strings.Contains(traceLine.String, "SystemCon") || strings.Contains(traceLine.String, "NamespaceTab") {

Nit: Why not just say "NamespaceTable"? :)

---

pkg/config/system_test.go, line 297 at r3 (raw file):

	// Real system tables and partitioned user tables.
	var userSQLCopy = make([]roachpb.KeyValue, len(userSQL))
	copy(userSQLCopy, userSQL)

Nit: You could also just copy userSQL into subzoneSQL to avoid the userSQLCopy business.

---

pkg/settings/cluster/cockroach_versions.go, line 275 at r3 (raw file):

		// added parentSchemaID column. In addition to the new column, the table is
		// no longer in the system config range -- implying it is no longer gossiped.
		Key:     VersionNamespaceTableUngossip,

Nit: Maybe clearer to name this VersionTempSchemas or something. Ungossiping is more of a side effect. Good comment though!

---

pkg/sql/physical_schema_accessors.go, line 214 at r3 (raw file):

		// As this table can not be renamed by users, the first check is not required.
		if desc.Name == name.Table() ||
			(name.Table() == sqlbase.NamespaceTable.Name && name.Catalog() == sqlbase.SystemDB.Name) {

Nit: Unnecessary parens

---

pkg/sql/physical_schema_accessors.go, line 281 at r3 (raw file):

	//  might be cached.
	var obj ObjectDescriptor
	var err error

Nit: Don't need to initialize these up here anymore.

---

pkg/sql/opt/exec/execbuilder/testdata/autocommit, line 36 at r3 (raw file):

  AND message NOT LIKE '%QueryTxn%'
----
dist sender send  r24: sending batch 1 CPut, 1 EndTxn to (n1,s1):1

Could you explain why there are so many new Gets happening?

---

pkg/sql/opt/exec/execbuilder/testdata/show_trace, line 319 at r3 (raw file):

dist sender send  r26: sending batch 1 Get to (n1,s1):1
dist sender send  querying next range at /NamespaceTable/30/1/52/0/"system"/4/1
dist sender send  r26: sending batch 1 Get to (n1,s1):1

Why did this change?

---

pkg/sql/sessiondata/search_path.go, line 195 at r3 (raw file):

		iter.implicitPgTempSchema = false
		return iter.tempSchemaName, true
	} else if iter.implicitPgCatalog {

Nit: else is unnecessary.

---

pkg/sql/sessiondata/search_path.go, line 208 at r3 (raw file):

		} else if iter.paths[iter.i-1] == PgTempSchemaName {
			return iter.Next()
		}

Minor: Both clearer and more efficient to avoid doing the string comparison twice:

if iter.paths[iter.i-1] == PgTempSchemaName {
	if iter.tempSchemaName == DefaultTemporarySchema {
		return iter.Next()
	}
	return iter.tempSchemaName, true
}

---

pkg/sql/sessiondata/search_path_test.go, line 31 at r3 (raw file):

		expectedSearchPathWithoutImplicitPgSchemas                     []string
		expectedSearchPathWhenTemporarySchemaExists                    []string
		expectedSearchPathWithoutImplicitPgSchemasWhenTempSchemaExists []string

Well that's a mouthful. :)

---

pkg/sql/sessiondata/search_path_test.go, line 33 at r3 (raw file):

		expectedSearchPathWithoutImplicitPgSchemasWhenTempSchemaExists []string
	}{
		{[]string{}, /* explicitSearchPath */

Instead of comments you could just put the field names in front.

---

pkg/sql/sqlbase/structured.go, line 3805 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

The comment?

Yeah, just to specify that it will use the public schema.

---

pkg/sql/tests/system_table_test.go, line 138 at r3 (raw file):

		}

		fmt.Println(gen)

Residual print statement

---

pkg/storage/client_replica_test.go, line 1718 at r3 (raw file):

	expectedReplicas := expectedSystemRanges*systemNumReplicas + expectedUserRanges*userNumReplicas
	log.Infof(ctx, "TestSystemZoneConfig: expecting %d system ranges and %d user ranges",
		expectedSystemRanges, expectedUserRanges)

Removed accidentally?

[arulajmani]

Thanks for the reviews. After going over poins 4 - 6 . by @knz, I've changed the approach on how we select which version of the system.namespace table. I've updated the commit message to reflect this, but here is a high level overview of what has changed:

• We no longer plumb cluster settings everywhere that reads from system.namespace to decide between new/deprecated based on the ClusterVersion. Instead, we optimistically try to read from the newer system.namespace, and if that fails, we try the deprecated system.namespace.

• Every write access, that involves a key deletion, is carried out on both the new and the deprecated versions. In multi-version clusters, the new system.namespace delete is a no-op. In >= 20.1 clusters, entries from both tables are deleted if the object was created before the upgrade. This is important because otherwise we could have errors due to the fallback code when performing reads.

• Every write access that involves adding a new entry is carried out on the version corresponding to the cluster version at that moment. The tricky bit here is that the cluster version can be asynchronously updated during a transaction, so we can not rely on the (still to come as followup work) namespace table migration to successfully copy all the entries over.
  This isn't an immediate problem, as the fallback code will ensure there is no data loss. This will be a problem when the fallback code is removed (20.2?). Thus, we must run another job (sometime after the cluster version has been bumped) to account for any objects created in this race condition window. This job will copy all entries in the older system.namespace table that don't exist in the newer one. This is safe, and idempotent, because of the point above.

This new approach should therefore work seamlessly for clients connected while the cluster version is bumped!

TODO still: Benchmarks + adding the opt issue.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @knz, and @solongordon)

---

pkg/ccl/partitionccl/partition_test.go, line 1098 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Nit: Why not just say "NamespaceTable"? :)

The pretty printing of ranges concatenates "NamespaceTable" to "NamespaceTab". It does the same thing to "SystemConfig", that's why the preceding condition checks for "SystemCon" as well.

---

pkg/settings/cluster/cockroach_versions.go, line 275 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Nit: Maybe clearer to name this VersionTempSchemas or something. Ungossiping is more of a side effect. Good comment though!

What do you think of VersionNamespaceTableWithSchemas? I like that better because this change adds support for multiple schemas, not just the temporary schema. Agreed that ungossiping is a side effect though.

---

pkg/sql/conn_executor.go, line 2044 at r3 (raw file):

Previously, knz (kena) wrote…

If you need the settings, store the reference to the settings in the tablecollection. This way you don't need to pass them as argument every time.

Only needed settings in one place in the new approach. Storing settings in the table collection was the cleanest way to achieve that.

---

pkg/sql/create_table.go, line 66 at r3 (raw file):

Previously, knz (kena) wrote…

Make a method IsTemporary on TableName that takes a sessiondata argument, and checks the schema name. then call it here. It will make the code more readable.
(The method will also need to fail if the schema name has not been computed/resolved yet. You want that defensive programming to prevent control from arriving here with non-resolved names.)

also the variable should be called isTemporary.

Done.

---

pkg/sql/create_table.go, line 71 at r3 (raw file):

Previously, knz (kena) wrote…

if isTemporary { ...

Done.

---

pkg/sql/create_test.go, line 104 at r3 (raw file):

Previously, knz (kena) wrote…

I think you have this now?

Also this test is only run with the new cluster version, so this should be fine.

Misleading comment, fixed.

Aside: We don't have a migration for 19.2 -> 20.1 for copying over system.namespace and initializing public schemas for all databases copied over yet. That'll come as part of a followup task.

---

pkg/sql/database.go, line 79 at r3 (raw file):

Previously, knz (kena) wrote…

see my comments above

Done.

---

pkg/sql/database.go, line 137 at r3 (raw file):

Previously, knz (kena) wrote…

should be stored in cache, no argument needed

New approach doesn't require settings to be plumbed down.

---

pkg/sql/database.go, line 182 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

n/a

---

pkg/sql/database.go, line 231 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

n/a

---

pkg/sql/database.go, line 280 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

n/a

---

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

Previously, knz (kena) wrote…

No need for the extra argument. The resolver (p) contains all the state needed already.

New approach doesn't need settings anymore.

---

pkg/sql/logical_schema_accessors.go, line 52 at r3 (raw file):

Previously, knz (kena) wrote…

see comments elsewhere

not applicable to new approach.

---

pkg/sql/logical_schema_accessors.go, line 77 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

See my comments in physical_schema_accessors.go

---

pkg/sql/opt_catalog.go, line 146 at r1 (raw file):

Previously, knz (kena) wrote…

It sounds to me that your approach here is perverting the very definition / meaning of this method's name, ResolveSchema.

If the temp schema is specified, but does not exist yet, this method should fail!
Since it's on the read-only data path, a failure here is a true failure, meaning the temp schema name was mistakenly provided (or the requested table does not exist in the temp namespace).

In PG, if a temp schema does not exist (no temp table has been created by a session), a query of the form CREATE [TEMP] TABLE pg_temp.t(a int); is expected to succeed. This is the only case where a non-existent schema should not cause failure.

I do agree this is sort of perverting the meaning of the method's name, but this is an exceptional scenario.

---

pkg/sql/opt_catalog.go, line 109 at r3 (raw file):

Previously, knz (kena) wrote…

see comments elsewhere

no longer applicable to new approach.

---

pkg/sql/opt_catalog.go, line 141 at r3 (raw file):

Previously, knz (kena) wrote…

	// Instead of searching if the temporary schema exists, we search for the
	// public schema, which is guaranteed to exist in every database.

Is it guaranteed to exist, really? What if the user mistakenly removes the namespace entry?

Users only have read privileges on the system.namespace table, so we should be safe for now.

---

pkg/sql/physical_schema_accessors.go, line 52 at r3 (raw file):

Previously, knz (kena) wrote…

see my comments elsewhere

Not applicable anymore to new approach.

---

pkg/sql/physical_schema_accessors.go, line 105 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/physical_schema_accessors.go, line 145 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

This requires settings to be plumbed down, because in mixed version clusters, system.namespace needs to be resolved to system.namespace_deprecated.

The UnchachedPhysicalAccessor does not have access to the state, as it doesn't have access to the table collection. I've added settings to the table collection, so the plumbing down is way cleaner/reduced now.

I considered the alternative of adding the settings in the UncachedPhysicalAccessor itself, instead of modifying the interface function signature like I've done. I prefer this approach because:

• Only one of the methods requires access to the settings in this approach.
• Various places in the code (that do not have access to settings, like the database cache) create UncachedPhysicalAccessors and call other interface methods on it. Adding settings to UncachedPhysicalAccessors would've required more state storage than necessary.

---

pkg/sql/physical_schema_accessors.go, line 237 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/physical_schema_accessors.go, line 272 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/resolver.go, line 58 at r3 (raw file):

Previously, knz (kena) wrote…

No need for the extra argument. The schema accessors should contain all the state needed already.

The uncached schema accessor doesn't have the required state,

---

pkg/sql/resolver.go, line 68 at r3 (raw file):

Previously, knz (kena) wrote…

No need for the extra argument. The resolver (p) contains all the state needed already.

Not applicable anymore.

---

pkg/sql/resolver.go, line 382 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

ditto

---

pkg/sql/resolver.go, line 453 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

ditto

---

pkg/sql/schema_accessors.go, line 78 at r3 (raw file):

Previously, knz (kena) wrote…

No need for the extra argument. The accessor object implementing the interface should contains all the state needed already.

UncachedPhysicalAccessor doesn't include table collection/state required. In the new approach, only GetObjDesc requires knowledge of the state though. See my comments above

---

pkg/sql/schema_accessors.go, line 87 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

not applicable in new approach.

---

pkg/sql/schema_accessors.go, line 93 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

see my comment above

---

pkg/sql/set_zone_config.go, line 328 at r3 (raw file):

Previously, knz (kena) wrote…

Please explain? why?

No strong reason except preserving previous functionality. I figured this was out of scope for this change, but if this is a welcome side-effect I can remove this.

---

pkg/sql/show_zone_config.go, line 128 at r3 (raw file):

Previously, knz (kena) wrote…

see my comment at top

not applicable anymore.

---

pkg/sql/table.go, line 156 at r3 (raw file):

Previously, knz (kena) wrote…

you don't need this if you have the info in TableCollection already

Added the state in TableCollection

---

pkg/sql/table.go, line 179 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/table.go, line 195 at r3 (raw file):

Previously, knz (kena) wrote…

ditto the schema accessors should store all the session state they need to make this determination

See comment above about storing the state in the schema accessor vs. modifying one function signature to . include settings (instead of all, like before)

---

pkg/sql/table.go, line 216 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/table.go, line 460 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/table.go, line 657 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/execinfrapb/api.proto, line 71 at r3 (raw file):

Previously, knz (kena) wrote…

it's a name right? Then better name it temporarySchemaName.
Also, our name guidelines for protobufs recommend snake_case over camelCase. The name "searchPath" was mistakenly named, it should have been search_path (I think you can change it here it will compile to the same Go identifier in the name).

Done.

---

pkg/sql/opt/exec/execbuilder/testdata/show_trace, line 319 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Why did this change?

Can't see the diff on reviewable, but is it because we are pretty printing NamespaceTable?

---

pkg/sql/sem/tree/name_resolution_test.go, line 768 at r3 (raw file):

Previously, knz (kena) wrote…

very nice tests 💯

Thanks! 💯

---

pkg/sql/sessiondata/search_path.go, line 28 at r3 (raw file):

Previously, knz (kena) wrote…

DefaultTemporarySchemaName

Done.

---

pkg/sql/sessiondata/search_path.go, line 204 at r3 (raw file):

Previously, knz (kena) wrote…

Can you explain (to me first) why you need to check iter.tempSchemaName != DefaultTemporarySchema?

Don't want to waste a "pg_no_schema_exists" lookup if a temporary schema has not been created by the session. Cheap optimization, will comment in code.

---

pkg/sql/sessiondata/search_path_test.go, line 33 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Instead of comments you could just put the field names in front.

Done.

---

pkg/sql/sessiondata/search_path_test.go, line 207 at r3 (raw file):

Previously, knz (kena) wrote…

Nice tests 💯

Thanks!

---

pkg/sql/sqlbase/keys.go, line 25 at r3 (raw file):

Previously, knz (kena) wrote…

1. can you expand the comment to explain why there is a difference between 20.1 and previous versions (and what is this fifference).
2. nit: space before period

Done.

---

pkg/sql/sqlbase/keys_test.go, line 28 at r3 (raw file):

Previously, knz (kena) wrote…

I would have left the 4 lines at the top in their original location in the file and only added the extra arg. It makes the diff more readable.

Except the test below expects the keys to be sorted correctly. The descriptor table ID is 3, and the namespace table moved from 2 -> 30. So I had no choice but to move those above.

---

pkg/sql/sqlbase/metadata.go, line 150 at r3 (raw file):

Previously, knz (kena) wrote…

I don't understand the condition parentID == keys.RootNamespaceID to imply "the system database is being initialized".

To me this condition is true for any database descriptor, not just system.
Can you explain further?

That is true, and I'll update the comment to reflect that. This should be the behavior for all databases, not just the system database (and indeed is, we create a pubic schema by default every time we create a new database).

Only system tables/database is initialized during bootstrap though, so that is why I had the comment specific to system database earlier :)

Default databases created as part of a previous migration (defaultdb, postgres, etc) go through the normal CREATE DATABASE mechanism, so they're initialized with public schemas by default in >= 20.1 clusters.

---

pkg/sql/sqlbase/metadata.go, line 166 at r3 (raw file):

Previously, knz (kena) wrote…

If the cluster is being bootstrapped, then necessarily it is the new version. 😅

I think some testing, like mixed version logic tests, still use this code path with older versions. I've changed getInitialValues to include the bootstrap version, and now we only populate the appropriate system.namespace table.

---

pkg/sql/sqlbase/structured.go, line 3805 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Yeah, just to specify that it will use the public schema.

Done.

---

pkg/sql/sqlbase/structured.go, line 3781 at r3 (raw file):

Previously, knz (kena) wrote…

We don't store pointers to asynchronously mutable data structures in data structures that are meant to be functionally pure / immutable. This is not the right place to store this information.

(However this would be an OK place to store the schema style enum I describe in the top level comment)

No need to do this based on the new approach.

---

pkg/sql/sqlbase/structured.go, line 3804 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

na

---

pkg/sql/tests/system_table_test.go, line 36 at r3 (raw file):

Previously, knz (kena) wrote…

see my comment elsewhere: we do know the version during bootstrap.

Updated this test by modifying the signature of GetInitialValues.

---

pkg/sql/tests/system_table_test.go, line 138 at r3 (raw file):

Previously, knz (kena) wrote…

If you keep this print, make it a t.Logf(...) instead.

Slipped through, removed now.

---

pkg/sql/tests/system_table_test.go, line 138 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Residual print statement

Done.

---

pkg/sqlmigrations/migrations.go, line 582 at r3 (raw file):

Previously, knz (kena) wrote…

See my comments elsewhere. You do know the cluster version here.

Fixed! :)

---

pkg/storage/client_replica_test.go, line 1718 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Removed accidentally?

Yeah, thought that was a debug statement that I added! Fixed :)

[arulajmani]

Did some digging for the added KV accesses -- turns out we were trying to resolve under the temporary schema, even if none existed. Added a check to preempt that KV access, which got rid of (almost) all the extra "Gets" in the KV traces.

@knz does this still need a benchmark before/after in the commit message?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @knz, and @solongordon)

---

pkg/sql/logictest/testdata/logic_test/sequences, line 4 at r3 (raw file):

Previously, knz (kena) wrote…

The tests affected by this tell you, as they were intended to that there are now 2 extra KV lookups at the start of every query (including lookups for sequences) introduced by your change, which wasn't there before.

This alone should be explained in the git commit message, together with an analysis of impact (on performance).

This was because of an extra lookup was happening for the temp schema, even if it hadn't been created. I've fixed that now, so there is no change in this file, as part of this PR.

---

pkg/sql/opt/exec/execbuilder/testdata/autocommit, line 36 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Could you explain why there are so many new Gets happening?

These Gets corresponded to looking up (unsuccessfully so) the temp schema during resolution. They shouldn't happen now. I've added an optimization which simply skips the first pg_temp lookup if a temporary schema has not been created by the session.

---

pkg/sql/opt/exec/execbuilder/testdata/show_trace, line 319 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Can't see the diff on reviewable, but is it because we are pretty printing NamespaceTable?

Looked into this again, here's the explanation of why this is happening:

• During name resolution, even before this PR, we first try to resolve a 2 part name as cur_db.schema_name.table name. If that doesn't work, we try to resolve it as database_name.public.table_name. The second type of name resolution is CRDB specific, to maintain backwards compatibility with v1.

Why did this start showing up as a result of this change, if the resolution behavior hasn't changed?

• Earlier, if the schema_name !=public, the resolution would fail without needing a KV access. This is no longer the case after this PR.
• Now, if schema_name == public, we get the ID without requiring KV access (public schemas have the same ID across all databases = 29). But if the schema name is anything else, we try to resolve it in the KV layer.

---

pkg/sql/opt/optbuilder/util.go, line 437 at r3 (raw file):

Previously, knz (kena) wrote…

This really ought to be addressed via permissions/privileges instead of name matches. Can you file an issue for this?

Done: #42383

[arulajmani]

There were two tests that failed because the hardcoded system.namespace descriptor did not have an index version set, which was expected after column family support for secondary indexes was added. Corrected that, and bors-ing again.

[arulajmani]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[arulajmani]

bors r-

[craig]

Canceled

[arulajmani]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[arulajmani]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Timed out

[arulajmani]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[arulajmani]

Some (and different) trace tests are intermittently failing.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)