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

Add no-downtime migrations support for composite types #21

Merged
merged 5 commits into from
Apr 30, 2019

Conversation

arybczak
Copy link
Collaborator

No description provided.

Copy link
Contributor

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor comments.

tableHasLess :: Show t => Text -> t -> Text
tableHasLess ptype missing =
"Table in the database has *less*" <+> ptype <+>
objectHasLess :: Show t => Text -> Text -> t -> Text
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: ptype seems to always be "columns", maybe these two should be objectHas{Less,More}Columns?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's leave it as-is, I don't think it's a big deal.

@@ -15,7 +15,7 @@ import Prelude
data Check = Check {
chkName :: RawSQL ()
, chkCondition :: RawSQL ()
, chkValidated :: Bool -- ^ Set to 'True' if check is created as NOT VALID and
, chkValidated :: Bool -- ^ Set to 'False' if check is created as NOT VALID and
-- not validated afterwards.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/not validated/validated/? We're supposed to call sqlValidateCheck at some future point after all.

Maybe rephrase as: "When set to 'False', the check is initially created as NOT VALID. It should be eventually validated with 'sqlValidateCheck'.".

Similarly for fkValidated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fkValidated should be set to False if check is created as NOT VALID and left like this (for whatever reason), so the current description is imo alright.

Copy link
Contributor

Choose a reason for hiding this comment

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

fkValidated should be set to False if check is created as NOT VALID and left like this (for whatever reason)

OK, maybe change both comments to say this instead of current wording? I find it less confusing than the current one (with the double negative).

@arybczak
Copy link
Collaborator Author

I pushed additional commit with support for concurrent index creation, please take a look at it.

Copy link
Contributor

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

Latest changes look OK, but there's a compile failure on GHC <= 8.2 which seems easy enough to fix (missing import).

@arybczak
Copy link
Collaborator Author

@23Skidoo Things seem to be good now. Would you mind merging and creating a release?

@23Skidoo 23Skidoo merged commit fecf157 into master Apr 30, 2019
@23Skidoo 23Skidoo deleted the no-downtime-composites branch April 30, 2019 12:58
@23Skidoo
Copy link
Contributor

Merged, will cut a release shortly.

@23Skidoo
Copy link
Contributor

New release is now on Hackage.

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.

None yet

2 participants