Single binary server (SQLite) and concurrency bugfixes for in memory storages #2114

wolfy-j · 2021-10-29T13:57:04Z

Fixed the issues causing SQLite to deadlock when multiple writers perform an operation
Updated SQL tools and server build to include SQLite database
Added connection option to automatically setup in-memory/file database schema
Removed the dependency on SQL shared cache in order to make the system work (you can use private caches now)
Added the reference counter for GC (not used at the moment due to non-overlapping DB connections)

In short, you can use temporal now as a single binary file with in-memory or file SQLite database.

These changes properly enabled the support for SQLite in the temporal server. The initial implementation did not address the fact that internal Temporal services are highly isolated, each will create at least a single connection to the database violating the SQLite concept of safety only within a single thread. While the base configuration works on any minor load it might and it will lock the database due to concurrent writers from different parts of the system.

The clean solution will be to refactor the Temporal container in order to provide a single database instance to all the services and limit concurrency on the SQL driver level. However, such change will cause a significant impact and will require very careful testing of the server. Instead, the change has been addressed on the plugin level only, since it's directly responsible for providing the valid database instance. Ideally, we also have to separate the database from the matching service and use the most appropriate solution for each service.

The following configuration is valid to start Temporal without any external dependencies or databases:

log:
  stdout: true
  level: error

persistence:
  defaultStore: default
  visibilityStore: visibility
  numHistoryShards: 1
  datastores:
    default:
      sql:
        user: ""
        password: ""
        pluginName: "sqlite"
        databaseName: "default"
        connectAddr: "localhost"
        connectProtocol: "tcp"
        connectAttributes:
          mode: "memory"
          cache: "private"
          setup: true
        maxConns: 1
        maxIdleConns: 1
        maxConnLifetime: "1h"
        tls:
          enabled: false
          сaFile: ""
          certFile: ""
          keyFile: ""
          enableHostVerification: false
          serverName: ""

    visibility:
      sql:
        user: ""
        password: ""
        pluginName: "sqlite"
        databaseName: "default"
        connectAddr: "localhost"
        connectProtocol: "tcp"
        connectAttributes:
          mode: "memory"
          cache: "private"
          setup: true
        maxConns: 1
        maxIdleConns: 1
        maxConnLifetime: "1h"
        tls:
          enabled: false
          сaFile: ""
          certFile: ""
          keyFile: ""
          enableHostVerification: false
          serverName: ""

services:
  frontend:
    rpc:
      grpcPort: 7233
      membershipPort: 6933
      bindOnLocalHost: true
    metrics:
      statsd:
        hostPort: "127.0.0.1:8125"
        prefix: "temporal"
  matching:
    rpc:
      grpcPort: 7235
      membershipPort: 6935
      bindOnLocalHost: true
    metrics:
      statsd:
        hostPort: "127.0.0.1:8125"
        prefix: "temporal"
  history:
    rpc:
      grpcPort: 7234
      membershipPort: 6934
      bindOnLocalHost: true
    metrics:
      statsd:
        hostPort: "127.0.0.1:8125"
        prefix: "temporal"
  worker:
    rpc:
      grpcPort: 7239
      membershipPort: 6939
      bindOnLocalHost: true
    metrics:
      statsd:
        hostPort: "127.0.0.1:8125"
        prefix: "temporal"

publicClient:
  hostPort: "localhost:7233"

global:
  membership:
    maxJoinDuration: 30s
    broadcastAddress: "127.0.0.1"

clusterMetadata:
  enableGlobalNamespace: false
  failoverVersionIncrement: 10
  masterClusterName: "active"
  currentClusterName: "active"
  clusterInformation:
    active:
      enabled: true
      initialFailoverVersion: 1
      rpcName: "frontend"
      rpcAddress: "localhost:7233"

The temporal database will be fully configured in memory (see connectionAttribute setup) and ready to use within a few seconds. You still have to init the namespace.

This setup can not be used on production for obvious reasons but can be used for demos, tests, and micro applications which do not experience high loads.

Tested locally with multiple concurrent workflows (1000). Observed performance ~30 workflows per second (hello world with a single activity, on PHP SDK).

Does not affect production usage.

P.S. Please let me know what you want me to change in here.

paulnpdev

just a couple of nits

common/persistence/sql/sqlplugin/sqlite/conn_pool.go

common/persistence/sql/sqlplugin/sqlite/plugin.go

yiminc

This looks good to me. @wolfy-j , thank you for your contribution.

common/persistence/sql/sqlplugin/sqlite/conn_pool.go

common/persistence/sql/sqlplugin/sqlite/plugin.go

wxing1292 · 2021-10-30T20:40:05Z

common/persistence/sql/sqlplugin/sqlite/conn_pool.go

+	refCount int
+}
+
+func newConnPool() *connPool {


connection pool exists here due to frontend / matching / history / worker (4 services) each will create its own DB connection & causing DB locking issue?

can you plz put a comment here?
we can possibly improve the DI to initialize this DB connection only once (for single binary), then this conn pool management can possibly be removed

Comment added. Yes, refactoring DI would be the ideal approach, but I'm not sure about the size of the impact.

Can we also try to turn off cluster, gossip and other services irrelevant for single binary server?

Also, some services open and close connections sequentially, getting DB connection stopped and destroying the database in memory.

Can we also try to turn off cluster, gossip and other services irrelevant for single binary server?

we definitely should (assuming we are talking about the ideal version), plus inter-service communication can by pass network layer.

Also, some services open and close connections sequentially, getting DB connection stopped and destroying the database in memory.

sorry what do you mean? if using DI, DI should only initialize the DB conn once (assuming 1 and only 1 conn)?

This use case won't exist with DI, but it exists now. We can probably take a look at it once it will be time to clean up all unnecessary services.

wxing1292

can you also create a new development_sqlite.yaml file with the static configs?

cmd/tools/sql/main.go

wxing1292 · 2021-10-30T21:05:23Z

also run make goimports

wolfy-j · 2021-11-02T18:51:56Z

I guess it's the question about turning CGO off or on now.

alexshtin · 2021-11-03T00:34:58Z

cmd/server/main.go

@@ -41,6 +41,7 @@ import (
 	"go.temporal.io/server/common/log/tag"
 	_ "go.temporal.io/server/common/persistence/sql/sqlplugin/mysql"      // needed to load mysql plugin
 	_ "go.temporal.io/server/common/persistence/sql/sqlplugin/postgresql" // needed to load postgresql plugin
+	_ "go.temporal.io/server/common/persistence/sql/sqlplugin/sqlite"     // needed to load sqlite plugin


This is what breaks the build. Apparently w/o this line, sqlite package is not even compiled. With this line it gets compiled but compilation fails because we don't use CGO for our production Linux builds and

Important: because this is a CGO enabled package you are required to set the environment variable CGO_ENABLED=1 and have a gcc compile present within your path.

I don't want to enable CGO and don't know what is the best way to solve this. The only one place where sqlite3 is used. Maybe we can avoid importing this package and use some other approach.

Well, you already have a driver which utilizes the SQLite CGO package. The alternative is to use a different type of embedded database... or something like this - https://www.reddit.com/r/golang/comments/n1u2b1/new_advanced_cgofree_sqlite_package/

I agree about avoiding CGO, maybe create a different build pipeline? Or it's too complicated?

I can also turn off the plugin in server and SQL so DataDog can patch their TemporalLite server.

FYI there was lots of discussion around CGO on the original PR. I did have a non-CGO version working at one point using modernc.org/sqlite, but we decided to just skip importing the driver by default: #1968 (comment)

I'd suggest doing the same here and follow up on supporting non-CGO builds or dealing with cross-compilation issues introduced by CGO separately.

In the mean time it would be pretty straightforward to add a file to the main package which uses a build tag to conditionally import the sqlite driver when CGO is enabled. That way anyone wanting to try out the sqlite driver just needs to compile from source rather than having to write their own Temporal server wrapper or pull in Temporalite.

jlegrone · 2021-11-30T22:46:48Z

common/persistence/sql/sqlplugin/sqlite/plugin.go

@@ -112,9 +123,38 @@ func (p *plugin) createDBConnection(
 	// Maps struct names in CamelCase to snake without need for db struct tags.
 	db.MapperFunc(strcase.ToSnake)

+	// only suitable for in memory databases!
+	if _, ok := cfg.ConnectAttributes["setup"]; ok {


Heads up we pass connection attributes more or less transparently into the sqlite driver here:

temporal/common/persistence/sql/sqlplugin/sqlite/plugin.go

Lines 176 to 187 in c38d169

for k, v := range cfg.ConnectAttributes {

key := strings.TrimSpace(k)

value := strings.TrimSpace(v)

if parameters.Get(key) != "" {

return nil, fmt.Errorf("duplicate connection attr: %v:%v, %v:%v",

key,

parameters.Get(key),

key, value,

)

}

parameters.Set(key, value)

}

The current driver may not be picky about it but I think we should avoid passing in non-standard sqlite config options. How about checking if the driver is using memory mode instead? When persisting to a file we can use the same migration patterns we do for postgres/mysql/cassandra backends.

Suggested change

if _, ok := cfg.ConnectAttributes["setup"]; ok {

if mode := cfg.ConnectAttributes["mode"]; mode == "memory" {

I have not seen a notification about this question. Ugh. I want to leave the dedicated flag since it makes possible to run bootstrap on sqlite file database (which is handy). But now this flag will be removed.

@jlegrone Also, I tried https://github.com/mattn/go-sqlite3 and I do not see a quick path to integrate it here without adding a few DB layer abstractions for sqlx compatibility. It might be easier and more effective to build a new DB type for in-memory use.

alexshtin

I enabled cgo by default in #2396, rebased this PR and tried to run it locally:

$ make bins         
Delete old binaries...
Build temporal-server with CGO_ENABLED=1 for linux/amd64...
go build -o temporal-server ./cmd/server
Build tctl with CGO_ENABLED=1 for linux/amd64...
go build -o tctl ./cmd/tools/cli
Build tctl-authorization-plugin with CGO_ENABLED=1 for linux/amd64...
go build -o tctl-authorization-plugin ./cmd/tools/cli/plugins/authorization
Build temporal-cassandra-tool with CGO_ENABLED=1 for linux/amd64...
go build -o temporal-cassandra-tool ./cmd/tools/cassandra
Build temporal-sql-tool with CGO_ENABLED=1 for linux/amd64...
go build -o temporal-sql-tool ./cmd/tools/sql

$ ./temporal-server --zone sqlite start
2022/01/20 17:21:41 Loading config; env=development,zone=sqlite,configDir=config
2022/01/20 17:21:41 Loading config files=[config/base.yaml config/development.yaml config/development_sqlite.yaml]

and it just hangs. I didn't debug it further. Does it work for you locally?

alexshtin · 2022-01-21T01:32:45Z

common/persistence/sql/sqlplugin/sqlite/plugin.go

+	// only suitable for in memory databases!
+	if mode := cfg.ConnectAttributes["mode"]; mode == "memory" {
+		// creates temporary DB overlay in order to configure database and schemas
+		err := p.setupSQLiteDatabase(cfg, db)


This call now conflicts with explicit schema creation in sqlite_test.go. Where it should be removed: here or in tests?

This is why unit tests are failing on Buildkite.

I think it makes sense to handle this inline here to simplify usage of the plugin; I don't know of any cases where it wouldn't make sense to initialize the schema with in-memory mode enabled.

In Temporalite we initialize schema when in-memory mode is being used OR when the db file doesn't already exist. We'd be able to skip the setup for in-memory mode here: https://github.com/DataDog/temporalite/blob/de413afe117f4a5117d1148198622f0f54d5edc2/server.go#L55-L64

As an aside, for file persistence mode I think we should aim for consistency with how migrations work with other sql storage drivers, ie. support sqlite via https://github.com/temporalio/temporal/tree/master/tools/sql.

I agree, removing it from tests.

alexshtin · 2022-01-21T01:38:15Z

config/development_sqlite.yaml

+        connectAttributes:
+          mode: "memory"
+          cache: "private"
+          setup: true


You don't need this setup parameter anymore, right?

Yes, it's no longer needed.

- old school is the best school

alexshtin · 2022-01-30T07:33:32Z

My bad, server actually was working. I am so get used to the log console output that we usually get, and I didn't notice that sqlite config has less verbose log level. I am changing the config to match to other SQL implementations. It might be noisy but I would like to keep in sync.

wolfy-j requested a review from a team October 29, 2021 13:57

paulnpdev reviewed Oct 29, 2021

View reviewed changes

common/persistence/sql/sqlplugin/sqlite/conn_pool.go Outdated Show resolved Hide resolved

common/persistence/sql/sqlplugin/sqlite/conn_pool.go Outdated Show resolved Hide resolved

wolfy-j requested a review from paulnpdev October 29, 2021 16:37

yiminc reviewed Oct 29, 2021

View reviewed changes

wxing1292 self-assigned this Oct 30, 2021

yiminc approved these changes Oct 30, 2021

View reviewed changes

common/persistence/sql/sqlplugin/sqlite/conn_pool.go Show resolved Hide resolved

common/persistence/sql/sqlplugin/sqlite/plugin.go Show resolved Hide resolved

wxing1292 reviewed Oct 30, 2021

View reviewed changes

wxing1292 approved these changes Oct 30, 2021

View reviewed changes

wxing1292 reviewed Oct 30, 2021

View reviewed changes

cmd/tools/sql/main.go Outdated Show resolved Hide resolved

wolfy-j requested a review from wxing1292 October 31, 2021 09:32

alexshtin reviewed Nov 3, 2021

View reviewed changes

paulnpdev approved these changes Nov 30, 2021

View reviewed changes

jlegrone reviewed Nov 30, 2021

View reviewed changes

alexshtin force-pushed the feature/single-binary-server branch 2 times, most recently from 9bd25fc to 244df69 Compare January 21, 2022 01:20

alexshtin requested changes Jan 21, 2022

View reviewed changes

wolfy-j and others added 12 commits January 29, 2022 22:53

- enables SQLite plugin for the server build and SQL tools

4a08b2f

- added SQLite connection pool and auto-setup

fd91222

- redundant code

aff3b39

- typo

edd5b2a

- old school is the best school

- drop the header

aab9044

- drop the header

d0d618a

- delete internal setup flag

bc20e53

- automatically setup in memory mode

38bf2ca

Add bins-cgo target

468f447

Revert Makefile changes

e2defe2

Fix import aliases

7c0b214

Fix tests and config

00a0426

alexshtin force-pushed the feature/single-binary-server branch from 2d8c7c0 to 00a0426 Compare January 30, 2022 07:31

alexshtin added 5 commits January 30, 2022 00:32

Add SQLite integration tests pipeline

af3be5f

Set ConnectAttributes only for SQLite

8b88f6d

Fix cycle

934abb3

Minor fix

de3d867

Expose ConnectAttributes

26fe219

alexshtin approved these changes Jan 30, 2022

View reviewed changes

Add SQLite coverage report

4a5f0d4

alexshtin merged commit 736fabf into temporalio:master Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single binary server (SQLite) and concurrency bugfixes for in memory storages #2114

Single binary server (SQLite) and concurrency bugfixes for in memory storages #2114

wolfy-j commented Oct 29, 2021 •

edited

Loading

paulnpdev left a comment

yiminc left a comment

wxing1292 Oct 30, 2021

wolfy-j Oct 31, 2021

wolfy-j Oct 31, 2021

wolfy-j Oct 31, 2021

wxing1292 Nov 2, 2021

wolfy-j Nov 2, 2021

wxing1292 left a comment

wxing1292 commented Oct 30, 2021

wolfy-j commented Nov 2, 2021

alexshtin Nov 3, 2021

alexshtin Nov 3, 2021

wolfy-j Nov 3, 2021

wolfy-j Nov 3, 2021

jlegrone Nov 30, 2021 •

edited

Loading

jlegrone Nov 30, 2021 •

edited

Loading

jlegrone Nov 30, 2021

wolfy-j Dec 21, 2021

wolfy-j Dec 21, 2021

alexshtin left a comment •

edited

Loading

alexshtin Jan 21, 2022

alexshtin Jan 21, 2022

jlegrone Jan 27, 2022

alexshtin Jan 30, 2022

alexshtin Jan 21, 2022

wolfy-j Jan 30, 2022

alexshtin commented Jan 30, 2022

	for k, v := range cfg.ConnectAttributes {
	key := strings.TrimSpace(k)
	value := strings.TrimSpace(v)
	if parameters.Get(key) != "" {
	return nil, fmt.Errorf("duplicate connection attr: %v:%v, %v:%v",
	key,
	parameters.Get(key),
	key, value,
	)
	}
	parameters.Set(key, value)
	}

	if _, ok := cfg.ConnectAttributes["setup"]; ok {
	if mode := cfg.ConnectAttributes["mode"]; mode == "memory" {

Single binary server (SQLite) and concurrency bugfixes for in memory storages #2114

Single binary server (SQLite) and concurrency bugfixes for in memory storages #2114

Conversation

wolfy-j commented Oct 29, 2021 • edited Loading

paulnpdev left a comment

Choose a reason for hiding this comment

yiminc 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

wxing1292 left a comment

Choose a reason for hiding this comment

wxing1292 commented Oct 30, 2021

wolfy-j commented Nov 2, 2021

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

jlegrone Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

jlegrone Nov 30, 2021 • edited Loading

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

alexshtin left a comment • edited Loading

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

alexshtin commented Jan 30, 2022

wolfy-j commented Oct 29, 2021 •

edited

Loading

jlegrone Nov 30, 2021 •

edited

Loading

jlegrone Nov 30, 2021 •

edited

Loading

alexshtin left a comment •

edited

Loading