sql: limit virtual tables to current database

[jordanlewis]

In postgres, the contents of tables in the pg_catalog and information_schema databases are limited to entries relating to the current database and system databases. This commit limits our implementation of these tables in the same way, unless the current user is root.

---

This change is 

[nvanbenschoten]

Reviewed 1 of 1 files at r1.
Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/information_schema.go, line 540 at r2 (raw file):

		_ tableLookupFn,
	) error {
		if db.Name == p.evalCtx.Database ||

How do we want this to behave if a user has no current database set?

---

Comments from Reviewable

[jordanlewis]

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/information_schema.go, line 540 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How do we want this to behave if a user has no current database set?

It seems reasonable to me that only system tables will be returned. I believe this is the current behavior.

---

Comments from Reviewable

[nvanbenschoten]

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/information_schema.go, line 540 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

It seems reasonable to me that only system tables will be returned. I believe this is the current behavior.

Ok, do we assert this behavior in tests?

---

Comments from Reviewable

[jordanlewis]

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/sql/information_schema.go, line 540 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Ok, do we assert this behavior in tests?

Good point, done. This testing actually uncovered some issues with the patch. I moved this check into forEachTableDescWithTableLookup so that pg_constraint and pg_depend and some tables from information_schema also behave correctly.

---

Comments from Reviewable

[jordanlewis]

Rebased (and reworked) this on top of #10196. I decided the easiest way to unify these two patches was a combination of temporarily setting the current database in the SHOW statements to the target database, and allowing the root user to see every row in the system tables - not just those from the current database. PTAL.

[knz]

I may be confused about what we're doing here, but it seems that the crux of the visibility criterion is whether the target database is the current database in the session.

What prevents a user from using "SET DATABASE = 'XXX'" to a database where the user has no permission and then seeing all its schema? Is that compatible with what you're trying to achieve?

---

Reviewed 3 of 6 files at r5.
Review status: 3 of 7 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/sql/information_schema.go, line 558 at r5 (raw file):

// isSystemDatabaseName returns true if the input name is not a user db name.
func isSystemDatabaseName(name string) bool {
	return name == informationSchemaName || name == pgCatalogName || name == sqlbase.SystemDB.Name

I'd see a switch here or a loop with a table rather than this disjunction which is bound to become larger and unwieldable.

---

pkg/sql/information_schema.go, line 640 at r5 (raw file):

	sort.Strings(dbNames)
	for _, dbName := range dbNames {
		if p.session.User != security.RootUser &&

I'd capture this entire condition in a helper function (*planner).isDatabaseVisible(dbName).

---

pkg/sql/testdata/information_schema, line 641 at r5 (raw file):

def                 constraint_column  primary          def            constraint_column  t3          rowid        1                 NULL

# TODO(nvanbenschoten) blocked on #8497.

I think #8497 has been solved.

---

Comments from Reviewable

[knz]

LGTM otherwise btw.

---

Review status: 3 of 7 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[jordanlewis]

There's nothing preventing a user from doing that right now, but it's still compatible with the goal of this patch, which is enabling sufficient compatibility with postgres behavior to allow OID casts to work consistently. In postgres, I don't think users can connect to arbitrary databases, FWIW.

TFTR!

---

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/sql/information_schema.go, line 558 at r5 (raw file):

Previously, knz (kena) wrote…

I'd see a switch here or a loop with a table rather than this disjunction which is bound to become larger and unwieldable.

Done.

---

pkg/sql/information_schema.go, line 640 at r5 (raw file):

Previously, knz (kena) wrote…

I'd capture this entire condition in a helper function (*planner).isDatabaseVisible(dbName).

Done.

---

pkg/sql/testdata/information_schema, line 641 at r5 (raw file):

Previously, knz (kena) wrote…

I think #8497 has been solved.

I fixed this in a different PR. I'll pick up in a rebase.

---

Comments from Reviewable

[jordanlewis]

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/sql/testdata/information_schema, line 641 at r5 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I fixed this in a different PR. I'll pick up in a rebase.

Err, no I didn't. Fixed - thanks for catching this!

---

Comments from Reviewable

[knz]

Reviewed 1 of 1 files at r7, 5 of 7 files at r8.
Review status: 6 of 8 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

pkg/sql/information_schema.go, line 558 at r5 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Done.

What about:

switch name {
   case informationSchemaName:
   case pgCatalogName:
   case sqlbase.SystemDB.Name:
   default:
     return false
}
return true

---

pkg/sql/logic_test.go, line 402 at r8 (raw file):

			if inDBName != outDBName {
				// Propagate the DATABASE setting to the newly-live connection.
				if _, err := t.db.Exec(fmt.Sprintf("SET DATABASE = '%s'", inDBName)); err != nil {

I don' think this change is useful/necessary. (If you really care about escaping special chars in the db name then you'd need sqlEscape or something.
(or this perhaps a base change? Hard to tell.)

---

Comments from Reviewable

[jordanlewis]

Review status: 6 of 8 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

pkg/sql/information_schema.go, line 558 at r5 (raw file):

Previously, knz (kena) wrote…

What about:

switch name {
   case informationSchemaName:
   case pgCatalogName:
   case sqlbase.SystemDB.Name:
   default:
     return false
}
return true

Much better thanks!

---

pkg/sql/logic_test.go, line 402 at r8 (raw file):

Previously, knz (kena) wrote…

I don' think this change is useful/necessary. (If you really care about escaping special chars in the db name then you'd need sqlEscape or something.
(or this perhaps a base change? Hard to tell.)

The reason this is necessary is that the database name can be empty in tests. If that's the case, this line will fail without the extra quoting.

---

Comments from Reviewable

[knz]

oh okthx