sql: code generation for missing pg catalog tables

[mnovelodou]

Previously, programmer had to add missing table manually,
This was inadequate because postgress added a lot of tables and
manual process can lead to coding mistakes or gaps
To address this, diff tool now have the hability to generate the
missing code

Release note: None

Fixes #58001

[blathers-crl]

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

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

• @rafiss (author of sql: create empty pg_catalog tables that are missing #58001)

_{🦉 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.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

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

[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]

Do we need this extra here? Shouldn't the return err handle it, we can also wrap the error instead.

[mnovelodou]

The problem of not having this is that the error is eventually wrapped and manipulated later in the code. During my tests I end up having a bad create table that I was unable to find which definition until I did some debugging of the code.

I strongly recommend to leave that log, in a production environment I don't believe it gets printed.

[RichardJCai]

Rename to tableDefCloseBracket (not sure Closure is the right word here)

[mnovelodou]

I named it closure because I was thinking of the function "printsBeforeClosure" probably closure is not the right word,
What printsBeforeClosure does:
It will read and skips all the lines (Except comments) until find the "closure" line, then it will print the s text and the "closure" text.

Please let me know if you believe there is a better word for this.

[RichardJCai]

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

---

pkg/sql/pg_catalog_diff.go, line 191 at r4 (raw file):

}

// getNotImplementedTables retrieves tables that are not yet part of crdb

Would like this comment to be more specific, I guess in this case it would be retrieves PGCatalog tables that are not yet part of CRDB.

Also in general comments should finished off with periods (valid sentences)

---

pkg/sql/pg_catalog_diff.go, line 202 at r4 (raw file):

}

//AreAllTypesImplemented verifies that all the types are implemented in cockroach db

Missing space from comment to first word.

Also the function name should be updated in the comment.

---

pkg/sql/pg_catalog_test.go, line 72 at r5 (raw file):

Closure

I suggest we just name it tableDefCloseBracket it's more clear in my opinion.

---

pkg/sql/pg_catalog_test.go, line 373 at r5 (raw file):

push back
Sure, I'm okay with leaving it if it is a big effort.

---

pkg/sql/virtual_schema.go, line 180 at r5 (raw file):

Previously, mnovelodou (Miguel Novelo) wrote…

The problem of not having this is that the error is eventually wrapped and manipulated later in the code. During my tests I end up having a bad create table that I was unable to find which definition until I did some debugging of the code.

I strongly recommend to leave that log, in a production environment I don't believe it gets printed.

Instead of logging here, would it be alright to wrap the returned error?
We can do errors.Wrapf(err, "initVirtualDesc problem: %s", t.schema)

If the returned error is unclear, I think we should make it clear instead of adding a log.

[mnovelodou]

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

---

pkg/sql/pg_catalog_diff.go, line 191 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Would like this comment to be more specific, I guess in this case it would be retrieves PGCatalog tables that are not yet part of CRDB.

Also in general comments should finished off with periods (valid sentences)

Done.

---

pkg/sql/pg_catalog_diff.go, line 202 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Missing space from comment to first word.

Also the function name should be updated in the comment.

Done.

---

pkg/sql/pg_catalog_test.go, line 72 at r5 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Closure

I suggest we just name it tableDefCloseBracket it's more clear in my opinion.

Done.

---

pkg/sql/pg_catalog_test.go, line 373 at r5 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

push back
Sure, I'm okay with leaving it if it is a big effort.

Done.

---

pkg/sql/virtual_schema.go, line 180 at r5 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Instead of logging here, would it be alright to wrap the returned error?
We can do errors.Wrapf(err, "initVirtualDesc problem: %s", t.schema)

If the returned error is unclear, I think we should make it clear instead of adding a log.

Done.

[RichardJCai]

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

---

pkg/sql/pg_metadata_test.go, line 78 at r6 (raw file):

	tableIDSuffix            = "TableID"
	tableDefsDeclaration     = `tableDefs: map[descpb.ID]virtualSchemaDef{`
	tableDefsCloseBracket    = `},`

Maybe we should name this tableDefsTerminal

---

pkg/sql/pg_metadata_test.go, line 80 at r6 (raw file):

	tableDefsCloseBracket    = `},`
	allTableNamesDeclaration = `allTableNames: buildStringSet(`
	allTableNamesClose       = `),`

allTablesNameTerminal

---

pkg/sql/pg_metadata_test.go, line 318 at r6 (raw file):

// printBeforeTerminalString will skip all the lines and print `s` text when finds the terminal string.
func printBeforeTerminalString(

I realized both "TerminalStrings" are actually chars (just a close bracket) I think we can rename this to printBeforeTerminalChar

---

pkg/sql/pg_metadata_test.go, line 675 at r6 (raw file):

	if *addMissingTables {
		notImplemented := diffs.getUnimplementedTables(pgTables)

notImplemented -> unimplemented

---

pkg/sql/virtual_schema.go, line 180 at r6 (raw file):

	)
	if err != nil {
		err = errors.Wrapf(err, "initVirtualDesc problem: %v\n%s", err, t.schema)

I think this is wrapping the error twice. errors.Wrapf will wrap the error for you, so we don't need to pass the err the second time.

[mnovelodou]

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

---

pkg/sql/pg_metadata_test.go, line 78 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Maybe we should name this tableDefsTerminal

Done.

---

pkg/sql/pg_metadata_test.go, line 80 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

allTablesNameTerminal

Done.

---

pkg/sql/pg_metadata_test.go, line 318 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I realized both "TerminalStrings" are actually chars (just a close bracket) I think we can rename this to printBeforeTerminalChar

Not Really, tableDefsTerminal is ), and all tableDefsTerminal is ), (Both have commas) and they check the trimmed entire line (This is not looking at a single character).

If you look at the function this terminalString is compared with trimText which is a line read from the file without blank spaces at beginning and end of the string.

---

pkg/sql/pg_metadata_test.go, line 675 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

notImplemented -> unimplemented

Done.

---

pkg/sql/virtual_schema.go, line 180 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think this is wrapping the error twice. errors.Wrapf will wrap the error for you, so we don't need to pass the err the second time.

Done.

[RichardJCai]

One thing I just realized is that the allTableNames list for schema definitions is not actually the source of truth for what tables we implement. Ie we don't implement pg_sequence but it's listed there.

I think this is fine to merge as is since tables that appear in allTableNames will return an unimplemented error if they're used and we already merged tables using this tool.

#61801 I opened an issue here.

What we can do to actually check which tables we implement is getting the tables using NewVirtualSchemaHolder in virtual_schema.go. This way we can check what actually works.

Either way I'm okay with merging this and iterating since this solution implements the tables that we did not list in allTableNames, we just have to change the soltuion to also add tables that we don't implement but also appear in allTableNames

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

[RichardJCai]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)