sql: avoid splitting the planner when expanding sub-queries

[knz]

Prior to this patch, the code that expands sub-queries would duplicate
the planner object to create the sub-query planNode's
recursively. This was perceived to be needed, as indicated by a
previous comment that had been erased in
4acdc8a, because there may be nested
sub-queries that would cause the subqueryVisitor embedded in planner
to be reset during the recursion.

The problem with this approach is that whichever leases were acquired
by the sub-queries, and embedded in the planner copy, are then lost
after the sub-query is replaced and cannot be released at the end of
the session.

This patch fixes this problem by ensuring the same planner object is
reused throughout the recursion, and preserves only the state of the
embedded subqueryVisitor across the recursive call.

A proper long-term protection against this type of issue would be to
avoid embedding visitors in the planner object, and instead managing a
cache of visitor objects if profiling shows that visitor allocation is
hurting performance.

Fixes #11701.

---

This change is 

[knz]

@vivekmenezes could you guide me about how to best test this. You explain in the linked issue we can check that all leases are released. How do you propose to do this?

My proposal would be to add a new directive in our testdata input parser (in TestLogic) to forcefully/manually purge leases currently held in the planner, then use a SELECT query in the lease table just after that to see whether there are remaining entries in the lease table. What do you think about this?

[vivekmenezes]

Look at the code in lease.go that references removeOnceDereferenced . From the logic tests we should always set this to true, that way the leases are released as soon as they are dereferenced. And then at the end of each test file query the lease table and ensure there are no records.

[vivekmenezes]

thanks for fixing it!

[tamird]

Should there be a NoCopy in planner?

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[knz]

@tamird yes, that's a good idea too!
(Doing so didn't uncover other unwanted copies. Yay.)

[vivekmenezes]

golang/go#8005 (comment)

is what tamir is referring to

[tamird]

not quite https://github.com/cockroachdb/cockroach/blob/master/pkg/util/nocopy.go

[knz]

I've used that before, thanks

[knz]

Ok I have added the NoCopy members and a specific test. PTAL.

[tamird]

---

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

---

pkg/sql/lease_test.go, line 646 at r2 (raw file):

			LeaseStoreTestingKnobs: csql.LeaseStoreTestingKnobs{
				RemoveOnceDereferenced: true,
				LeaseReleasedEvent: func(lease *csql.LeaseState, err error) {

s/err/_/

---

pkg/sql/lease_test.go, line 677 at r2 (raw file):

		t.Fatal("lease from sub-query was not released")
	case <-fooRelease:
		return

remove

---

Comments from Reviewable

[vivekmenezes]

close fooRelease

[vivekmenezes]

is this really needed?

[knz]

TFYRs!

---

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

---

pkg/sql/lease_test.go, line 646 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/err/_/

Done.

---

pkg/sql/lease_test.go, line 648 at r2 (raw file):

Previously, vivekmenezes wrote…

close fooRelease

Done.

---

pkg/sql/lease_test.go, line 668 at r2 (raw file):

Previously, vivekmenezes wrote…

is this really needed?

the leases are not released until the session is closed. Closing the connection releases the session.

---

pkg/sql/lease_test.go, line 677 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

remove

Done.

---

Comments from Reviewable

[tamird]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[vivekmenezes]

---

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/lease_test.go, line 668 at r2 (raw file):

Previously, knz (kena) wrote…

the leases are not released until the session is closed. Closing the connection releases the session.

https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/executor.go#L673

is where it gets released at the end of a transaction. Are you sure this is needed?

---

Comments from Reviewable

[RaduBerinde]

LGTM

[knz]

TFYRs

---

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

---

pkg/sql/lease_test.go, line 668 at r2 (raw file):

Previously, vivekmenezes wrote…

https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/executor.go#L673

is where it gets released at the end of a transaction. Are you sure this is needed?

No you are right the close was unnecessary! Thanks for hinting.

---

Comments from Reviewable