Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Postgresql doesn't accepts limits on text columns #8276

Merged
merged 1 commit into from

3 participants

Victor Costan Carlos Antonio da Silva Rafael Mendonça França
Victor Costan

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!

Carlos Antonio da Silva carlosantoniodasilva commented on the diff
activerecord/test/schema/postgresql_specific_schema.rb
@@ -192,5 +192,10 @@
_SQL
rescue #This version of PostgreSQL either has no XML support or is was not compiled with XML support: skipping table
end
+
+ create_table :limitless_fields, force: true do |t|
+ t.binary :binary, limit: 100_000
+ t.text :text, limit: 100_000
+ end

I don't see this table being used anywhere else, is it really necessary?

Sorry I missed the detail about this new table in your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carlos Antonio da Silva

It's going to need a changelog entry.

/cc @rafaelfranca @tenderlove

Rafael Mendonça França

Seems good. We need the CHANGELOG entry

Victor Costan

@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!

Rafael Mendonça França

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.

Victor Costan

@rafaelfranca Thank you very much for explaining! Should my CHANGELOG entry describe the :limit changes to both :binary and :text then?

Rafael Mendonça França

Yes

Rafael Mendonça França rafaelfranca merged commit 3a89068 into from
Victor Costan

@rafaelfranca wow that was fast. Thank you so much! :+1: :+1: :+1:

Carlos Antonio da Silva

@pwnall no need for comments now :), thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 20, 2012
  1. Victor Costan
This page is out of date. Refresh to see the latest.
5 activerecord/CHANGELOG.md
View
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* When running migrations on Postgresql, the `:limit` option for `binary` and `text` columns is silently dropped.
+ Previously, these migrations caused sql exceptions, because Postgresql doesn't support limits on these types.
+
+ *Victor Costan*
+
* Don't change STI type when calling `ActiveRecord::Base#becomes`.
Add `ActiveRecord::Base#becomes!` with the previous behavior.
7 activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
View
@@ -396,6 +396,13 @@ def type_to_sql(type, limit = nil, precision = nil, scale = nil)
when nil, 0..0x3fffffff; super(type)
else raise(ActiveRecordError, "No binary type has byte size #{limit}.")
end
+ when 'text'
+ # PostgreSQL doesn't support limits on text columns.
+ # The hard limit is 1Gb, according to section 8.3 in the manual.
+ case limit
+ when nil, 0..0x3fffffff; super(type)
+ else raise(ActiveRecordError, "The limit on text can be at most 1GB - 1byte.")
+ end
when 'integer'
return 'integer' unless limit
18 activerecord/test/cases/adapters/postgresql/sql_types_test.rb
View
@@ -0,0 +1,18 @@
+require "cases/helper"
+
+class SqlTypesTest < ActiveRecord::TestCase
+ def test_binary_types
+ assert_equal 'bytea', type_to_sql(:binary, 100_000)
+ assert_raise ActiveRecord::ActiveRecordError do
+ type_to_sql :binary, 4294967295
+ end
+ assert_equal 'text', type_to_sql(:text, 100_000)
+ assert_raise ActiveRecord::ActiveRecordError do
+ type_to_sql :text, 4294967295
+ end
+ end
+
+ def type_to_sql(*args)
+ ActiveRecord::Base.connection.type_to_sql(*args)
+ end
+end
5 activerecord/test/schema/postgresql_specific_schema.rb
View
@@ -192,5 +192,10 @@
_SQL
rescue #This version of PostgreSQL either has no XML support or is was not compiled with XML support: skipping table
end
+
+ create_table :limitless_fields, force: true do |t|
+ t.binary :binary, limit: 100_000
+ t.text :text, limit: 100_000
+ end

I don't see this table being used anywhere else, is it really necessary?

Sorry I missed the detail about this new table in your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
Something went wrong with that request. Please try again.