sql: cleanup after prepare caching

[jordanlewis]

Responding to some cleanup comments. Add comments and clarify code.

Updates #15639.

[cockroach-teamcity]

This change is 

[andreimatei]

thanks!

---

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/sql/executor.go, line 419 at r1 (raw file):

// contain partial type information for placeholders. Prepare will
// populate the missing types. The PreparedStatement is returned (or
// nil if there are no results). An error is returned if there is more than

I'd consider again making this take parser.Statement. Seems very weird to have a function take an array but say that it must contain one element.

---

pkg/sql/parser/placeholders.go, line 181 at r1 (raw file):

// replacePlaceholders replaces all placeholders in the input expression with
// their supplied values in the SemaContext's Placeholders map. If there is no
// available value for a placeholder, it is left alone. A nil ctx makes

but is this "leaving alone" what we want? You hinted in the other PR that this is called at prepare time. But... should it be called at prepare time?

---

pkg/sql/parser/type_check.go, line 738 at r1 (raw file):

	// Perform placeholder typing. This function is only called during Prepare,
	// when there are no available values for the placeholders yet, because
	// during Execute all placeholders are replaced from the AST before type

could we enforce this by having the SemaContext be aware of the phase?

---

pkg/sql/pgwire/v3.go, line 828 at r1 (raw file):

	results := c.executor.ExecutePreparedStatement(c.session, stmt, pinfo)
	return c.finishExecute(results, portalMeta.outFormats, false, int(limit))

mind commenting the false inline?

---

Comments from Reviewable

[jordanlewis]

TFTR!

---

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/sql/executor.go, line 419 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'd consider again making this take parser.Statement. Seems very weird to have a function take an array but say that it must contain one element.

It must contain either 0 or 1 elements, not exactly 1. I agree that it's weird!

---

pkg/sql/parser/placeholders.go, line 181 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

but is this "leaving alone" what we want? You hinted in the other PR that this is called at prepare time. But... should it be called at prepare time?

Yeah, this should not get called at prepare time at all. Good idea to augment SemaContext to contain the phase, but I'm not going to do this right now.

---

pkg/sql/parser/type_check.go, line 738 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

could we enforce this by having the SemaContext be aware of the phase?

Good idea

---

pkg/sql/pgwire/v3.go, line 828 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

mind commenting the false inline?

I really don't like this style! My editor annotates bare constants like this with the field name automatically, so I prefer to leave them alone.

---

Comments from Reviewable