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

pkg/sql/distsql_running.go, line 303 at r1 (raw file):

	}

	return func() {

The caller could in principle change planCtx after this function but this function binds it as a pointer. If we want to restrict that it should be documented. Otherwise we should pull the if outside of the func and copy a pointer to curPlan

pkg/sql/distsql_running.go, line 307 at r1 (raw file):

		// flow.Cleanup, which closes memory accounts that expect to be emptied.
		if planCtx.planner != nil && !planCtx.ignoreClose {
			planCtx.planner.curPlan.execErr = recv.resultWriter.Err()

Won't the main query and postquery all write execErr (in whatever order we happen to clean up)?

It kind of feels like this is happening in the wrong place, maybe what this if does should happen once, not once per query/postquery

pkg/sql/distsql_running.go, line 308 at r1 (raw file):

		if planCtx.planner != nil && !planCtx.ignoreClose {
			planCtx.planner.curPlan.execErr = recv.resultWriter.Err()
			planCtx.planner.curPlan.close(ctx)

Should planTop.close() close the postqueries as well?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @RaduBerinde)

pkg/sql/distsql_running.go, line 303 at r1 (raw file):

Hm, I didn't consider this, fixed, thanks!

pkg/sql/distsql_running.go, line 307 at r1 (raw file):

This if will only be true for the main query (because ignoreClose is set to true for sub- and postqueries).

(Also, I looked into where this execErr is used, and it is only in one place when retrieving a query statement by fingerprint, and it is only checked whether it is non-nil, so - at least at the moment - it wouldn't actually matter if we overrode the error. But I agree, that this could have been problematic some time in the future if we were to use execErr somewhere else, so thanks for pointing this out!)

pkg/sql/distsql_running.go, line 308 at r1 (raw file):

Yeah, I think so, thanks.

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

pkg/sql/distsql_running.go, line 307 at r1 (raw file):

Definitely feels like this would be more clean if we just moved it after the call to PlanAndRun (and got rid of ignoreClose). The curPlan is a higher-level object than what this function is logically dealing with.

In fact, I think a defer planCtx.planner.curPlan.close() belongs somewhere earlier in execWithDistSQLEngine (or even higher up in its caller).

Let's see what @jordanlewis thinks.

pkg/sql/distsql_running.go, line 311 at r2 (raw file):

			// before flow.Cleanup, which closes memory accounts that expect to be
			// emptied.
			plannerCopy.curPlan.execErr = recv.resultWriter.Err()

This does nothing now, it just modifies the copy.. I think we should do curPlan := &planCtx.planner.curPlan

I like this suggestion, thanks, done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @RaduBerinde)

pkg/sql/distsql_running.go, line 307 at r1 (raw file):

I agree with you here, and I actually tried doing this briefly during revision 2, but it was a little complicated, so I cooked up this quick patch without changes to the logic. I think it should be ok to merge this as is so that it unblocks work on FKs, and I (or maybe Matt as part of his work on other things) will address it shortly.

pkg/sql/distsql_running.go, line 311 at r2 (raw file):

Oops, thanks.

Is it stylistically ok to call this cleanup function right away, i.e. as do a single line dsp.Run(planCtx, noTxn, &p, recv, &evalCtxCopy, finishedSetupFn)() instead of a double liner if we don't want to delay the clean up?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @RaduBerinde)

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @mjibson, and @RaduBerinde)

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @mjibson, and @RaduBerinde)

pkg/sql/distsql_running.go, line 307 at r1 (raw file):

Can I kindly ask you to clean up what this thread is discussing? I'm confused about what the code around here, and I'm generally bothered by the cleanup return value from this function. I realize there were reasons why you've deferred things, but what greater pleasure is there in life than disentangling those reason?
Ideally a higher level would handle the closing of the plan nodes, and this method would handle the flow.Cleanup().
The time to separate memory accounts using planning and execution might be upon us.

As a nit, does the final returned cleanup in this method (the one that just does flow.Cleanup()) need to be deferred, or can it just be called here?

I got here because I'm adding yet more cleanup to a flow and it's fairly unclear if that needs to be deferred or not (although I have a pretty good guess). The fact that the cleanup return value is not usefully documented doesn't help, and I think it's not documented because it's hard to document because, in turn, it doesn't make a whole lotta sense.