this is looking good! it's actually less messy than i thought it would be..

what's your concern with search_path? is it special in some way?

(btw, we should make sure this also works with SET LOCAL ROLE -- looks like it should, but thats another thing to test)

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

pkg/sql/conn_executor_exec.go, line 820 at r5 (raw file):

	ex.phaseTimes.SetSessionPhaseTime(sessionphase.SessionEndTransactionCommit, timeutil.Now())
	if !ex.implicitTxn() {
		ex.sessionDataStack = ex.sessionDataStack.Pop()

so i guess for the param status stuff, we'd need a check (maybe inside of Pop that will send out param status messages for all the variables that need it.

pkg/sql/conn_executor_exec.go, line 1248 at r5 (raw file):

) (_ fsm.Event, payload fsm.EventPayload) {
	switch s := ast.(type) {
	case *tree.BeginTransaction:

i think we also need something similar when starting a new SAVEPOINT. (that's the only way to get a nested txn. if you do BEGIN inside an explicit txn, it's an error.)

pkg/sql/exec_util.go, line 2395 at r4 (raw file):

func (noopParamStatusUpdater) BufferParamStatusUpdate(string, string) {}

// sessionDataMutatorFactory creates sessionMutators.

the comment could include why it's needed and also how it interacts with the session data stack

pkg/sql/planner.go, line 347 at r4 (raw file):

	p.extendedEvalCtx = internalExtendedEvalCtx(
		ctx, sd, &sessionDataMutator{

nit: seemed a bit easier to read when it wasn't inlined

pkg/sql/set_var.go, line 145 at r5 (raw file):

	}
	if n.local {
		if params.p.EvalContext().TxnImplicit {

let's match the Postgres warning: "SET LOCAL can only be used in transaction blocks"

weirdly, postgres shows that warning, but then still lets the SET LOCAL go ahead as if it were a normal SET.

pkg/sql/parser/sql.y, line 4432 at r5 (raw file):

| set_exprs_internal   { /* SKIP DOC */ }
| SET CONSTRAINTS error { return unimplemented(sqllex, "set constraints") }
| SET LOCAL set_rest

hm i think this works better with a new rule so that we can have the right help text

// %Help: SET LOCAL - change a session variable scoped to the current transaction
// %Category: Cfg
// %Text:
// SET LOCAL <var> { TO | = } <values...>
// SET LOCAL TIME ZONE <tz>
//
// %SeeAlso: SHOW SESSION, RESET, DISCARD, SHOW, SET CLUSTER SETTING, SET TRANSACTION, SET SESSION,
// WEBDOCS/set-vars.html
set_local_stmt:
  SET LOCAL set_rest
  {
    // ...
  }

also, what happens if we put this under preparable_set_stmt?

pkg/sql/sem/tree/eval.go, line 3458 at r3 (raw file):

type EvalContext struct {
	// Session variables. This is a read-only copy of the values owned by the
	// Session. A new element is added to the stack for each explicit transaction.

what is "for each explicit transaction" referring to? i believe it's when a SAVEPOINT is created right?

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

}

// SessionDataStack represents a stack of SessionData objects.

it would be helpful for the comment to say why we need it -- to support SET LOCAL and txn-scoped session vars

pkg/sql/sessiondata/session_data.go, line 195 at r2 (raw file):

// NewSessionDataStack creates a new SessionDataStack.
func NewSessionDataStack(sds ...*git difSessionData) SessionDataStack {

typo on git diff

Drive-by commentary because I was curious.

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

pkg/sql/exec_util.go, line 2488 at r10 (raw file):

}

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

nit: this isn't an interface

pkg/sql/sessiondata/session_data.go, line 196 at r10 (raw file):

	// Use an internal variable to prevent abstraction leakage.
	stack []*SessionData

Why not a linked list? How do you feel about:

type Stack struct {
    cur   SessionData // could see making this a pointer
    next *Stack
}

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

pkg/sql/exec_util.go, line 2395 at r4 (raw file):

Done.

pkg/sql/sessiondata/session_data.go, line 195 at r2 (raw file):

Done.

pkg/sql/sessiondata/session_data.go, line 196 at r10 (raw file):

was thinking it'd re-use the same slice space and because there are certain PopN/fetching all items in the stack as a slice operations i'd like slicing for.

release note nit: the release note should describe how it works with savepoints (SET LOCAL isn't rolled back after releasing a savepoint)

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

pkg/sql/conn_executor_savepoints.go, line 140 at r20 (raw file):

	// We will restore the currSessionData on it, as the SET LOCAL parameters
	// are still kept.
	// We do not have to report param status updates as the SessionData

wait why don't we report them? based on the tests below it looked like the timezone was changing even after doing a RELEASE

pkg/sql/set_var.go, line 32 at r17 (raw file):

)

// setVarNode represents a SET SESSION statement.

nit: represents a SET { LOCAL | SESSION } statement

pkg/sql/set_var.go, line 139 at r19 (raw file):

	}

	if n.v.RuntimeSet != nil {

could you add a comment saying why RuntimeSet and SetWithPlanner don't work with applyOnSessionDataMutators?

pkg/sql/logictest/testdata/logic_test/set_local, line 38 at r17 (raw file):

2020-08-25 08:16:17.123456 -0700 PDT  P1DT15H16M17.123456S

# Rollback and ensure the values are correct.

i think this comment also deserves a link to #69396 mentioning that CRDB diverges from postgres

pkg/sql/parser/testdata/set, line 520 at r16 (raw file):

parse
SET LOCAL TIME ZONE +3

can we test SET LOCAL ROLE bob as well?

pkg/sql/sessiondata/session_data.go, line 56 at r17 (raw file):

func (s *SessionData) Clone() *SessionData {
	// Currently clone does a shallow copy of everything - we can get away with it
	// as all the slices/maps does a copy if it mutates OR are idempotent

nit: i don't think idempotent is the right word. maybe it could just say "operations that affect the whole SessionDataStack"

pkg/sql/sessiondata/session_data.go, line 58 at r17 (raw file):

	// as all the slices/maps does a copy if it mutates OR are idempotent
	// operations (e.g. setting SequenceState should be the setting the
	// same value across all copied sessions).

nit: "across all copied SessionData"

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

pkg/sql/conn_executor_savepoints.go, line 140 at r20 (raw file):

whereabouts? it's because RELEASE doesn't actually commit or rollback a transaction; they just scrub previous savepoints.

pkg/sql/set_var.go, line 139 at r19 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/set_local, line 38 at r17 (raw file):

Done.

pkg/sql/parser/sql.y, line 4432 at r5 (raw file):

Done.

pkg/sql/parser/testdata/set, line 520 at r16 (raw file):

Done.

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

Done.

pkg/sql/sem/tree/eval.go, line 3458 at r3 (raw file):

Done.