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

Consider constraints when sorting columns #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nvanoorschot
Copy link
Contributor

The ordering of columns is a great option that we use for a while now. The current implementation however does not take table CONSTRAINTs into account. This results in all CONSTRAINTs to be placed in front of the columns definitions, which is not the PostgreSQL default. And it does not actually sort the CONSTRAINTs since it only sorts based on the first word of the statement.

This PR fixes both issues.

PS: most whitespace correction tools enforce files to end with a single end-of-line character by default. The dump always ends with 2. It is a small issue I also fixed here, but if you like I can place it in a separate PR.

Let me know what you think.

@nvanoorschot
Copy link
Contributor Author

Hi Lukas,

Do you have time to look at this PR? It changes the output structure from:

CREATE TABLE(
  CONTRAINT c,
  CONTRAINT a,
  CONTRAINT b,
  column_a,
  column_b,
  column_c
)

to:

CREATE TABLE(
  column_a,
  column_b,
  column_c,
  CONTRAINT a,
  CONTRAINT b,
  CONTRAINT c
)

@@ -127,52 +127,34 @@ def run

# Reduce 2+ lines of whitespace to one line of whitespace
dump.gsub!(/\n{2,}/m, "\n\n")
# End the file with a single end-of-line character
dump.sub!(/\n*\z/m, "\n")

This comment was marked as outdated.

@andyatkinson
Copy link
Collaborator

@nvanoorschot It's been a long time. Is this still something you're looking to get in? If so, please confirm and rebase on latest master. If you could address the comment from @RKushnir as well, that would help.

Can you confirm that this change would re-order all the existing constraint positions in the existing db/structure.sql files, out in the wild? If so, we should probably put that in the Changelog notes at a minimum.

Did you get a chance to verify all constraint types are treated the same, even if they aren't in your schema? PK, FK, Unique, Not null, check, Exclusion etc.? https://www.postgresql.org/docs/current/ddl-constraints.html

@RKushnir
Copy link
Contributor

If you could address the comment from @RKushnir as well, that would help.

Oh, thanks for the hat tip :) But I no longer know what I meant with that comment and I would probably not suggest that today. So I'm just going to withdraw my comment, pretend it's not there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants