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

update database schema update docs #4393

Merged
merged 7 commits into from
Nov 8, 2023
Merged

update database schema update docs #4393

merged 7 commits into from
Nov 8, 2023

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
schema/crdb/README.adoc Outdated Show resolved Hide resolved
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.

schema/crdb/README.adoc Show resolved Hide resolved
* 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.

schema/crdb/README.adoc Outdated Show resolved Hide resolved
@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!

schema/crdb/README.adoc Show resolved Hide resolved
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.

schema/crdb/README.adoc Outdated Show resolved Hide resolved
* 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.

schema/crdb/README.adoc Outdated Show resolved Hide resolved
@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
18 of 20 checks passed
@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.

4 participants