sql: refactoring virtualSchemaTable.allTableNames into undefinedTables

pkg/sql/BUILD.bazel

@@ -487,6 +487,7 @@ go_test(
         "upsert_test.go",
         "user_test.go",
         "values_test.go",
+        "virtual_schema_test.go",
         "virtual_table_test.go",
         "zone_config_test.go",
         "zone_test.go",

pkg/sql/information_schema.go

@@ -46,7 +46,7 @@ var pgCatalogNameDString = tree.NewDString(pgCatalogName)
 // information_schema.
 var informationSchema = virtualSchema{
 	name: sessiondata.InformationSchemaName,
-	allTableNames: buildStringSet(
+	undefinedTables: buildStringSet(
 		// Generated with:
 		// select distinct '"'||table_name||'",' from information_schema.tables
 		//    where table_schema='information_schema' order by table_name;
@@ -55,60 +55,38 @@ var informationSchema = virtualSchema{
 		"_pg_foreign_table_columns",
 		"_pg_foreign_tables",
 		"_pg_user_mappings",
-		"administrable_role_authorizations",
-		"applicable_roles",
 		"attributes",
-		"character_sets",
 		"check_constraint_routine_usage",
-		"check_constraints",
-		"collation_character_set_applicability",
-		"collations",
 		"column_domain_usage",
 		"column_options",
-		"column_privileges",
-		"column_udt_usage",
-		"columns",
-		"constraint_column_usage",
 		"constraint_table_usage",
 		"data_type_privileges",
 		"domain_constraints",
 		"domain_udt_usage",
 		"domains",
 		"element_types",
-		"enabled_roles",
 		"foreign_data_wrapper_options",
 		"foreign_data_wrappers",
 		"foreign_server_options",
 		"foreign_servers",
 		"foreign_table_options",
 		"foreign_tables",
 		"information_schema_catalog_name",
-		"key_column_usage",
-		"parameters",
-		"referential_constraints",
 		"role_column_grants",
 		"role_routine_grants",
-		"role_table_grants",
 		"role_udt_grants",
 		"role_usage_grants",
 		"routine_privileges",
-		"routines",
-		"schemata",
-		"sequences",
 		"sql_features",
 		"sql_implementation_info",
 		"sql_languages",
 		"sql_packages",
 		"sql_parts",
 		"sql_sizing",
 		"sql_sizing_profiles",
-		"table_constraints",
-		"table_privileges",
-		"tables",
 		"transforms",
 		"triggered_update_columns",
 		"triggers",
-		"type_privileges",
 		"udt_privileges",
 		"usage_privileges",
 		"user_defined_types",
@@ -117,7 +95,6 @@ var informationSchema = virtualSchema{
 		"view_column_usage",
 		"view_routine_usage",
 		"view_table_usage",
-		"views",
 	),
 	tableDefs: map[descpb.ID]virtualSchemaDef{
 		catconstants.InformationSchemaAdministrableRoleAuthorizationsID:  informationSchemaAdministrableRoleAuthorizations,

pkg/sql/pg_catalog.go

@@ -77,82 +77,18 @@ var invertedIndexOid = stringOid(indexTypeInvertedIndex)
 // in https://www.postgresql.org/docs/9.6/static/catalogs.html.
 var pgCatalog = virtualSchema{
 	name: pgCatalogName,
-	allTableNames: buildStringSet(
+	undefinedTables: buildStringSet(
 		// Generated with:
 		// select distinct '"'||table_name||'",' from information_schema.tables
 		//    where table_schema='pg_catalog' order by table_name;
-		"pg_aggregate",
-		"pg_am",
-		"pg_amop",
-		"pg_amproc",
-		"pg_attrdef",
-		"pg_attribute",
-		"pg_auth_members",
-		"pg_authid",
-		"pg_available_extension_versions",
-		"pg_available_extensions",
-		"pg_cast",
-		"pg_class",
-		"pg_collation",
-		"pg_config",
-		"pg_constraint",
-		"pg_conversion",
-		"pg_cursors",
-		"pg_database",
-		"pg_db_role_setting",
-		"pg_default_acl",
-		"pg_depend",
-		"pg_description",
-		"pg_enum",
-		"pg_event_trigger",
-		"pg_extension",
-		"pg_file_settings",
-		"pg_foreign_data_wrapper",
-		"pg_foreign_server",
-		"pg_foreign_table",
-		"pg_group",
-		"pg_hba_file_rules",
-		"pg_index",
-		"pg_indexes",
-		"pg_inherits",
 		"pg_init_privs",
-		"pg_language",
-		"pg_largeobject",
 		"pg_largeobject_metadata",
-		"pg_locks",
-		"pg_matviews",
-		"pg_namespace",
-		"pg_opclass",
-		"pg_operator",
-		"pg_opfamily",
 		"pg_partitioned_table",
 		"pg_pltemplate",
-		"pg_policies",
 		"pg_policy",
-		"pg_prepared_statements",
-		"pg_prepared_xacts",
-		"pg_proc",
-		"pg_publication",
-		"pg_publication_rel",
-		"pg_publication_tables",
-		"pg_range",
-		"pg_replication_origin",
 		"pg_replication_origin_status",
 		"pg_replication_slots",
-		"pg_rewrite",
-		"pg_roles",
-		"pg_rules",
-		"pg_seclabel",
-		"pg_seclabels",
-		"pg_sequence",
 		"pg_sequences",
-		"pg_settings",
-		"pg_shadow",
-		"pg_shdepend",
-		"pg_shdescription",
-		"pg_shmem_allocations",
-		"pg_shseclabel",
-		"pg_stat_activity",
 		"pg_stat_all_indexes",
 		"pg_stat_all_tables",
 		"pg_stat_archiver",
@@ -183,26 +119,8 @@ var pgCatalog = virtualSchema{
 		"pg_statio_user_sequences",
 		"pg_statio_user_tables",
 		"pg_statistic",
-		"pg_statistic_ext",
 		"pg_stats",
-		"pg_subscription",
 		"pg_subscription_rel",
-		"pg_tables",
-		"pg_tablespace",
-		"pg_timezone_abbrevs",
-		"pg_timezone_names",
-		"pg_transform",
-		"pg_trigger",
-		"pg_ts_config",
-		"pg_ts_config_map",
-		"pg_ts_dict",
-		"pg_ts_parser",
-		"pg_ts_template",
-		"pg_type",
-		"pg_user",
-		"pg_user_mapping",
-		"pg_user_mappings",
-		"pg_views",
 	),
 	tableDefs: map[descpb.ID]virtualSchemaDef{
 		catconstants.PgCatalogAggregateTableID:                  pgCatalogAggregateTable,

pkg/sql/pg_metadata_test.go

@@ -37,12 +37,15 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"reflect"
 	"regexp"
 	"sort"
 	"strings"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/sql/parser"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
@@ -72,15 +75,15 @@ var (
 
 // strings used on constants creations and text manipulation.
 const (
-	pgCatalogPrefix          = "PgCatalog"
-	pgCatalogIDConstant      = "PgCatalogID"
-	tableIDSuffix            = "TableID"
-	tableDefsDeclaration     = `tableDefs: map[descpb.ID]virtualSchemaDef{`
-	tableDefsTerminal        = `},`
-	allTableNamesDeclaration = `allTableNames: buildStringSet(`
-	allTableNamesTerminal    = `),`
-	virtualTablePosition     = `// typOid is the only OID generation approach that does not use oidHasher, because`
-	virtualTableTemplate     = `var %s = virtualSchemaTable{
+	pgCatalogPrefix            = "PgCatalog"
+	pgCatalogIDConstant        = "PgCatalogID"
+	tableIDSuffix              = "TableID"
+	tableDefsDeclaration       = `tableDefs: map[descpb.ID]virtualSchemaDef{`
+	tableDefsTerminal          = `},`
+	undefinedTablesDeclaration = `undefinedTables: buildStringSet(`
+	undefinedTablesTerminal    = `),`
+	virtualTablePosition       = `// typOid is the only OID generation approach that does not use oidHasher, because`
+	virtualTableTemplate       = `var %s = virtualSchemaTable{
 	comment: "%s was created for compatibility and is currently unimplemented",
 	schema:  vtable.%s,
 	populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
@@ -288,10 +291,13 @@ func fixVtable(t *testing.T, notImplemented PGMetadataTables) {
 	})
 }
 
-// fixPgCatalogGo will update pgCatalog.allTableNames, pgCatalog.tableDefs and
+// fixPgCatalogGo will update pgCatalog.undefinedTables, pgCatalog.tableDefs and
 // will add needed virtualSchemas.
-func fixPgCatalogGo(notImplemented PGMetadataTables) {
-	allTableNamesText := getAllTableNamesText(notImplemented)
+func fixPgCatalogGo(t *testing.T, notImplemented PGMetadataTables) {
+	undefinedTablesText, err := getUndefinedTablesText(notImplemented, pgCatalog)
+	if err != nil {
+		t.Fatal(err)
+	}
 	tableDefinitionText := getTableDefinitionsText(pgCatalogGo, notImplemented)
 
 	rewriteFile(pgCatalogGo, func(input *os.File, output outputFile) {
@@ -309,8 +315,8 @@ func fixPgCatalogGo(notImplemented PGMetadataTables) {
 			switch trimText {
 			case tableDefsDeclaration:
 				printBeforeTerminalString(reader, output, tableDefsTerminal, tableDefinitionText)
-			case allTableNamesDeclaration:
-				printBeforeTerminalString(reader, output, allTableNamesTerminal, allTableNamesText)
+			case undefinedTablesDeclaration:
+				printBeforeTerminalString(reader, output, undefinedTablesTerminal, undefinedTablesText)
 			}
 		}
 	})
@@ -410,7 +416,7 @@ func updateFile(inputFileName, outputFileName string, f func(input *os.File, out
 	}
 	defer dClose(input)
 
-	output, err := os.OpenFile(outputFileName, os.O_CREATE|os.O_WRONLY, 0644)
+	output, err := os.OpenFile(outputFileName, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644)
 	if err != nil {
 		panic(fmt.Errorf("error opening file %s: %v", outputFileName, err))
 	}
@@ -506,25 +512,57 @@ func printVirtualSchemas(newTableNameList PGMetadataTables) string {
 	return sb.String()
 }
 
-// getAllTableNamesText retrieves pgCatalog.allTableNames, then it merges the
+// getTableNameFromCreateTable uses pkg/sql/parser to analyze CREATE TABLE
+// statement to retrieve table name.
+func getTableNameFromCreateTable(createTableText string) (string, error) {
+	stmt, err := parser.ParseOne(createTableText)
+	if err != nil {
+		return "", err
+	}
+
+	create := stmt.AST.(*tree.CreateTable)
+	return create.Table.Table(), nil
+}
+
+// getUndefinedTablesText retrieves pgCatalog.undefinedTables, then it merges the
 // new table names and formats the replacement text.
-func getAllTableNamesText(notImplemented PGMetadataTables) string {
-	newTableNameSet := make(map[string]struct{})
-	for tableName := range pgCatalog.allTableNames {
-		newTableNameSet[tableName] = none
+func getUndefinedTablesText(newTables PGMetadataTables, vs virtualSchema) (string, error) {
+	newTableList, err := getUndefinedTablesList(newTables, vs)
+	if err != nil {
+		return "", err
 	}
-	for tableName := range notImplemented {
-		newTableNameSet[tableName] = none
+	return formatUndefinedTablesText(newTableList), nil
+}
+
+// getUndefinedTablesList checks undefinedTables in the virtualSchema and makes
+// sure they are not defined in tableDefs or are newTables to implement.
+func getUndefinedTablesList(newTables PGMetadataTables, vs virtualSchema) ([]string, error) {
+	var undefinedTablesList []string
+	removeTables := make(map[string]struct{})
+	for _, table := range vs.tableDefs {
+		tableName, err := getTableNameFromCreateTable(table.getSchema())
+		if err != nil {
+			return nil, err
+		}
+
+		removeTables[tableName] = struct{}{}
 	}
-	newTableList := make([]string, 0, len(newTableNameSet))
-	for tableName := range newTableNameSet {
-		newTableList = append(newTableList, tableName)
+
+	for tableName := range newTables {
+		removeTables[tableName] = struct{}{}
 	}
-	sort.Strings(newTableList)
-	return formatAllTableNamesText(newTableList)
+
+	for tableName := range vs.undefinedTables {
+		if _, ok := removeTables[tableName]; !ok {
+			undefinedTablesList = append(undefinedTablesList, tableName)
+		}
+	}
+
+	sort.Strings(undefinedTablesList)
+	return undefinedTablesList, nil
 }
 
-func formatAllTableNamesText(newTableNameList []string) string {
+func formatUndefinedTablesText(newTableNameList []string) string {
 	var sb strings.Builder
 	for _, tableName := range newTableNameList {
 		sb.WriteString("\t\t\"")
@@ -675,9 +713,46 @@ func TestPGCatalog(t *testing.T) {
 	rewriteDiffs(t, diffs, filepath.Join(testdata, fmt.Sprintf(expectedDiffs, *catalogName)))
 
 	if *addMissingTables {
+		validateUndefinedTablesField(t)
 		unimplemented := diffs.getUnimplementedTables(pgTables)
 		fixConstants(t, unimplemented)
 		fixVtable(t, unimplemented)
-		fixPgCatalogGo(unimplemented)
+		fixPgCatalogGo(t, unimplemented)
 	}
 }
+
+// TestPGMetadataCanFixCode checks for parts of the code this file is checking with
+// add-missing-tables flag to verify that a potential refactoring does not
+// break the code.
+func TestPGMetadataCanFixCode(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	// TODO: Add more checks, for now adding the test that is relevant for
+	// rewrite undefinedTables.
+	validateUndefinedTablesField(t)
+}
+
+// validateUndefinedTablesField checks the definition of virtualSchema objects
+// (pg_catalog and information_schema) have a undefinedTables field which can
+// be rewritten by this code.
+func validateUndefinedTablesField(t *testing.T) {
+	propertyIndex := strings.IndexRune(undefinedTablesDeclaration, ':')
+	property := undefinedTablesDeclaration[:propertyIndex]
+	// Using pgCatalog but information_schema is a virtualSchema as well
+	assertProperty(t, property, pgCatalog)
+}
+
+// assertProperty checks the property (or field) exists in the given interface.
+func assertProperty(t *testing.T, property string, i interface{}) {
+	t.Run(fmt.Sprintf("assertProperty/%s", property), func(t *testing.T) {
+		value := reflect.ValueOf(i)
+		if value.Type().Kind() != reflect.Ptr {
+			value = reflect.New(reflect.TypeOf(i))
+		}
+
+		field := value.Elem().FieldByName(property)
+		if !field.IsValid() {
+			t.Fatalf("field %s is not a field of type %T", property, i)
+		}
+	})
+}