sql: optimizer always on

[RaduBerinde]

sql, workload: don't disable the optimizer in tests

In preparation for removing the possibility of turning off the
optimizer, adjust tests and benchmarks that are disabling the
optimizer.

Release note: None

sql: optimizer always on

This change deprecates the optimizer session setting and associated
cluster setting. It is no longer possible to disable the optimizer.
The settings still exist but the only allowed value is "on" (mostly so
that SET OPTIMIZER = ON still works).

The logic test configurations are reworked: they now all use the
optimizer, and the -opt versions are gone (e.g. what used to be
local-opt is now local). The HP-specific planner tests are
removed.

Release note: None

[cockroach-teamcity]

This change is 

[knz]

This must have been tedious. But good riddance! 💯

Reviewed 7 of 7 files at r1, 198 of 198 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @RaduBerinde)

---

pkg/sql/exec_util.go, line 125 at r2 (raw file):

	"on",
	map[int64]string{
		1: "on",

I think the settings machinery is not happy to see a value stored in kv that's not valid according to the map here. Not sure. In either case, maybe worth considering a migration to reset the value? Or you could remove it entirely, and add the name to retiredSettings in settings/registry.go.

In any case you should call SetDeprecated() on the setting if you keep it.

---

pkg/sql/logictest/testdata/logic_test/explain_analyze_plans, line 35 at r2 (raw file):

# Verify data placement.
query TTTI colnames,rowsort

Can you explain why this changed.

---

pkg/sql/logictest/testdata/logic_test/explain_analyze_plans, line 69 at r2 (raw file):

SELECT url FROM [EXPLAIN ANALYZE (DISTSQL) SELECT DISTINCT(kw.w) FROM kv JOIN kw ON kv.k = kw.w ORDER BY kw.w]
----
https://cockroachdb.github.io/distsqlplan/decode.html#eJzkWV1v2zYUfd-vIO7ThsmQSEmOLWBAUOxh3YB1aPc2-EGxuFioLRkkvTYo8t8HyQ4ciQk_LJY0kLfqg-K5l-fc0xN_g6at6J_ljnIo_gEMERCIIIUIMoggh1UEe9auKect6145LnhffYUiiaBu9gfR3V5FsG4ZheIbiFpsKRTwd3m3pR9pWVEWJxBBRUVZb_tt9qzelezh9vN_EMGnfdnwAs3ibuMPB1Gg2w4Ga79wxGhZFai75KLcbpGod7RACYcI7h4EfXqBLNE7WD1G0B7ECdAZx90D2pR8M0Rwi2H1uOo_e0-hwI_RZYXNXynsy7mwGD8vjfgujbxa2vk7h6ZlFWW0Gnxp1a3UvfJCf34r-eb3tm4oi_Ho4Lf0X_HjLf7pF1bfb_p_DRrTPUaj7vT3pBb168evHm9K73LRMlohXle0QP07EMGu_Ip2dNeyB3TgtGt0gv6o352eVDX_fLqfSP0_9za1oc2vNRd1sxYxzqUD03NCwqvClU04cxVykqiR55OR5zYd_dQyQVlM8BjVz5fAwhcQYD6Ai83nBtYPxJjM4vRaRqJFaTcGI3FQWuCRiD2PRDxpJCbmIzExHInd517T6YRxqKHMeRzO1UMlcTwOiTmViYFK01mcXYtKLUpbGKh0UFpglRLPKiVvRKUaypxVeuNXpak5lVMDlWazOL8WlVqUtjRQ6aC0wCpNPas0naTSzFylWfh4oaHNWakLtVIzx0rNzOmcGSg1n12LTi0Ky_U6ncUYlU2FMGrFhrJrUWzmWbHZG_FVDXnOal369VVNpP5I-b5tOB2f-4tfTrrDptU9PZKHtwe2pn-xdt1vc7z80K_r00BFuTg-JceL983TIy7K_tunytuDoKfa5VJfOBXomWm-_8L1_k8S5bQRF-DBzhsyFdDcDBD2BYgk3juELShMvgOF1fsvXO8_sR_YeUOmApqbAXJIYQ1jEu8dImNAyXNA6QBPMl6cKhdnQz2OF2fKxbl651y5mAyZlnwH6c39upf2HNV4nLuZ5f4B3EsDyL97aRjj371u_LrXRDzO3cxy_wDupQHk3700jPHvXgulDSzVHrK0ca9nxegs3nOw0f-f49qSjgaQe7OwRuDfHXSsCRBuPKebqYACxB3feccaQYCAc3UJB6sjDtZkHGwVcixsQso_oW1CDSiATagBBcgUOkTObcOaNQFsQorioW1CDSiATagBhfizmAaRc9uwZk0Am5Dy-dAmbjQ2IWUjRzYhRZxLbMKlwasBBbAJNaAQNqFBZGgT_hC5tw0ZgZS2L7EJlzxWAwpgE2pAIWxCg8jQJvwhcm8b8u8lUk4f_vKA1TZBpHh0oU2sHn_4PwAA__-RJ7dt

And this too

[RaduBerinde]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @knz)

---

pkg/sql/exec_util.go, line 125 at r2 (raw file):

Previously, knz (kena) wrote…

I think the settings machinery is not happy to see a value stored in kv that's not valid according to the map here. Not sure. In either case, maybe worth considering a migration to reset the value? Or you could remove it entirely, and add the name to retiredSettings in settings/registry.go.

In any case you should call SetDeprecated() on the setting if you keep it.

@jordanlewis what is your preference? I think I want to remove it altogether, but I can also add back the values and say in the text that it doesn't do anything anymore.

---

pkg/sql/logictest/testdata/logic_test/explain_analyze_plans, line 35 at r2 (raw file):

Previously, knz (kena) wrote…

Can you explain why this changed.

It's a SELECT with no ORDER BY so the optimizer doesn't guarantee any particular ordering.

---

pkg/sql/logictest/testdata/logic_test/explain_analyze_plans, line 69 at r2 (raw file):

Previously, knz (kena) wrote…

And this too

It's a better plan. It's faster to do a sort followed by streaming distinct than a non-streaming distinct followed by a sort.

[knz]

Reviewed 199 of 199 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)

---

pkg/sql/logictest/testdata/logic_test/explain_analyze_plans, line 35 at r2 (raw file):

Previously, RaduBerinde wrote…

It's a SELECT with no ORDER BY so the optimizer doesn't guarantee any particular ordering.

thanks

[RaduBerinde]

Updated to remove the cluster setting (checked offline with Jordan).

[jordanlewis]

Great stuff

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @RaduBerinde)

---

pkg/sql/exec_util.go, line 125 at r2 (raw file):

Previously, RaduBerinde wrote…

@jordanlewis what is your preference? I think I want to remove it altogether, but I can also add back the values and say in the text that it doesn't do anything anymore.

Yeah, I believe Raphael's right that if you upgrade into a new version with missing possible settings it will log messages about unknown cluster settings.

I'm okay with deleting this and adding to retiredSettings (which TIL exists)

---

pkg/sql/metric_test.go, line 72 at r4 (raw file):

		{query: "SET DISTSQL = 'off'", miscCount: 1, miscExecutedCount: 1, optCount: 1},
		{query: "BEGIN; END", txnBeginCount: 1, txnCommitCount: 1},
		{query: "SELECT 1", selectCount: 1, selectExecutedCount: 1, txnCommitCount: 1, optCount: 1},

Should we just delete optCount from these tests or is it still useful somehow?

[RaduBerinde]

---

pkg/sql/metric_test.go, line 72 at r4 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Should we just delete optCount from these tests or is it still useful somehow?

We will want to clean up the counter itself too, we'll clean up the test at the same time.

[RaduBerinde]

TFTRs!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)