Skip to content

Conversation

ericharmeling
Copy link
Contributor

Fixes #5356.

  • Added a new page on using Flyway with CRDB. This page includes a simple tutorial, and some notes on transaction boundaries and retries.

@ericharmeling ericharmeling requested a review from rafiss May 14, 2020 18:48
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ericharmeling
Copy link
Contributor Author

@vy-ton @awoods187

@cockroach-teamcity
Copy link
Member

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

looks great to me. thanks!

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@ericharmeling ericharmeling requested a review from lnhsingh May 14, 2020 22:04
Copy link
Contributor

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

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

Nice job! This :lgtm: , with a couple format nits / suggestions on adding links

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling)


v20.1/flyway.md, line 42 at r1 (raw file):

    {{site.data.alerts.callout_info}}
    The SSL connection parameters in the connection URL must specify the full path to the certificates that you generated when you [started the secure cluster](secure-a-cluster.html). Also, the user that you specify (e.g., `max`) must also have admin privileges on the database whose schema you want to change (e.g., `bank`).

Link "admin privileges" to grant.html


v20.1/flyway.md, line 61 at r1 (raw file):

    ~~~ sql
    /* Create accounts table */
    CREATE TABLE IF NOT EXISTS accounts (

I think the start of the statement lines for CREATE TABLE.. and INSERT INTO.. need > preceding them


v20.1/flyway.md, line 94 at r1 (raw file):

## Step 4. Add additional migrations

Suppose that you want to change the primary key of the `accounts` table from a simple, incrementing integer (in this case, `id`) to an auto-generated UUID, to follow some [CockroachDB best practices](performance-best-practices-overview.html#unique-id-best-practices). You can make these changes to the schema by creating and executing an additional migration:

You might want to link to int.html and uuid.html here


v20.1/flyway.md, line 110 at r1 (raw file):

    ~~~ sql
    /* Add new UUID-typed column */
    ALTER TABLE accounts ADD COLUMN unique_id UUID NOT NULL DEFAULT gen_random_uuid();

Add > on the statement lines


v20.1/flyway.md, line 162 at r1 (raw file):

### Transaction retries

When multiple, concurrent transactions or statements are issued to a single CockroachDB cluster, [transaction contention](performance-best-practices-overview.html#understanding-and-avoiding-transaction-contention) can cause schema migrations to fail. In the event of transaction contention, CockroachDB returns a `40001` SQLSTATE (i.e., a serialization failure), and Flyway automatically retries the migration. For more information about client-side transaction retries in CockroachDB, see [Transaction Retries](transactions.html#transaction-retries).

nit: 40001 SQLSTATE > 40001 SQLSTATE

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @lnhsingh and @rafiss)


v20.1/flyway.md, line 42 at r1 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

Link "admin privileges" to grant.html

Done.


v20.1/flyway.md, line 61 at r1 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

I think the start of the statement lines for CREATE TABLE.. and INSERT INTO.. need > preceding them

Because these are lines in a .sql file that is parsed by Flyway and not directly by the SQL shell, I think we can omit the >.


v20.1/flyway.md, line 94 at r1 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

You might want to link to int.html and uuid.html here

Done.


v20.1/flyway.md, line 110 at r1 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

Add > on the statement lines

Because these are lines in a .sql file that is parsed by Flyway and not directly by the SQL shell, I think we can omit the >.


v20.1/flyway.md, line 162 at r1 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

nit: 40001 SQLSTATE > 40001 SQLSTATE

Done.

@cockroach-teamcity
Copy link
Member

@ericharmeling ericharmeling requested review from vy-ton and awoods187 May 18, 2020 15:36
@ericharmeling
Copy link
Contributor Author

@awoods187 @vy-ton
This is ready to merge from a technical point of view. Adding both of you to review to make sure we have the product messaging correct.

Copy link
Contributor

@vy-ton vy-ton left a comment

Choose a reason for hiding this comment

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

Read through the section and :lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @awoods187, @ericharmeling, @lnhsingh, and @rafiss)


v20.1/flyway.md, line 158 at r2 (raw file):

When used with CockroachDB, Flyway does *not* wrap schema migrations in transactions. [Transaction boundaries](transactions.html) are instead handled by CockroachDB.

Probably for Andy, what could the user see because of this?

Copy link
Contributor

@awoods187 awoods187 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @lnhsingh, @rafiss, and @vy-ton)


v20.1/flyway.md, line 158 at r2 (raw file):

Previously, vy-ton (Vy Ton) wrote…
When used with CockroachDB, Flyway does *not* wrap schema migrations in transactions. [Transaction boundaries](transactions.html) are instead handled by CockroachDB.

Probably for Andy, what could the user see because of this?

we dont do well with schema changes inside transactions for a variety of reasons. It should cause a problem for the user to not have them in place since these migrations are running a series of schema changes and aren't trying to modify the data in the same transaction

@ericharmeling ericharmeling merged commit 0a38358 into master May 19, 2020
@ericharmeling ericharmeling deleted the flyway branch August 24, 2020 16:42
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.

sql: document Flyway
6 participants