sql: fix statement bundle creation when memo isn't detached

[cucaroach]

When a memo is deemed not resuable we don't detach it from the factory
and this causes problems later when we execute SetIndexRecommendations
which resets the optimizer context which will reset the memo. This
causes the schema.sql and opt*.txt files to be empty.

Fixes: #92920

Release note: None

[cockroach-teamcity]

This change is 

[rytaft]

Great find!

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach)

---

pkg/sql/plan_opt.go line 510 at r1 (raw file):

//
// The returned memo is only safe to use in one thread, during execution of the
// current statement.

Do we need this warning anymore? (not that it did much good before...)

---

pkg/sql/opt/norm/factory.go line 179 at r1 (raw file):

	m := f.mem
	f.mem = nil
	f.Init(f.ctx, f.evalCtx, nil /* catalog */)

Do we need to call Init here? Would it be enough to just return m and set f.mem to nil?

[mgartner]

Nice!

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @rytaft)

---

pkg/sql/opt/norm/factory.go line 179 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Do we need to call Init here? Would it be enough to just return m and set f.mem to nil?

Seems like Init shouldn't be necessary. Looks like opc.reset() is called whenever an optimizer factory is reused, which calls the factory's Init via the optimizer's Init. Even in DetachMemo above, f.Init looks redundant - maybe it was just for additional safety?

---

pkg/sql/explain_bundle_test.go line 109 at r1 (raw file):

		checkBundle(t, fmt.Sprint(rows), "", func(name, contents string) error {
			if name == "opt.txt" {
				if contents == "no plan" {

nit: can you create a constant for this string that's used in both pkg/sql/explain_bundle.go and this test? That provides a little bit more safety to prevent this test from becoming a test that always passes if someone changes the "no plan" text in pkg/sql/explain_bundle.go without updating this test.

[cucaroach]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

---

pkg/sql/plan_opt.go line 510 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Do we need this warning anymore? (not that it did much good before...)

I'm not sure.

---

pkg/sql/opt/norm/factory.go line 179 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Seems like Init shouldn't be necessary. Looks like opc.reset() is called whenever an optimizer factory is reused, which calls the factory's Init via the optimizer's Init. Even in DetachMemo above, f.Init looks redundant - maybe it was just for additional safety?

👍

---

pkg/sql/explain_bundle_test.go line 109 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: can you create a constant for this string that's used in both pkg/sql/explain_bundle.go and this test? That provides a little bit more safety to prevent this test from becoming a test that always passes if someone changes the "no plan" text in pkg/sql/explain_bundle.go without updating this test.

Good idea

[rytaft]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach and @mgartner)

---

pkg/sql/plan_opt.go line 510 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

I'm not sure.

Ok, no harm in leaving it I guess...

---

pkg/sql/opt/norm/factory.go line 175 at r2 (raw file):

// ReleaseMemo is just like DetachMemo but it doesn't call Detach on the memo
// preserving any statistics information for explain purposes (distinct, null
// count etc).

nit: update comment since now it also doesn't call Init anymore

[cucaroach]

bors r+

[mgartner]

LGTM

Reviewed 1 of 3 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)