Sort table columns by name when dumping schema#53281
Conversation
|
I think it makes sense, anything that helps with
I also wonder if ignoring the column other could result in bug. I think as long as you use higher level Active Record APIs you're fine, but there might be some really intricate cases where code is dependent on columns ordering? |
|
@fatkodima i saw your change to reduce merge conflicts in schema #53797 and wondering what you think about the change I have here. Have you noticed issues with columns changing order in the schema.rb and would the change I've made here help your team with that? Happy to make changes here too if you had suggestions. |
|
#53797 is about |
|
I didn't get some other opinion, but I think this change is OK. It just need to be added in |
|
@byroot ah, sorry, I misread the PR and didn't look at the diff itself. I'll update with those changes and then ask for another review, thanks! |
|
Sorry for the delayed reply 😅 Yeah, I worry partly about the Footnotes
|
|
@matthewd thanks for the feedback, I kind of worry about that too but I also think it's already happening and people just ignore those diffs. Either way I'll make it something that can be disabled so that people can opt out of it if they want. |
|
This should not be an option. This should just be default behavior. Schema dumping should be deterministic regardless of the platform. |
48a539c to
d430b58
Compare
|
@dhh sounds good! Just updated the PR to remove the option and always sort the columns when dumping. If you want any other changes let me know. |
d430b58 to
e1adacc
Compare
e1adacc to
e403adf
Compare
|
I rebased your branch because it was quite outdated. I also added a changelog entry, I'll merge on green. |
|
Thanks @byroot! |
|
Agree there's no need for a separate option here: this can easily be left to the category of low-level schema details where, if you need to reproduce your database more precisely than our schema dump, that's what the |
|
Hi! I wrote the original fix-db-schema-conflicts gem to solve this problem 11 years ago. Over time we ended up adding several other sorts:
Should we add sorts to those other things too? |
|
@jakeonrails I had looked at a few of those when I made my change and it seemed like indexes and foreign keys were already sorted. Feel free to take another look though in case I missed something (I didn't look at extensions). Since we're doing columns now I don't see any reason not to do the others. |
|
Hmm… so I wanted to write up a PR with a bug fix since I discovered the schema re-sorts my table columns, but I've found this was changed on purpose… The description says this adds an option to change the behavior like this - I would strongly vote against making this non-configurable, otherwise I think I'm just gonna have to monkey-patch this method. I want my table columns to be in the order I specified, not alphabetical. Yes, this makes the order deterministic, but it's now deterministically *wrong* ¯\_(ツ)_/¯ If this stays non-configurable, then I think you'd also need to remove support for |
…abetically in all test SQL and create_table definitions
…ls/rails#53281 by sorting columns alphabetically in all test SQL and create_table definitions
|
Ordering column in the table is a known performance optimization in postgresql |
|
This PR appears to diverge from the original proposal. Ideally, sorting columns alphabetically in For many developers, the order of columns in relational database tables is more than a stylistic preference. It often serves practical purposes. While some ordering choices are subjective, consistent patterns can significantly improve readability and workflow efficiency. For example, I find it helpful to place foreign keys at the beginning. Knowing that Altering this established behavior could be seen as a step backward for many users who rely on this structure for clarity and productivity. It may be worth reconsidering whether enforcing alphabetical sorting by default is necessary or whether it could remain configurable to accommodate different workflows. |
|
FYI, I have a PR here adding an option to restore the previous behavior of the dumper, I rebased it now on the latest version: #55414 |
|
A real world example: we interact with customers that dump CSV files from their databases using a |
Motivation / Background
In projects when multiple people are creating migrations they can often be run out of order from one another, this results in each developer machine having columns added in a different order. Each time we run migrations the schema is dumped based on our local database base so the schema output has columns out of order from machine to machine resulting in noisy diffs and potentially conflicts in the schema file that make no difference overall. ex:
Detail
This PR adds an option to sort the columns before outputting to the table dump. I found this previous PR which does this just for MySQL, but I decided to do this at the base schema dumper level as an option because there could be cases where teams don't want to do this (MySQL being one of them since you can add columns relative to existing columns).
Additional information
If this approach is accepted I think making a config option to set this would be a good idea and it could be useful to make enabling this the default depending on the database you ere using (ex enabled by default for postgresql and sqlite3, not for mysql).
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]