Skip to content

Commit

Permalink
Merge #63144
Browse files Browse the repository at this point in the history 
63144: sql: refactoring virtualSchemaTable.allTableNames into undefinedTables r=RichardJCai a=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

Co-authored-by: MiguelNovelo <[email protected]>
  • Loading branch information
craig[bot] and MiguelNovelo committed Apr 12, 2021
2 parents 175820b + 345f9d7 commit 1068d19
Show file tree
Hide file tree
Showing 6 changed files with 214 additions and 144 deletions.
1 change: 1 addition & 0 deletions pkg/sql/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
25 changes: 1 addition & 24 deletions pkg/sql/information_schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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",
Expand All @@ -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,
Expand Down
84 changes: 1 addition & 83 deletions pkg/sql/pg_catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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,
Expand Down
133 changes: 104 additions & 29 deletions pkg/sql/pg_metadata_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
}
}
})
Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -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\"")
Expand Down Expand Up @@ -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)
}
})
}
Loading

0 comments on commit 1068d19

Please sign in to comment.