Postgresql doesn't accepts limits on text columns #8276
Conversation
| create_table :limitless_fields, force: true do |t| | ||
| t.binary :binary, limit: 100_000 | ||
| t.text :text, limit: 100_000 | ||
| end |
There was a problem hiding this comment.
I don't see this table being used anywhere else, is it really necessary?
There was a problem hiding this comment.
Sorry I missed the detail about this new table in your comments.
|
It's going to need a changelog entry. |
|
Seems good. We need the CHANGELOG entry |
|
@carlosantoniodasilva Sorry for being confusing! Should I add a comment saying what the table is for? @rafaelfranca I was hoping to backport this to 3-2-stable, like I did for #6328 / #6418. Should I add the entry to the CHANGELOG there? I'm sorry if I'm doing this wrong by the way, I thought bug fixes should first be applied to master and then backported, but I'm not sure how well that works for the CHANGELOG. Thank you for your guidance! |
|
We are adding CHANGELOG entries for all the branches that we apply the patch. So bug fixes are first in master and then backported, but the CHANGELOG entry need to be added to master too. |
|
@rafaelfranca Thank you very much for explaining! Should my CHANGELOG entry describe the :limit changes to both :binary and :text then? |
|
Yes |
Postgresql doesn't accepts limits on text columns
|
@rafaelfranca wow that was fast. Thank you so much! 👍 👍 👍 |
|
@pwnall no need for comments now :), thanks. |
Postgresql doesn't accepts limits on text columns Conflicts: activerecord/CHANGELOG.md activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
TL;DR similar to accepted pull request #6328, but this time targeting text columns
#6238
The long story: PostgreSQL doesn't take a limit for "text" columns so developers must choose between removing :limit from their binary column definitions, and not being able to target pgsql.
This change drops :limit specifications, when migrations are applied against a pgsql database. It's not ideal, because the limit information is lost. However, the mysql adapters do something similar, so I hope the approach is OK.
A better long-term solution might be to map :text columns without :limit to "text", and :text columns with :limit to "character varying(xxx)". However, that requires changes in the code that maps postgresql types back into Rails types, and this change seems like a smaller step in the same direction.
Testing: I added a table to postgresql_specific_schema that uses :limit on :text and :binary columns. Setting up this table fails without the patch in this pull request. I also added a couple of unit tests to a new sql_types_test file, which follows the convention in the MySQL adapter tests.
Please let me know if this patch needs more work, and thank you very much for Rails!