-
Notifications
You must be signed in to change notification settings - Fork 40
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
update database schema update docs #4393
Conversation
schema/crdb/README.adoc
Outdated
separate transaction. | ||
** CockroachDB documentation recommends the following: "Execute schema | ||
changes... in a single explicit transaction consisting of the single schema |
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'm not sure if the CockroachDB docs changed, but the text as-is here does not (quite) appear in the linked docs. I only noticed because I was trying to learn more and looked for the source and had a hard time finding it. There's an extra word "single" here.
schema/crdb/README.adoc
Outdated
* All `up.sql` files can be applied twice without error | ||
|
||
==== Handling common schema changes | ||
==== General notes |
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.
This is kind of a vague heading but the one that was here didn't seem to describe the contents of this section.
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.
Maybe "Writing Schema Change Statements"? This appears to be advice for making particular statements possible, assuming that the reader already understands the high-level flow of how they're applied.
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.
Changes look good. I share the question about the "Adding new source tables to an existing view" and I agree about grouping those last sections under a "Scenario-specific gotchas" heading.
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.
Thanks for cleaning this up!
** Each file should contain _either_ one schema-modifying statement _or_ some | ||
number of data-modifying statements. You can combine multiple data-modifying | ||
statements. But you should not mix schema-modifying statements and | ||
data-modifying statements in one file. And you should not include multiple | ||
schema-modifying statements in one file. |
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.
Great summary, this is spot-on.
Technically there are exceptions:
Some schema change operations can be run within explicit, multiple statement transactions. CREATE TABLE and CREATE INDEX statements can be run within the same transaction with the same atomicity guarantees as other SQL statements. There are no performance or rollback issues when using these statements within a multiple statement transaction.
But I think this is an apt summary, unless someone performing a schema change can cite more-specific docs from CRDB for a multi-statement transaction.
schema/crdb/README.adoc
Outdated
* All `up.sql` files can be applied twice without error | ||
|
||
==== Handling common schema changes | ||
==== General notes |
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.
Maybe "Writing Schema Change Statements"? This appears to be advice for making particular statements possible, assuming that the reader already understands the high-level flow of how they're applied.
I like this title (whether you meant it seriously or not). Done in 658f195. |
Enabling auto-merge because I think I've addressed all the feedback. |
I found some of the schema update docs confusing so I took a swing at clarifying them. I still had some questions (will comment inline below).
edit: I fixed the line wrapping here in a separate commit so that you can ignore the re-wrapping changes by just ignoring commit 47ddd43. (The file is hard-wrapped, but it was wrapped inconsistently at anywhere from 80 to 90 columns, which was surprisingly disruptive to read (at least in my editor, because I saw both the hard wraps and the soft wraps, so every other line was like 5 columns long).)