persistent session interface #595

max-hoffman · 2021-10-19T23:19:14Z

No description provided.

…nto max/sql-config

server/server.go

sql/config/prefix_config.go

max-hoffman · 2021-10-21T19:43:48Z

sql/system_variables.go

@@ -159,11 +173,48 @@ func (sv *globalSystemVariables) SetGlobal(name string, val interface{}) error {
 	return nil
 }

-// init initializes SystemVariables as it functions as a global variable.
-func init() {
+func DecodeSystemVar(name string, val string) (interface{}, error) {


it's just annoying to call this repetitively, maybe GetAndDecodeSystemVariable because it validates and decodes?

Shouldn't have this at all, make the integrator responsible for all encoding / decoding

max-hoffman · 2021-10-21T19:52:21Z

server/server.go

+	NoDefaults bool
+}
+
+func (c Config) WithDefaults(conf config.ReadableConfig) (Config, error) {


this is awkward, but i don't have access to the decoding in Dolt. We don't streamline config merging right now in general. Need key constants for the system variables, well documented process for cli overrides -> server config -> persisted defaults -> static defaults. And same thing for everywhere else we would access Dolt specifc config.

zachmu

In general this PR tries to do too much. You have two different interfaces with overlapping functionality: persisted session and config. The new session interface type is clearly necessary and proper, but I'm not convinced that ReadableConfig / WritableConfig needs to be defined in the engine at all, as opposed to details that integrators need to care about. And you have several implementations of these interfaces that aren't currently used and whose purpose I don't understand.

Start smaller. You need a persisted session interface and a test implementation that you can use for unit tests, which you have. You need a way for integrators to add system variables, which already exists (sql.SystemVariables.AddSystemVariables). It makes sense to have a method similar to InitSystemVariablesWithDefaults, but you don't want a ReadableConfig there, you want a collection (or maybe just a slice) of SystemVariables.

I would consider pruning the Config interfaces altogether. If you don't (because you want to support file-based config for people running the in memory server), then move the PersistedSession type into the memory package instead, and consider moving the file-based config implementation there as well.

zachmu · 2021-10-21T22:42:18Z

server/context.go

@@ -53,6 +53,7 @@ type SessionManager struct {
 	builder     SessionBuilder
 	sessions    map[uint32]sql.Session
 	pid         uint64
+	noDefaults  bool


As a general rule, don't name boolean variables with a negative, makes it harder to reason about their meaning

so useDefaults here, flip the case

not resolved :)

Not actually used, it seems, just remove

server/server.go

zachmu · 2021-10-21T22:52:20Z

sql/config/config_hierarchy.go

+	nameToConfig map[string]ReadWriteConfig
+}
+
+var _ ReadableConfig = &ConfigHierarchy{}


Nil pointer is more idiomatic (just because it saves an allocation)

zachmu · 2021-10-21T23:44:31Z

sql/config/config.go

+var ErrConfigParamNotFound = errors.New("Param not found")
+
+// ReadableConfig interface provides a mechanisms for getting key value pairs from a config
+type ReadableConfig interface {


Main comment on these interfaces is that they should not work in strings like this. They should be interface{} valued just like like the session variable interfaces on session:

SetSessionVariable(ctx *Context, sysVarName string, value interface{}) error GetSessionVariable(ctx *Context, sysVarName string) (interface{}, error)

Implementer should be responsible for storing an appropriate type and returning it, then the engine can coerce it into a more appropriate SQL type as necessary

zachmu · 2021-10-22T00:04:27Z

sql/config/config.go

+
+	// GetStringOrDefault retrieves a string from the config hierarchy and returns it if available. Otherwise it returns
+	// the default string value
+	GetStringOrDefault(key, defStr string) string


Would remove this, add an ok return param to the Get method instead

zachmu · 2021-10-22T00:21:43Z

sql/persisted_session.go

+	ResetPersistAll(ctx *Context) error
+}
+
+type PersistedSession struct {


Larger comment is: why is this a struct? Integrators should implement this.

If we provide one as part of GMS, it should be a file-based one, using the file-based config store you wrote

If you want an in-memory one for testing the set plan node, that seems fair, put one in the plan_test package

zachmu · 2021-10-22T00:22:40Z

sql/plan/set.go

+		persistSess, ok := ctx.Session.(sql.PersistableSession)
+		if !ok {
+			// todo find old error
+			return sql.ErrUnsupportedFeature.New("PERSIST")


Probably not the right error here. It's not that the feature is unsupported, it's that the session doesn't support persistence

zachmu · 2021-10-22T00:24:21Z

sql/system_variables.go

@@ -159,11 +173,48 @@ func (sv *globalSystemVariables) SetGlobal(name string, val interface{}) error {
 	return nil
 }

-// init initializes SystemVariables as it functions as a global variable.
-func init() {
+func DecodeSystemVar(name string, val string) (interface{}, error) {


Shouldn't have this at all, make the integrator responsible for all encoding / decoding

zachmu · 2021-10-22T00:27:09Z

sql/system_variables.go

+	return decoded, nil
+}
+
+func InitSystemVariablesWithDefaults(defaults config.ReadableConfig) error {


This is probably not functionality that the engine should be providing. If you want to provide this as a utility method that takes a ReadableConfig and puts all of its values into a session, that's fine.

zachmu · 2021-10-22T00:27:49Z

sql/system_variables.go

+	return decoded, nil
+}
+
+func InitSystemVariablesWithDefaults(defaults config.ReadableConfig) error {
 	for _, sysVar := range systemVars {
 		SystemVariables.sysVarVals[sysVar.Name] = sysVar.Default


This seems misplaced in the context of this function. Setting up the system vars and their defaults is not the same as initializing a session with a config store.

max-hoffman · 2021-10-22T01:21:27Z

@zachmu Responding to a couple of the points just for documentation, I think we're mostly in alignment but the cross-package code muddies the waters. Will follow up in person tomorrow:

The config interfaces are wholesale transplanted from Dolt, the only one I modified is PrefixConfig to make my life easier for namespacing. I don't disagree with your individual critiques of the configs, but I do disagree that the config portion is overcomplicating; refactoring all of the config code is considerably harder than moving it over. If I implemented your suggestions I'd have to refactor all of how Dolt uses those interfaces.
The alternative is accomplishing this without moving config from Dolt to GMS. In general, sysVar access is mostly private to GMS. If we persist as part of a sysVar set, GMS needs a handle to an interface for writing those changes. I think this is my most lingering question after reading your comments: an alternative plan of: intercepting plan.Set in Dolt, persisting vars there, and having GMS be blind to Persist queries is doable, I think. But I immediately veered away from that given the structuring of current Set logic, and how this is a MySQL feature that I think makes sense as part of our MySQL server logic.
Also, having the integrator do encoding/decoding is appealing, but all of the typing logic lives in GMS already. I originally wrote duplicate encoders for the listener configs, but then found the encoder/decoder typing logic and refactored.

…nto max/sql-config

zachmu

LGTM, just a couple comments

memory/persisted_session.go

zachmu · 2021-10-28T19:31:11Z

server/context.go

@@ -53,6 +53,7 @@ type SessionManager struct {
 	builder     SessionBuilder
 	sessions    map[uint32]sql.Session
 	pid         uint64
+	noDefaults  bool


not resolved :)

server/server_config.go

zachmu · 2021-10-28T19:33:59Z

sql/parse/parse.go

+			varToSet := expression.NewSystemVar(setExpr.Name.String(), sql.SystemVariableScope_PersistOnly)
+			res[i] = expression.NewSetField(varToSet, innerExpr)
+			// TODO reset persist
+		//case sqlparser.SetScope_ResetPersist:


You have this handled in plan/set.go

zachmu · 2021-10-28T19:35:41Z

enginetest/enginetests.go

+	e := NewEngine(t, harness)
+	ctx := NewContext(harness)
+
+	for _, tt := range q {


Need tests of reset here as well

no parser support for RESET yet

zachmu · 2021-10-28T19:36:05Z

server/context.go

@@ -53,6 +53,7 @@ type SessionManager struct {
 	builder     SessionBuilder
 	sessions    map[uint32]sql.Session
 	pid         uint64
+	noDefaults  bool


Not actually used, it seems, just remove

sql/session.go

max-hoffman and others added 20 commits October 18, 2021 08:40

initial config move

5c030e5

merge main

a7cd857

fix system var / session var disconnect

542b7d7

put daylon's naming back

e4618a3

save progress, todo refactor PersistedSession

3f4d2cb

PersistedSession

33f7532

change file name

195c6b3

[ga-format-pr] Run ./format_repo.sh to fix formatting

c26492d

fmt

ef200a5

Merge branch 'max/sql-config' of github.com:dolthub/go-mysql-server i…

bd07b04

…nto max/sql-config

persist tests

55e85ba

prefixConf tests

9cb4a66

[ga-format-pr] Run ./format_repo.sh to fix formatting

af72cd3

fmt

4f3be69

Merge branch 'main' into max/sql-config

7310261

interface{} to int

1bff3eb

Merge branch 'max/sql-config' of github.com:dolthub/go-mysql-server i…

9096470

…nto max/sql-config

add system vars reset function for testing

8d42b9b

add persisted defaults loading tests

87943a6

[ga-format-pr] Run ./format_repo.sh to fix formatting

650be2a

max-hoffman commented Oct 21, 2021

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

max-hoffman commented Oct 21, 2021

View reviewed changes

sql/config/prefix_config.go Outdated Show resolved Hide resolved

max-hoffman commented Oct 21, 2021

View reviewed changes

Add NoDefaults tests

335c383

zachmu reviewed Oct 22, 2021

View reviewed changes

max-hoffman and others added 3 commits October 22, 2021 15:59

zach's comments

672d2d0

merge upstream

f6a3319

[ga-format-pr] Run ./format_repo.sh to fix formatting

cd28cc7

max-hoffman added 7 commits October 22, 2021 16:16

small fixes

44325f6

Merge branch 'max/sql-config' of github.com:dolthub/go-mysql-server i…

f20483a

…nto max/sql-config

engine tests, name tweaks

85c2e0a

fix server config tests

db4620e

missed default in switch

7e90010

reorder tesets

f1c9b6e

Fix formatting

dffc288

max-hoffman marked this pull request as ready for review October 25, 2021 19:31

max-hoffman changed the title ~~Max/sql config~~ sql config persistence in dolt config.json Oct 26, 2021

max-hoffman changed the title ~~sql config persistence in dolt config.json~~ persistent session interface Oct 26, 2021

max-hoffman added 5 commits October 26, 2021 14:45

Interface tweaks

d191098

fix test

7da66bf

Bump read timeout, was breaking dolt tests

41f9ba1

Merge branch 'max/sql-config' of github.com:dolthub/go-mysql-server i…

15c0533

…nto max/sql-config

Separate server from conf

0aeeabd

max-hoffman assigned zachmu Oct 28, 2021

zachmu approved these changes Oct 28, 2021

View reviewed changes

max-hoffman added 2 commits October 29, 2021 09:14

Zach's comments

49ad446

small fixes

43a1226

max-hoffman merged commit 62885d6 into main Oct 29, 2021

max-hoffman deleted the max/sql-config branch October 29, 2021 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

persistent session interface #595

persistent session interface #595

max-hoffman commented Oct 19, 2021

max-hoffman Oct 21, 2021 •

edited

Loading

zachmu Oct 22, 2021

max-hoffman Oct 21, 2021

zachmu left a comment

zachmu Oct 21, 2021

zachmu Oct 28, 2021

zachmu Oct 28, 2021

zachmu Oct 21, 2021

zachmu Oct 21, 2021

zachmu Oct 21, 2021

zachmu Oct 22, 2021

zachmu Oct 22, 2021

zachmu Oct 22, 2021

zachmu Oct 22, 2021

zachmu Oct 22, 2021

zachmu Oct 22, 2021

zachmu Oct 22, 2021

max-hoffman commented Oct 22, 2021

zachmu left a comment

zachmu Oct 28, 2021

zachmu Oct 28, 2021

zachmu Oct 28, 2021

max-hoffman Oct 28, 2021

zachmu Oct 28, 2021

persistent session interface #595

persistent session interface #595

Conversation

max-hoffman commented Oct 19, 2021

max-hoffman Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-hoffman commented Oct 22, 2021

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-hoffman Oct 21, 2021 •

edited

Loading