sql: refactoring virtualSchemaTable.allTableNames into undefinedTables

[mnovelodou]

Previously, allTableNames have implemented and unimplemented tableNames
This was inadequate because the duplicity makes difficult to maintain
To address this, this patch refactors allTableNames into
undefinedTables to keep only the tables that are not in tableDefs

Release note: None

Fixes #61801

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[RichardJCai]

One thought I had was instead of referring to these tables as unimplementedTables, maybe we should use undefinedTables since we have tables that are defined but technically not implemented as well.

These tables can strictly be the ones that are undefined. Thoughts?

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

---

pkg/sql/pg_metadata_test.go, line 82 at r1 (raw file):

	tableDefsDeclaration               = `tableDefs: map[descpb.ID]virtualSchemaDef{`
	tableDefsTerminal                  = `},`
	unimplementedTableNamesDeclaration = `unimplementedTableNames: buildStringSet(`

One thing I just thought of is if we change the code in pg_catalog.go, if we rename this variable again for whatever reason, we might miss updating the corresponding code here.

Is there a test or anything that keeps this in check? I definitely see this getting or some part of this code being updated in the future.
What happens right now if we update the variable name for unimplementedTableNames without changing this code here?

---

pkg/sql/pg_metadata_test.go, line 515 at r1 (raw file):

// getTableNameFromCreateTable uses pkg/sql/parser to analize CREATE TABLE
// statement to retrieve table name

nit: Missing period

---

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

// VirtualSchemaTest validates virtualSchema that unimplementedTableNames
// only have tables that do not have a virtualSchemaTable defined.

nit: only has tables

[mnovelodou]

Love it, I've been having difficulties to separate what is a unimplemented and undefined. Let me work on that change.

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

---

pkg/sql/pg_metadata_test.go, line 82 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

One thing I just thought of is if we change the code in pg_catalog.go, if we rename this variable again for whatever reason, we might miss updating the corresponding code here.

Is there a test or anything that keeps this in check? I definitely see this getting or some part of this code being updated in the future.
What happens right now if we update the variable name for unimplementedTableNames without changing this code here?

If you don't change the code here, it will break the code in charge of modifying the unimplementedTableNames, I mean, no actual production code will break here, What I can do is to create a goTest that validates this unimplementedTableNamesDeclaration is a field of virtualSchema, I think I can do some reflection here.

---

pkg/sql/pg_metadata_test.go, line 515 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: Missing period

Done.

---

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

Previously, RichardJCai (Richard Cai) wrote…

nit: only has tables

Done.

[RichardJCai]

Looks pretty much good to me, small nits.

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

---

pkg/sql/pg_metadata_test.go, line 515 at r2 (raw file):

}

// getTableNameFromCreateTable uses pkg/sql/parser to analize CREATE TABLE

analize -> analyze

---

pkg/sql/pg_metadata_test.go, line 529 at r2 (raw file):

// getUndefinedTablesText retrieves pgCatalog.undefinedTables, then it merges the
// new table names and formats the replacement text.
func getUndefinedTablesText(notImplemented PGMetadataTables, vs virtualSchema) (string, error) {

I think notImplemented should become undefinedTables here

---

pkg/sql/pg_metadata_test.go, line 543 at r2 (raw file):

	removeTables := make(map[string]struct{})
	for _, table := range vs.tableDefs {
		tableName, err := getTableNameFromCreateTable(table.getSchema())

I'm surprised we have to do all this work to grab the name of the table, we probably shoulda added a name field and populated it upstream but I don't want to introduce more work here right now since that would be fairly tedious.

[mnovelodou]

Done.

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

---

pkg/sql/pg_metadata_test.go, line 515 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

analize -> analyze

Done.

---

pkg/sql/pg_metadata_test.go, line 529 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think notImplemented should become undefinedTables here

Actually, they are notImplemented that will be defined as tableDefs after this process ran, so here it is correct, but I will rename it as newTables for better readability

---

pkg/sql/pg_metadata_test.go, line 543 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I'm surprised we have to do all this work to grab the name of the table, we probably shoulda added a name field and populated it upstream but I don't want to introduce more work here right now since that would be fairly tedious.

I don't feel it like a lot of work, initially in this file I was using a regular expression, here I am just reusing code that parses the CREATE TABLE but I don't feel it is really a big deal. I think this isn't likely to fail due to a refactoring unless you stop using "CREATE TABLE" for this.

By the way adding a name might do this code more prone for human failure as there is CREATE TABLE name and there would be another tableName field.

[RichardJCai]

Thanks for this! Overall looks good to me

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

---

pkg/sql/pg_metadata_test.go, line 543 at r2 (raw file):

Previously, mnovelodou (Miguel Novelo) wrote…

I don't feel it like a lot of work, initially in this file I was using a regular expression, here I am just reusing code that parses the CREATE TABLE but I don't feel it is really a big deal. I think this isn't likely to fail due to a refactoring unless you stop using "CREATE TABLE" for this.

By the way adding a name might do this code more prone for human failure as there is CREATE TABLE name and there would be another tableName field.

Yeah that's fair

---

pkg/sql/pg_metadata_test.go, line 741 at r3 (raw file):

	propertyIndex := strings.IndexRune(undefinedTablesDeclaration, ':')
	property := undefinedTablesDeclaration[:propertyIndex]
	// Using pgCatalog but information_schema is a virtualSchema as well

Reading off your comment here, should we check information_schema as well?

[mnovelodou]

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

---

pkg/sql/pg_metadata_test.go, line 543 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Yeah that's fair

Done.

---

pkg/sql/pg_metadata_test.go, line 741 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Reading off your comment here, should we check information_schema as well?

I don't think so, this is checking the type, not a particular object, I coudn't see how to just use the type for doing this, so I am passing pg_catalog, if you have a suggestion, I can check using another way.

[RichardJCai]

Sounds good to me

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)