-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add replication support for ClickHouse state table + Fixes ListMigration query for ClickHouse #520
base: master
Are you sure you want to change the base?
Changes from all commits
013a935
235a213
a820168
93e0cdb
b37f6e8
8144d06
0debc41
ffbcf8b
51168fd
e29e8ea
b6c510a
ab72893
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
package cfg | ||
|
||
import "os" | ||
import ( | ||
"os" | ||
) | ||
|
||
var ( | ||
GOOSEDRIVER = envOr("GOOSE_DRIVER", "") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,19 @@ package dialectquery | |
|
||
import "fmt" | ||
|
||
type Clickhouse struct{} | ||
const ( | ||
paramOnCluster = "ON_CLUSTER" | ||
paramClusterMacro = "CLUSTER_MACRO" | ||
) | ||
|
||
type clusterParameters struct { | ||
OnCluster bool | ||
ClusterMacro string | ||
} | ||
|
||
type Clickhouse struct { | ||
Params clusterParameters | ||
} | ||
|
||
var _ Querier = (*Clickhouse)(nil) | ||
|
||
|
@@ -11,10 +23,23 @@ func (c *Clickhouse) CreateTable(tableName string) string { | |
version_id Int64, | ||
is_applied UInt8, | ||
date Date default now(), | ||
tstamp DateTime default now() | ||
tstamp DateTime64(9, 'UTC') default now64(9, 'UTC') | ||
) | ||
ENGINE = MergeTree() | ||
ORDER BY (date)` | ||
ENGINE = KeeperMap('/goose_version') | ||
PRIMARY KEY version_id` | ||
|
||
qCluster := `CREATE TABLE IF NOT EXISTS %s ON CLUSTER '%s' ( | ||
version_id Int64, | ||
is_applied UInt8, | ||
date Date default now(), | ||
tstamp DateTime64(9, 'UTC') default now64(9, 'UTC') | ||
) | ||
ENGINE = KeeperMap('/goose_version_repl') | ||
PRIMARY KEY version_id` | ||
|
||
if c.Params.OnCluster { | ||
return fmt.Sprintf(qCluster, tableName, c.Params.ClusterMacro) | ||
} | ||
return fmt.Sprintf(q, tableName) | ||
} | ||
|
||
|
@@ -34,6 +59,20 @@ func (c *Clickhouse) GetMigrationByVersion(tableName string) string { | |
} | ||
|
||
func (c *Clickhouse) ListMigrations(tableName string) string { | ||
q := `SELECT version_id, is_applied FROM %s ORDER BY version_id DESC` | ||
q := `SELECT version_id, is_applied FROM %s ORDER BY tstamp DESC` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is fixing a bug? (I think you mentioned this in the description) The core goose library relies on a non-user-defined order for when migrations get applied, this is typically a sequential id in all other databases. It seems here we were using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. There is a no equivalent of an auto incrementing ID in clickhouse and it doesn't provide any way to get a globally incremented unique ID across its nodes. So I relied on using timestamps, with just second precision, it was non-deterministic as migrations could have same timestamps. With nanosecond precision, that chance is almost non-existent and the implementation further augments it with use of KeeeperMap Engine. |
||
return fmt.Sprintf(q, tableName) | ||
} | ||
|
||
func (c *Clickhouse) AttachOptions(options map[string]string) error { | ||
if val, ok := options[paramOnCluster]; ok { | ||
if val == "true" { | ||
clusterMacro, ok := options[paramClusterMacro] | ||
if !ok { | ||
clusterMacro = "{cluster}" | ||
} | ||
c.Params.ClusterMacro = clusterMacro | ||
c.Params.OnCluster = true | ||
} | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
package dialectquery | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
) | ||
|
||
func TestClickhouseCreateTable(t *testing.T) { | ||
t.Parallel() | ||
|
||
type testData struct { | ||
clickhouse *Clickhouse | ||
result string | ||
} | ||
|
||
tests := []testData{ | ||
{ | ||
clickhouse: &Clickhouse{ | ||
Params: clusterParameters{ | ||
OnCluster: true, | ||
ClusterMacro: "{cluster}", | ||
}, | ||
}, | ||
result: `CREATE TABLE IF NOT EXISTS schema_migrations ON CLUSTER '{cluster}' ( | ||
version_id Int64, | ||
is_applied UInt8, | ||
date Date default now(), | ||
tstamp DateTime64(9, 'UTC') default now64(9, 'UTC') | ||
) | ||
ENGINE = KeeperMap('/goose_version_repl') | ||
PRIMARY KEY version_id`, | ||
}, | ||
{ | ||
clickhouse: &Clickhouse{ | ||
Params: clusterParameters{ | ||
OnCluster: true, | ||
ClusterMacro: "dev-cluster", | ||
}, | ||
}, | ||
result: `CREATE TABLE IF NOT EXISTS schema_migrations ON CLUSTER 'dev-cluster' ( | ||
version_id Int64, | ||
is_applied UInt8, | ||
date Date default now(), | ||
tstamp DateTime64(9, 'UTC') default now64(9, 'UTC') | ||
) | ||
ENGINE = KeeperMap('/goose_version_repl') | ||
PRIMARY KEY version_id`, | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
out := test.clickhouse.CreateTable("schema_migrations") | ||
if diff := cmp.Diff(test.result, out); diff != "" { | ||
t.Errorf("clickhouse.CreateTable() mismatch (-want +got):\n%s", diff) | ||
} | ||
} | ||
} | ||
|
||
func TestClickhouseAttachOptions(t *testing.T) { | ||
t.Parallel() | ||
|
||
type testData struct { | ||
options map[string]string | ||
input *Clickhouse | ||
err error | ||
expected clusterParameters | ||
} | ||
|
||
tests := []testData{ | ||
{ | ||
options: map[string]string{ | ||
"ON_CLUSTER": "true", | ||
}, | ||
input: &Clickhouse{}, | ||
err: nil, | ||
expected: clusterParameters{ | ||
OnCluster: true, | ||
ClusterMacro: "{cluster}", | ||
}, | ||
}, | ||
{ | ||
options: map[string]string{ | ||
"ON_CLUSTER": "true", | ||
"CLUSTER_MACRO": "dev-cluster", | ||
}, | ||
input: &Clickhouse{}, | ||
err: nil, | ||
expected: clusterParameters{ | ||
OnCluster: true, | ||
ClusterMacro: "dev-cluster", | ||
}, | ||
}, | ||
{ | ||
options: map[string]string{ | ||
"ON_CLUSTER": "false", | ||
}, | ||
input: &Clickhouse{}, | ||
err: nil, | ||
expected: clusterParameters{ | ||
OnCluster: false, | ||
}, | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
err := test.input.AttachOptions(test.options) | ||
if err != test.err { | ||
t.Errorf("AttachOptions mismatch expected error: %v, got: %v", test.err, err) | ||
} | ||
if diff := cmp.Diff(test.expected, test.input.Params); diff != "" { | ||
t.Errorf("clickhouse.AttachOptions() mismatch (-want +got):\n%s", diff) | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,3 +26,9 @@ type Querier interface { | |
// The query should return the version_id and is_applied columns. | ||
ListMigrations(tableName string) string | ||
} | ||
|
||
// QuerierOptions is an interface to provide specific options to a dialect. | ||
// For ex: providing replication support to underlying migration table. | ||
type QuerierOptions interface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I initially added a "store" and "querier" the idea was to have the core goose library only interact with the store, and have no idea about the underlying queries. The store abstracts the underlying querier which holds the raw string queries for the different database technologies. Not sure if this made it easier/harder to evolve, but we should probably think about if there's a more elegant way to now thread options from the core library to those 2 internal layers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I struggled around this as well. I didn't find an easier way to hookup options all the way to the implementation specific detail. I tried to minimize the leaky abstraction(s) but I think it will have to exposed in one way. If you have any suggestions, I can try those out. |
||
AttachOptions(map[string]string) error | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been coming back to this PR on/off, trying to think through the implementation from the perspective of a "general purpose" migration tool. On the surface, everything looks reasonable with the exception of changing the engine.
For context, another PR #530 came in requesting another engine.
Should the default engine be
MergeTree
but allow this to be configurable?Maybe we could put our heads together on how to solve this so the tool can be useful for all or most ClickHouse users? cc @iamluc @arunk-s
I think PR #530 is on the right track to setting a custom engine, and afaics this would satisfy:
MergeTree()
ReplicatedMergeTree()
KeeperMap()
Other questions, does the
ORDER BY
orPARTITION BY
need to be configurable? Hopefully not so we can make this as generic as possible.Last thought, leaving
ENGINE = %s
to the user may be a footgun and I wish we could do more upfront validation, but then we have to account for all the possible ClickHouse engine types and this seems like a lot of work to maintain.Maybe there's a world where the user is allowed to supply a template for the table and as long as it contains:
version_id
is_applied
(legacy field, deprecated)tstamp
Not sure I like this idea, since goose should own the table it creates to ensure the identifiers match up across all implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I had a quick look at the PR and it looks good to me (and could replace my own PR #530)
My only concern is about the engine. I'm not a ClickHouse expert and didn't know about the
KeeperMap
engine, but it seems it must be enabled (https://clickhouse.com/docs/en/engines/table-engines/special/keeper-map) which could be not possible with hosted services.I suggest to add the engine as a new option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an option to toggle underlying table engine can definitely work!
I think these settings are affected by the choice of Engine so yeah they'll need to be configurable if the custom engine is allowed.
I can understand the concern with hosted services to not have
KeeperMap
though I doubt that platforms like ClickHouse Cloud disallows creating tables with this table engine.Definitely, convention over configuration is generally desirable however in this particular case I'm afraid the abstractions will leak a bit in order to properly support the underlying database. But in any case we need to have a sensible default and only selected number of choices.
The library does quite well supporting different databases but that does come at a cost I'm afraid.
I'll be happy to lend a hand with ClickHouse specific topics though it can take a bit of time for me to catch up.