sql: fix panic with UNION ALL

[jordanlewis]

Previously, UNION ALL distsql plans could panic the server if the two
sub plans had different post processing. Now, this condition is
detected, and a no-op stage is inserted after the plans if necessary.

Fixes #25611.

Release note (bug fix): fix a server crash when trying to plan
certain UNION ALL queries.

[cockroach-teamcity]

This change is 

[asubiotto]

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

---

pkg/sql/distsqlplan/physical_plan.go, line 263 at r1 (raw file):

// CheckConsistentPost returns a non-nil error if not all of the processors of
// the last stage of the PhysicalPlan have identical post-processing.
func (p *PhysicalPlan) CheckConsistentPost() error {

I find this comment a bit confusing. You might consider rephrasing to something like:

CheckConsistentPost checks that the processors of the last stage of the PhysicalPlan have identical post-processing, returning an error if not.

Also, since this checks the latest stage of a physical plan, it might make sense to call this CheckLastStagePost.

---

Comments from Reviewable

[RaduBerinde]

Maybe we should add a call to GetLastStagePost in createPlanForNode (after the switch), perhaps behind the TestRace flag? I would at least try to add it temporarily and run all the logictests to see if it discovers other cases.

---

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/sql/logictest/testdata/logic_test/union, line 260 at r1 (raw file):

query I
SELECT a FROM a WHERE a > 2 UNION ALL (select a from c WHERE b > 2) limit 1;

[nit] SELECT FROM LIMIT

---

Comments from Reviewable

[jordanlewis]

I did it temporarily, saw tests pass, and then added it to the guard behind the metadata check planning where it can live on forever.

---

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/sql/distsqlplan/physical_plan.go, line 263 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I find this comment a bit confusing. You might consider rephrasing to something like:

CheckConsistentPost checks that the processors of the last stage of the PhysicalPlan have identical post-processing, returning an error if not.

Also, since this checks the latest stage of a physical plan, it might make sense to call this CheckLastStagePost.

Done.

---

pkg/sql/logictest/testdata/logic_test/union, line 260 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] SELECT FROM LIMIT

Done.

---

Comments from Reviewable

[jordanlewis]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)