opt: fix ordering-related optimizer panics

[DrewKimball]

It is possible for some functional-dependency information to be visible to a child operator but invisible to its parent. This could previously cause panics when a child provided an ordering that could be proven to satisfy the required ordering with the child FDs, but not with the parent's FDs.

This patch adds a step to the logic that builds provided orderings that ensures a provided ordering can be proven to respect the required ordering without needing additional FD information. This ensures that a parent never needs to know its child's FDs in order to prove that the provided ordering is correct. The extra step is a no-op in the common case when the provided ordering can already be proven to respect the required ordering.

Informs #85393
Informs #87806
Fixes #96288

Release note (bug fix): Fixed a rare internal error in the optimizer that has existed since before version 22.1, which could occur while enforcing orderings between SQL operators.

[cockroach-teamcity]

This change is 

[msirek]

Nice fix!

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @michae2)

---

pkg/sql/opt/ordering/ordering.go line 91 at r1 (raw file):

	}
	provided := funcMap[expr.Op()].buildProvidedOrdering(expr, required)
	provided = finalizeProvided(provided, required, expr.Relational().OutputCols)

BuildProvided can be called both during planning when building the memo and also at the end when finding the lowest cost tree in the memo. If we finalize provided during planning, it seems like it would restrict the plans that we can search as it's locking in some columns over others. Maybe a boolean parameter should be added to BuildProvided to control when to finalize, and only finalize when called from setLowestCostTree.

---

pkg/sql/opt/ordering/ordering.go line 445 at r1 (raw file):

	// First check if the given provided is already suitable.
	providedCols := provided.ColSet()
	if len(provided) == len(required.Columns) && providedCols.SubsetOf(outCols) {

Should this be changed to len(provided) >= len(required.Columns) ?
An ordering on more columns than required should be able to satisfy the required ordering as well.

[mgartner]

Very nice!

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

[DrewKimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @msirek)

---

pkg/sql/opt/ordering/ordering.go line 91 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

BuildProvided can be called both during planning when building the memo and also at the end when finding the lowest cost tree in the memo. If we finalize provided during planning, it seems like it would restrict the plans that we can search as it's locking in some columns over others. Maybe a boolean parameter should be added to BuildProvided to control when to finalize, and only finalize when called from setLowestCostTree.

Provided orderings necessarily lock in some columns, since they don't provided alternatives like required orderings do. Also, there's no reason AFAICT why the panic this is fixing couldn't occur during planning (rather than just the end), so I think it's necessary to do this unconditionally. FWIW in the vast majority of cases this shouldn't actually change the provided ordering - that's why not many tests changed.

---

pkg/sql/opt/ordering/ordering.go line 445 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

Should this be changed to len(provided) >= len(required.Columns) ?
An ordering on more columns than required should be able to satisfy the required ordering as well.

We already remove optional columns in remapProvided and trimProvided. Do you know of a case where we'd be able to take advantage of additional columns in provided beyond what was requested in required?

[msirek]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball and @michae2)

---

pkg/sql/opt/ordering/ordering.go line 91 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Provided orderings necessarily lock in some columns, since they don't provided alternatives like required orderings do. Also, there's no reason AFAICT why the panic this is fixing couldn't occur during planning (rather than just the end), so I think it's necessary to do this unconditionally. FWIW in the vast majority of cases this shouldn't actually change the provided ordering - that's why not many tests changed.

OK. I think alternatives might be handled by FDs.

---

pkg/sql/opt/ordering/ordering.go line 445 at r1 (raw file):

Do you know of a case where we'd be able to take advantage of additional columns in provided beyond what was requested in required?

No, looking at this closer, it looks like the other call, from populateBestProps, is not happening during optimization.

[DrewKimball]

TFYRs!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from b9b8da6 to blathers/backport-release-22.1-100776: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

error creating merge commit from b9b8da6 to blathers/backport-release-22.2-100776: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

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

[mgartner]

I added the backport-23.1.0 label because I believe we want to include this in the .0 release, correct @DrewKimball?

[DrewKimball]

I added the backport-23.1.0 label because I believe we want to include this in the .0 release, correct @DrewKimball?

Yeah, SGTM, thanks for doing that.

[DrewKimball]

blathers backport 23.1.0

[DrewKimball]

blathers backport 22.2

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from b9b8da6 to blathers/backport-release-22.2-100776: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2 failed. See errors above.

---

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