Skip to content

Conversation

@davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Oct 30, 2023

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).)

@davepacheco davepacheco requested a review from smklein October 30, 2023 21:26
lexicographic order before being executed. Each will be executed in a
separate transaction.
** CockroachDB documentation recommends the following: "Execute schema
changes... in a single explicit transaction consisting of the single schema
Copy link
Collaborator Author

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.

* All `up.sql` files can be applied twice without error

==== Handling common schema changes
==== General notes
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

@davepacheco davepacheco marked this pull request as ready for review October 30, 2023 21:55
Copy link
Contributor

@david-crespo david-crespo left a 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.

Copy link
Collaborator

@smklein smklein left a 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!

Comment on lines +69 to +73
** 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.
Copy link
Collaborator

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.

* All `up.sql` files can be applied twice without error

==== Handling common schema changes
==== General notes
Copy link
Collaborator

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.

@davepacheco
Copy link
Collaborator Author

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.

I like this title (whether you meant it seriously or not). Done in 658f195.

@davepacheco
Copy link
Collaborator Author

Enabling auto-merge because I think I've addressed all the feedback.

@davepacheco davepacheco enabled auto-merge (squash) November 8, 2023 18:59
@davepacheco davepacheco merged commit 6478a97 into main Nov 8, 2023
@davepacheco davepacheco deleted the dap/schema-docs branch November 8, 2023 20:27
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.

5 participants