Skip to content
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

Clickhouse: set mutations_sync 2 at delete version to avoid apply down migration twice #454

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

chapsuk
Copy link
Contributor

@chapsuk chapsuk commented Jan 26, 2023

Got an error for down-to command, steps for reproduce:

  1. Setup test env by blog post
    https://pressly.github.io/goose/blog/2022/improving-clickhouse/#getting-started
  2. Add one more migration file
≻ cat tests/clickhouse/testdata/migrations/00004_d.sql 
-- +goose Up
ALTER TABLE clickstream ADD COLUMN foo String;

-- +goose Down
ALTER TABLE clickstream DROP COLUMN foo;
  1. Up version
≻ GOOSE_DRIVER=clickhouse \
          GOOSE_DBSTRING="tcp://clickuser:password1@localhost:9000/clickdb" \
          GOOSE_MIGRATION_DIR="tests/clickhouse/testdata/migrations" \
          ./goose up
2023/01/26 12:23:15 OK   00004_d.sql (6.82ms)
2023/01/26 12:23:15 goose: no migrations to run. current version: 4
  1. Down-to version 2
≻ GOOSE_DRIVER=clickhouse \
          GOOSE_DBSTRING="tcp://clickuser:password1@localhost:9000/clickdb" \
          GOOSE_MIGRATION_DIR="tests/clickhouse/testdata/migrations" \
          ./goose down-to 2
2023/01/26 12:23:18 OK   00004_d.sql (12.85ms)
2023/01/26 12:23:18 goose run: ERROR 00004_d.sql: failed to run SQL migration: failed to execute SQL query "ALTER TABLE clickstream DROP COLUMN foo;\n": code: 10, message: Wrong column name. Cannot find column `foo` to drop

Expected:

version 2, no error

Actual:

00004_d.sql down migrations applied twice

Fix:

set mutations_sync = 2 for delete version query (ALTER ... DELETE) to execute query synchronously 

@mfridman
Copy link
Collaborator

Given the nature of async in ClickHouse, is it normal for ClickHouse migrations to be run one at a time?

I originally had some thoughts/questions in this issue, but ideally, we get some input from the community on ClickHouse best practices.

@chapsuk
Copy link
Contributor Author

chapsuk commented Jan 27, 2023

Given the nature of async in ClickHouse, is it normal for ClickHouse migrations to be run one at a time?

For our use cases it's ok:

  • CI pipelines when migrations runs at fresh ClickHouse instance from first to last
  • Prod/Stage environment, usually we run only one version at a time

@mfridman
Copy link
Collaborator

Given the nature of async in ClickHouse, is it normal for ClickHouse migrations to be run one at a time?

For our use cases it's ok:

  • CI pipelines when migrations runs at fresh ClickHouse instance from first to last
  • Prod/Stage environment, usually we run only one version at a time

Do you think this should be configurable, where the default behavior is the current behavior, but users can configure the ClickHouse mutations_sync setting?

@chapsuk
Copy link
Contributor Author

chapsuk commented Jan 27, 2023

I think it must be set for goose db version queries only, because nature of that queries is synchronously

@mfridman
Copy link
Collaborator

mfridman commented Jan 27, 2023

To confirm, we only need to do this for the goose down version, and the goose up version is writing data synchronously by default?

EDIT: indeed that appears to be the case, the relevant docs.

Copy link
Collaborator

@mfridman mfridman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@mfridman
Copy link
Collaborator

mfridman commented Jan 27, 2023

Hmm, this means we should be able to update the ClickHouse test and remove the retryCheckTableMutation function in here:

func TestClickUpDownAll(t *testing.T) {

I added a retry function to ensure the end state of the down-to 0 migration is correct, but with this change, we can remove that and expect the correct behavior because it's happening synchronously.

@mfridman
Copy link
Collaborator

@chapsuk you'll need to rebase (or merge) latest master and then fix the lint issues. I added a linter in #456

@mfridman mfridman merged commit ad90652 into pressly:master Jan 27, 2023
@chapsuk chapsuk deleted the clickhouse_mutate_sync branch January 27, 2023 14:42
@chapsuk
Copy link
Contributor Author

chapsuk commented Jan 27, 2023

Thanks!

@chapsuk
Copy link
Contributor Author

chapsuk commented Jan 27, 2023

To confirm, we only need to do this for the goose down version, and the goose up version is writing data synchronously by default?

I believe yes, because we are using MergeTree engine. For ReplicatedMergeTree or Distributed engines it should to use insert_quorum or insert_distributed_sync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants