Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @otan)

pkg/sql/conn_executor.go, line 1480 at r1 (raw file):

}

// sessionData returns the top SessionData in the executor's sessionDataStack.

it's worth mentioning in this comment that the purpose of doing this is so that each transacation can track a separate sessiondata, and so we can get txn-scoped session variable changes.

pkg/sql/exec_util.go, line 2428 at r2 (raw file):

// a transaction where there may be two or more SessionData elements in
// the stack)
type sessionDataMutatorTopOnlyBase struct {

heh i can see why this is the name, but at the same time i feel like there could be a better name

something like sessionDataMutatorTopCallbacks or even just sessionDataMutatorCallbacks?

pkg/sql/exec_util.go, line 2453 at r2 (raw file):

		sessionDataMutatorBase: f.sessionDataMutatorBase,
	}
	// Change the sessionDataMutatorTopOnlyBase if our SessionData element is not

hmm could you explain a bit more about why we need this? i get that we're going to be creating a mutator for each element in the stack, but then why change this? is this the step that prevents us from sending out multiple param status updates?

also, what is sessionDataMutatorTopOnlyBase going to be if we don't change it? it looks like it would just be nil?

(i'm just asking since i couldn't piece it together on my own. but maybe this info would be useful to put in this comment.)

pkg/sql/exec_util.go, line 2455 at r2 (raw file):

	// Change the sessionDataMutatorTopOnlyBase if our SessionData element is not
	// the top.
	if sd != f.sds.Top() {

comparing the pointers seems a bit fragile to me.. but maybe my instincts are wrong on this. what would you feel about making each sessiondata keep track of its nesting level, and comparing those instead?

pkg/sql/exec_util.go, line 2486 at r2 (raw file):

) error {
	for _, sd := range f.sds.Elems() {
		if err := applyFunc(f.mutator(sd)); err != nil {

could we end up in a bad state if we error out of this loop early? it would mean that some of the sessiondatas had the function applied and some did not. e.g. in the case of RESET ALL, some levels of the txn nesting would get reset while others would not

the ideal would be that if we encounter an error, we undo all the changes that were done so far. idk if it's feasible to do so -- maybe we could copy each sessiondata before calling applyFunc on it, and revert the stack elements to those copies?

pkg/sql/exec_util.go, line 2493 at r2 (raw file):

}

// sessionDataMutator is the object used by sessionVars to change the session

should sessionVars still be using this? why don't they need to use the sessionDataMutatorIterator?

actually, why do we need this struct at all? can't we get rid of this sessionDataMutator and rename sessionDataMutatorIterator to sessionDataMutator?

pkg/sql/sessiondata/session_data.go, line 206 at r1 (raw file):

func (s *Stack) Top() *SessionData {
	if len(s.stack) == 0 {
		return nil

another followup from the last PR:

should this function return an AssertionError here if the stack is empty?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @rafiss)

pkg/sql/conn_executor.go, line 1480 at r1 (raw file):

Yep.

pkg/sql/exec_util.go, line 2428 at r2 (raw file):

Done.

pkg/sql/exec_util.go, line 2453 at r2 (raw file):

it prevents us from sending multiple paramstatusupdates for a SET x inside a transaction. otherwise, it would create paramstatusupdate for each level of nesting in the txn!

pkg/sql/exec_util.go, line 2455 at r2 (raw file):

done!

pkg/sql/exec_util.go, line 2486 at r2 (raw file):

could we end up in a bad state if we error out of this loop early?

potentially, but I'm not sure it's worth doing much more than that until we do #69391. in theory, if something errors here, we're in bigger trouble.

seems like the savepointStack has a similar issue if we're paranoid, but I'm ok with things as is.

pkg/sql/exec_util.go, line 2493 at r2 (raw file):

because the set_vars.go will be responsible for delegating to either the top mutator, or all mutators.
the iterator makes it easy to transform mutator objects based on whether or not it's the top. without the separation also, we'd have to take in a islocal argument to all the varGen variables which is hairier imo.

pkg/sql/sessiondata/session_data.go, line 206 at r1 (raw file):

Not everything errors here; we can panic if you want. I'm ok with the other assertions we already made in the pop functions.

looks good! just some optional comments if you'd like

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @otan)

pkg/sql/exec_util.go, line 2453 at r2 (raw file):

it's more clear to me now thanks!

pkg/sql/exec_util.go, line 2486 at r2 (raw file):

excellent!

pkg/sql/exec_util.go, line 2493 at r2 (raw file):

ohh i see, i missed the change where applyFunc was added to func (n *setVarNode) startExec(). yes that definitely seems nicer than having to pass in isLocal to everything

pkg/sql/exec_util.go, line 2403 at r6 (raw file):

	defaults SessionDefaults
	settings *cluster.Settings
	sessionDataMutatorCallbacks

if we embedded sessionDataMutatorCallbacks inside of sessionDataMutatorIterator and sessionDataMutator instead of embedding it here, would it make things cleaner? then we could avoid the logic of having to unset it inside of mutator() right?

pkg/sql/exec_util.go, line 2475 at r6 (raw file):

	elems := f.sds.Elems()
	for i, sd := range elems {
		applyFunc(f.mutator(i == len(elems)-1, sd))

we just as well could change this to i == 0 -- that way the param status notifications are sent as soon as the value is updated in the top-level, outside of any txn. and if any of the per-txn data mutators fail, the param status will still have been sent out. (ditto for forEachMutatorError)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @otan, and @rafiss)

pkg/sql/exec_util.go, line 2493 at r2 (raw file):

Done.

pkg/sql/exec_util.go, line 2403 at r6 (raw file):

good idea, done!

pkg/sql/exec_util.go, line 2475 at r6 (raw file):

Done.