sql/resolver: wrong error when db does not exist for virtual schemas

[fqazi]

Fixes: #68060

Previously, when a database did not exist under a virtual
schema the code would fall through do a normal look up.
Before a recent refactor of the some of the internals,
we accidentally changed behavior, so that an undefined
relation error was returned. To address this, this patch
adds a check inside the resolver layer to return the correct
error instead of falling through.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

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

---

pkg/sql/catalog/resolver/resolver.go, line 377 at r1 (raw file):

Quoted 5 lines of code…

			if found, prefix, result, err = r.LookupObject(ctx, lookupFlags, curDb, u.Schema(), u.Object()); found || err != nil {
				prefix.ExplicitDatabase = false
				prefix.ExplicitSchema = true
				return found, prefix, result, err
			} else if isVirtualSchema && !found && lookupFlags.Required {

how does this work? I see that it does, but it's surprising. The reason it's surprising is that you'll only enter the second condition if Required is true, but that means that I'd expect an err to be non-nil in which case, you would have entered the preceding branch. Did you confirm from coverage that you hit this.

It might be nice also to add more of an integration test. Here's the bad behavior @rafiss saw. Maybe something in pkg/cli/interactive_tests/?

Andrew-Werner's-MacBook-Pro:cockroach ajwerner$ ./cockroach sql --url postgresql://root@localhost:26257/mydb --insecure
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
warning: unable to retrieve the server's version: pq: relation "crdb_internal.node_build_info" does not exist
#
# Enter \? for a brief introduction.
#
warning: error retrieving the database name: pq: relation "crdb_internal.session_variables" does not exist
root@localhost:26257/?>

[ajwerner]

Seems like the behavior change, while seeming sound, breaks stuff.

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

[fqazi]

Yes, I accidentally left another change in there too. I pushed the error back into ResolveExisting, since we need to know if the schema is virtual. We can move it to a higher level, but it will lead to tons of code duplication since we need to pass around if the prefix was virtual.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gh-casper and @rafiss)

---

pkg/sql/catalog/resolver/resolver.go, line 377 at r1 (raw file):

Previously, ajwerner wrote…

			if found, prefix, result, err = r.LookupObject(ctx, lookupFlags, curDb, u.Schema(), u.Object()); found || err != nil {
				prefix.ExplicitDatabase = false
				prefix.ExplicitSchema = true
				return found, prefix, result, err
			} else if isVirtualSchema && !found && lookupFlags.Required {

how does this work? I see that it does, but it's surprising. The reason it's surprising is that you'll only enter the second condition if Required is true, but that means that I'd expect an err to be non-nil in which case, you would have entered the preceding branch. Did you confirm from coverage that you hit this.

It might be nice also to add more of an integration test. Here's the bad behavior @rafiss saw. Maybe something in pkg/cli/interactive_tests/?

Andrew-Werner's-MacBook-Pro:cockroach ajwerner$ ./cockroach sql --url postgresql://root@localhost:26257/mydb --insecure
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
warning: unable to retrieve the server's version: pq: relation "crdb_internal.node_build_info" does not exist
#
# Enter \? for a brief introduction.
#
warning: error retrieving the database name: pq: relation "crdb_internal.session_variables" does not exist
root@localhost:26257/?>

Contract wise we expect the higher-level layers to generate errors, but it is extremely tricky in this case. The name prefix resolution stuff would either need to say if the schema is virtual schema or we can modify things so that this layer

Yes, there was an existing test, which I extended to include the full error.

[ajwerner]

Reviewed 1 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @gh-casper)

---

pkg/ccl/backupccl/backupresolver/targets.go, line 105 at r5 (raw file):

	ctx context.Context, flags tree.ObjectLookupFlags, dbName, scName, obName string,
) (bool, catalog.ResolvedObjectPrefix, catalog.Descriptor, error) {
	resolvedPrefix := catalog.ResolvedObjectPrefix{}

why this change?

---

pkg/sql/catalog/resolver/resolver.go, line 377 at r5 (raw file):

				prefix.ExplicitDatabase = false
				prefix.ExplicitSchema = true
				if !found && err == nil && isVirtualSchema && prefix.Database == nil {

is the prefix.Database == nil check necessary?

---

pkg/sql/catalog/resolver/resolver.go, line 378 at r5 (raw file):

				prefix.ExplicitSchema = true
				if !found && err == nil && isVirtualSchema && prefix.Database == nil {
					// If the database was not found during the lookup for a virtual schema

Can you also note that this is ignoring the Required flag.

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @gh-casper)

---

pkg/ccl/backupccl/backupresolver/targets.go, line 105 at r5 (raw file):

Previously, ajwerner wrote…

why this change?

For the backup/changefeed case the resolver did not follow the same guarantees, so it was previously always returning an empty prefix. We need to know if the table was missing or if the DB was missing. The planner implementations of LookupObject always populate ResolvedObjectPrefix, and this was the only exception. So, this corrects the behaviour and fixes the error returned on a statement like:

EXPERIMENTAL CHANGEFEED FOR information_schema.tables;

I'll add a comment saying:

// LookupObject requires that the ResolvedObjectPrefix is populated even if the object isn't found. If the database and schema are resolved, then we should at least return those.

---

pkg/sql/catalog/resolver/resolver.go, line 377 at r5 (raw file):

Previously, ajwerner wrote…

is the prefix.Database == nil check necessary?

Yes, we need to distinguish for cases where the database is found but the object itself isn't found. One such case I noticed was:

SELECT 1::pg_catalog.special_int, this should return a type error. I'll make this more clear in the comment too.

---

pkg/sql/catalog/resolver/resolver.go, line 378 at r5 (raw file):

Previously, ajwerner wrote…

Can you also note that this is ignoring the Required flag.

Good point, let me add that in

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @gh-casper)

[fqazi]

TFTR @ajwerner!

[fqazi]

bors r=ajwerner

[craig]

Build succeeded:

• GitHub CI (Cockroach)