sql: make tail-call optimization work with nested routines

[DrewKimball]

sql: defer tail-call identification until execbuilding

This commit changes the way routine tail-calls are handled. Before, only
PL/pgSQL sub-routines were considered as tail-calls, and this was determined
by a TailCall property that was set during optbuilding. This approach was
fragile, did not work for explicit tail-calls, and did not work well with
nested routine calls in general.

Now, tail-calls are determined after optimization, during execbuilding.
This will allow explicit (user-specified) tail calls to be optimized. It
also prevents inlining rules from causing correctness bugs, since the old
TailCall property only applied to the original calling routine.

See ExtractTailCalls for further details. The next commit will add
additional testing.

Informs #120916

Release note: None

sql: add tests for tail-call property

This commit adds tests for the ExtractTailCalls function from the
previous commit, and adds a tail-call field to UDFCall expressions
that are in tail-call position in another routine. It also adds a
regression test for #120916.

Informs #120916

Release note: None

sql: check exception handler before applying TCO

This commit finishes the tail-call optimization fix begun by the previous
commits, by preventing TCO when it would lose the reference to the calling
routine's exception handler. PL/pgSQL sub-routines always maintain a
reference to their parent's exception handler, so this isn't a problem for
them. However, explicit (user-specified) nested routines do not track the
calling routine's exception handler.

There is no release note because this bug hasn't appeared in any release.

Fixes #120916

Release note: None

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

[cockroach-teamcity]

This change is 

[yuzefovich]

Nice - very thorough testing!

Reviewed 8 of 8 files at r1, 19 of 19 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)

---

pkg/sql/routine.go line 504 at r3 (raw file):

		return childBlock == g.expr.BlockState || childBlock.Parent == g.expr.BlockState
	}
	return false

I'm confused here. Based on the comment within the if block, this return must be for the case when we don't have an exception handler, so I'd guess we would be returning true here. Why is this false?

---

pkg/sql/opt/memo/extract.go line 454 at r1 (raw file):

}

// ExtractTailCalls traverses of the given expression tree, searching for

nit: probably s/traverses of/traverses/.

---

pkg/sql/opt/memo/extract.go line 484 at r1 (raw file):

		// routine cannot directly be used as the result of the calling routine.
		//
		// * No routine in the input of the project can be a tail-call, since the

This condition doesn't seem to be included in the code - am I missing something?

---

pkg/sql/opt/memo/testdata/logprops/tail-calls line 0 at r1 (raw file):
nit: logprops/tail-calls is an empty file in the first commit.

[DrewKimball]

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

---

pkg/sql/routine.go line 504 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I'm confused here. Based on the comment within the if block, this return must be for the case when we don't have an exception handler, so I'd guess we would be returning true here. Why is this false?

Good catch, that should be true. I plan to run some simple perf tests to make sure we're properly optimizing PL/pgSQL loops, which would catch this.

---

pkg/sql/opt/memo/extract.go line 454 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: probably s/traverses of/traverses/.

Done.

---

pkg/sql/opt/memo/extract.go line 484 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This condition doesn't seem to be included in the code - am I missing something?

We enforce this condition by simply not recursively calling ExtractTailCalls on the Project operator's input. I augmented the comment with this detail.

---

pkg/sql/opt/memo/testdata/logprops/tail-calls line at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: logprops/tail-calls is an empty file in the first commit.

Done.

[yuzefovich]

The CI is still red, but the changes

Reviewed 23 of 23 files at r4, 20 of 20 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[DrewKimball]

TFYR!

bors r+

[craig]

Build failed (retrying...):

• check_generated_code

[yuzefovich]

Looks like this needs a rebase and ./dev gen testlogic.

bors r-

[craig]

Canceled.

[DrewKimball]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)
• check_generated_code
• docker_image_amd64
• examples_orms
• linux_amd64_build
• linux_amd64_fips_build
• linux_arm64_build
• macos_amd64_build
• macos_arm64_build
• windows_build