sql: add crdb_internal.show_create_all_tables builtin

[RichardJCai]

sql: add crdb_internal.show_create_all_tables builtin

Making this a PR first before continuing with #53488, we can alias this builtin with SHOW CREATE ALL TABLES.

Example output:

query T
SELECT crdb_internal.show_create_all_tables('d')
----
CREATE TABLE public.parent (
  x INT8 NULL,
  y INT8 NULL,
  z INT8 NULL,
  UNIQUE INDEX parent_x_y_z_key (x ASC, y ASC, z ASC),
  UNIQUE INDEX parent_x_key (x ASC),
  FAMILY f1 (x, y, z, rowid)
);
CREATE TABLE public.full_test (
  x INT8 NULL,
  y INT8 NULL,
  z INT8 NULL,
  UNIQUE INDEX full_test_x_key (x ASC),
  FAMILY f1 (x, y, z, rowid)
);
CREATE SEQUENCE public.s MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1;
CREATE VIEW public.vx ("?column?") AS SELECT 1;
ALTER TABLE public.full_test ADD CONSTRAINT fk_x_ref_parent FOREIGN KEY (x, y, z) REFERENCES public.parent(x, y, z) MATCH FULL ON DELETE CASCADE ON UPDATE CASCADE;
ALTER TABLE public.full_test ADD CONSTRAINT test_fk FOREIGN KEY (x) REFERENCES public.parent(x) ON DELETE CASCADE;
-- Validate foreign key constraints. These can fail if there was unvalidated data during the SHOW CREATE ALL TABLES
ALTER TABLE public.full_test VALIDATE CONSTRAINT fk_x_ref_parent;
ALTER TABLE public.full_test VALIDATE CONSTRAINT test_fk;

Release note (sql change): crdb_internal.show_create_all_tables is
a new builtin that takes in a database name (string) and returns
a flat log of all the CREATE TABLE statements in the database followed
by alter statements to add constraints. The output can be used
to recreate a database. This builtin was added to replace old dump logic.

[cockroach-teamcity]

This change is 

[rafiss]

awesome! just have relatively minor comments

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

---

pkg/sql/logictest/testdata/logic_test/show_create_all_tables_builtin, line 2 at r1 (raw file):

query T
SELECT crdb_internal.show_create_all_tables('d')

nice tests! maybe you did this already, but perhaps you could take a look to see if dump used to have any other good test cases that we could pull in here too?

---

pkg/sql/sem/builtins/builtins.go, line 4829 at r1 (raw file):

	),

	"crdb_internal.show_create_all_tables": makeBuiltin(

hmm random thing i just thought of -- is this OK to have in crdb_internal? aren't things in crdb_internal usually experimental/undocumented? (idk though maybe it's too sketchy to put into the normal namespace)

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 39 at r1 (raw file):

}

func showCreateAllTablesBuiltin(evalCtx *tree.EvalContext, arg tree.Datums) (tree.Datum, error) {

nit: could you add a comment?

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 46 at r1 (raw file):

	if len(mds) == 0 {
		return tree.NewDString("no tables found"), nil

is this what dump used to return? i wonder if it would be better for this function to always return valid SQL so that way a tool that relies on it doesn't get confused. (in this case it could just return NULL or an empty string

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 111 at r1 (raw file):

}

func getMetadataForTablesInDB(evalCtx *tree.EvalContext, arg tree.Datums) ([]tableMetadata, error) {

need a comment

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 135 at r1 (raw file):

}

func getTableMetadata(

need a comment

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 264 at r1 (raw file):

}

func collect(tid int64, byID map[int64]tableMetadata, seen map[int64]bool, collected *[]int64) {

need a comment -- could you mention why a table could get collected twice?

[RichardJCai]

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

---

pkg/sql/logictest/testdata/logic_test/show_create_all_tables_builtin, line 2 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nice tests! maybe you did this already, but perhaps you could take a look to see if dump used to have any other good test cases that we could pull in here too?

Yep pulled some more tests in, good call

---

pkg/sql/sem/builtins/builtins.go, line 4829 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hmm random thing i just thought of -- is this OK to have in crdb_internal? aren't things in crdb_internal usually experimental/undocumented? (idk though maybe it's too sketchy to put into the normal namespace)

After some offline discussion, I think leaving this in crdb_internal is fine and we'll expose this through SHOW CREATE ALL TABLES

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 46 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is this what dump used to return? i wonder if it would be better for this function to always return valid SQL so that way a tool that relies on it doesn't get confused. (in this case it could just return NULL or an empty string

Updated to return empty string

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 111 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

need a comment

Done.

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 135 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

need a comment

Done.

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 264 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

need a comment -- could you mention why a table could get collected twice?

Done.

[rafiss]

looks great! thanks

Reviewable status: complete! 0 of 0 LGTMs obtained

[RichardJCai]

Thanks for the quick reviews!
bors r=rafiss

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[otan]

bors r-

this needs a rebase and re-TESTFLAGS='-rewrite'

[craig]

Canceled.

[RichardJCai]

Updated!
bors r=rafiss

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[otan]

bors r-

[23:46:18][Ensure generated code is up-to-date] pkg/sql/sem/builtins/show_create_all_tables_builtin.go:204:39: evalCtx.InternalExecutor.Query undefined (type tree.InternalExecutor has no field or method Query)
[23:46:18][Ensure generated code is up-to-date] pkg/sql/sem/builtins/show_create_all_tables_builtin.go:254:39: evalCtx.InternalExecutor.Query undefined (type tree.InternalExecutor has no field or method Query)

[craig]

Canceled.

[rafiss]

looks like @yuzefovich just removed that function in #60428. it seems like we have to use QueryIterator now?

[yuzefovich]

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

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 204 at r2 (raw file):

		WHERE descriptor_id = $1
		`, dbName, ts)
	rows, err := evalCtx.InternalExecutor.Query(

Yeah, you'll need to use QueryIterator method here. The code should be refactored to something like:

it, err := evalCtx.InternalExecutor.QueryIterator(...)
if err != nil {
  return tableMetadata{}, err
}
var refs []int64
var ok bool
for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {
  id := tree.MustBeDInt(it.Cur()[0])
  refs = append(refs, int64(id))
}
if err != nil {
  return tableMetadata{}, err
}
...

[RichardJCai]

This time for sure!
bors r=rafiss

[rafiss]

bors r-

[craig]

Canceled.

[rafiss]

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

---

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 205 at r3 (raw file):

		WHERE descriptor_id = $1
		`, dbName, ts)
	it, err := evalCtx.InternalExecutor.(sqlutil.InternalExecutor).QueryIterator(

we don't want to do a type assertion here -- instead add QueryIterator to the pkg/sql/sem/tree.InternalExecutor interface

[RichardJCai]

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

pkg/sql/sem/builtins/show_create_all_tables_builtin.go, line 205 at r3 (raw file):

		WHERE descriptor_id = $1
		`, dbName, ts)
	it, err := evalCtx.InternalExecutor.(sqlutil.InternalExecutor).QueryIterator(

we don't want to do a type assertion here -- instead add QueryIterator to the pkg/sql/sem/tree.InternalExecutor interface

Can't add QueryIteratorEx to the InternalExecutor interface due to cyclic dependencies.
Added a followup issue to remove that Interface completely #60507

[RichardJCai]

Ok this time for sure for sure.

bors r=rafiss

[craig]

Build succeeded:

• GitHub CI (Cockroach)