PostgreSQL, remove varchar limit. #14579

Merged
merged 2 commits into from Apr 4, 2014

Conversation

Projects
None yet
8 participants
@senny
Member

senny commented Apr 3, 2014

Closes #13435
Closes #9153

There is no reason for the PG adapter to have a default limit of 255 on :string
columns. See this snippet from the PG docs:

Tip: There is no performance difference among these three types, apart
from increased storage space when using the blank-padded type, and a
few extra CPU cycles to check the length when storing into a
length-constrained column. While character(n) has performance
advantages in some other database systems, there is no such advantage
in PostgreSQL; in fact character(n) is usually the slowest of the
three because of its additional storage costs. In most situations text
or character varying should be used instead.
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Apr 3, 2014

Member

@matthewd @rafaelfranca @tenderlove @rafaelfranca let me know if there are any objections.

Member

senny commented Apr 3, 2014

@matthewd @rafaelfranca @tenderlove @rafaelfranca let me know if there are any objections.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Apr 3, 2014

Member

I think the reflection test should be looking at a column that has an explicit limit: it sounds like it's making an assertion that we can successfully read the limit, not about what the adapter's default is.

Member

matthewd commented Apr 3, 2014

I think the reflection test should be looking at a column that has an explicit limit: it sounds like it's making an assertion that we can successfully read the limit, not about what the adapter's default is.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 3, 2014

Member

Agree with @matthewd

Member

rafaelfranca commented Apr 3, 2014

Agree with @matthewd

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Apr 4, 2014

Member

UPDATE:

  • reflection test no verifies a defined limit (same for all adapters) as suggested by @matthewd .
  • remove the limit for SQLite as well.
  • test case to illustrate the behavior from this comment. Removing the limit does fix it, so PG and SQLite are working but MySQL still shows wrong behavior. A fix is out of scope for this PR.
Member

senny commented Apr 4, 2014

UPDATE:

  • reflection test no verifies a defined limit (same for all adapters) as suggested by @matthewd .
  • remove the limit for SQLite as well.
  • test case to illustrate the behavior from this comment. Removing the limit does fix it, so PG and SQLite are working but MySQL still shows wrong behavior. A fix is out of scope for this PR.

senny added some commits Apr 3, 2014

PostgreSQL and SQLite, remove varchar limit. [Vladimir Sazhin & Toms …
…Mikoss & Yves Senn]

There is no reason for the PG adapter to have a default limit of 255 on :string
columns. See this snippet from the PG docs:

    Tip: There is no performance difference among these three types, apart
    from increased storage space when using the blank-padded type, and a
    few extra CPU cycles to check the length when storing into a
    length-constrained column. While character(n) has performance
    advantages in some other database systems, there is no such advantage
    in PostgreSQL; in fact character(n) is usually the slowest of the
    three because of its additional storage costs. In most situations text
    or character varying should be used instead.
test, show current adapter behavior for `add_column limit: nil`.
This is an illustration of #13435 (comment)
Removing the limit from the PG and SQLite adapter solves the issue.
MySQL is still affected by the issue.

rafaelfranca added a commit that referenced this pull request Apr 4, 2014

Merge pull request #14579 from senny/pg/remove_string_limit
PostgreSQL, remove varchar limit.

Conflicts:
	activerecord/CHANGELOG.md

@rafaelfranca rafaelfranca merged commit 80f4a65 into rails:master Apr 4, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@Envek

This comment has been minimized.

Show comment
Hide comment
@Envek

Envek Apr 5, 2014

Contributor

Whoohoo! We've waiting this for such a long time!

Is there any chance to get this in the 4.1 release?
Is there any chances to get it backported to 4.0 and 3.2?

Contributor

Envek commented Apr 5, 2014

Whoohoo! We've waiting this for such a long time!

Is there any chance to get this in the 4.1 release?
Is there any chances to get it backported to 4.0 and 3.2?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Apr 5, 2014

Member

@Envek this patch changes the behavior so we can't backport to 4-0-stable. With 4.1.0 we are currently in late RC and only regression fixes and blockers can make it in. This will have to wait for 4.2.0.

Member

senny commented Apr 5, 2014

@Envek this patch changes the behavior so we can't backport to 4-0-stable. With 4.1.0 we are currently in late RC and only regression fixes and blockers can make it in. This will have to wait for 4.2.0.

@yahonda

This comment has been minimized.

Show comment
Hide comment
@yahonda

yahonda Apr 7, 2014

Contributor

Thanks for creating a test for #13435. I've got to know Oracle enhanced adapter also needs #13435 support.
but it cannot be simply removing limit: 255 since Oracle VARCHAR2 datatype for Rails :string needs needs the length specified.

(5.6ms) CREATE TABLE "ACCOUNTS" ("ID" NUMBER(38) NOT NULL PRIMARY KEY, "FIRM_ID" NUMBER(38), "FIRM_NAME" VARCHAR2, "CREDIT_LIMIT" NUMBER(38))
OCIError: ORA-00906: missing left parenthesis: CREATE TABLE "ACCOUNTS" ("ID" NUMBER(38) NOT NULL PRIMARY KEY, "FIRM_ID" NUMBER(38), "FIRM_NAME" VARCHAR2, "CREDIT_LIMIT" NUMBER(38))

If I understand the reason mysql/mysql2 does not support this yet is MySQL varchar datatype also needs length specified.

   (1.7ms)  CREATE TABLE `accounts` (`id` int(11) auto_increment PRIMARY KEY, `firm_id` int(11), `firm_name` varchar, `credit_limit` int(11)) ENGINE=InnoDB
Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ' `credit_limit` int(11)) ENGINE=InnoDB' at line 1: CREATE TABLE `accounts` (`id` int(11) auto_increment PRIMARY KEY, `firm_id` int(11), `firm_name` varchar, `credit_limit` int(11)) ENGINE=InnoDB

I'm thinking about how to implement this as follows but I'm not sure if it is a good idea or not.

  • limit: nil sets the max length of database datatype, such as 65535 for MySQL varchar, 4000 or 32767 for Oracle.
Contributor

yahonda commented on 80f4a65 Apr 7, 2014

Thanks for creating a test for #13435. I've got to know Oracle enhanced adapter also needs #13435 support.
but it cannot be simply removing limit: 255 since Oracle VARCHAR2 datatype for Rails :string needs needs the length specified.

(5.6ms) CREATE TABLE "ACCOUNTS" ("ID" NUMBER(38) NOT NULL PRIMARY KEY, "FIRM_ID" NUMBER(38), "FIRM_NAME" VARCHAR2, "CREDIT_LIMIT" NUMBER(38))
OCIError: ORA-00906: missing left parenthesis: CREATE TABLE "ACCOUNTS" ("ID" NUMBER(38) NOT NULL PRIMARY KEY, "FIRM_ID" NUMBER(38), "FIRM_NAME" VARCHAR2, "CREDIT_LIMIT" NUMBER(38))

If I understand the reason mysql/mysql2 does not support this yet is MySQL varchar datatype also needs length specified.

   (1.7ms)  CREATE TABLE `accounts` (`id` int(11) auto_increment PRIMARY KEY, `firm_id` int(11), `firm_name` varchar, `credit_limit` int(11)) ENGINE=InnoDB
Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ' `credit_limit` int(11)) ENGINE=InnoDB' at line 1: CREATE TABLE `accounts` (`id` int(11) auto_increment PRIMARY KEY, `firm_id` int(11), `firm_name` varchar, `credit_limit` int(11)) ENGINE=InnoDB

I'm thinking about how to implement this as follows but I'm not sure if it is a good idea or not.

  • limit: nil sets the max length of database datatype, such as 65535 for MySQL varchar, 4000 or 32767 for Oracle.

This comment has been minimized.

Show comment
Hide comment
@senny

senny Apr 7, 2014

Member

@yahonda as far as I know MySQL doesn't need it but there are / were concerns when it's not specified. The reason why it's not supported at the moment is a bug 😓 . limit: nil will be merged with the columns default, which simply replaces it by 255. It's on my ToDo list and I'll look into fixing that.

If Oracle requires a limit that code should first live in the Oracle adapter. None of the core adapters do require the :limit to be specified at all times.

Hope that helps. Let me know if you have further questions / suggestions and thanks for the heads up! 💛

Member

senny replied Apr 7, 2014

@yahonda as far as I know MySQL doesn't need it but there are / were concerns when it's not specified. The reason why it's not supported at the moment is a bug 😓 . limit: nil will be merged with the columns default, which simply replaces it by 255. It's on my ToDo list and I'll look into fixing that.

If Oracle requires a limit that code should first live in the Oracle adapter. None of the core adapters do require the :limit to be specified at all times.

Hope that helps. Let me know if you have further questions / suggestions and thanks for the heads up! 💛

This comment has been minimized.

Show comment
Hide comment
@yahonda

yahonda Apr 7, 2014

Contributor

@senny Thanks for the reply. I was not aware that MySQL itself does not need the length for varchar.
I'm going to open another issue/pull request in Oracle enhanced adapter group and will hear from users and developers.

Contributor

yahonda replied Apr 7, 2014

@senny Thanks for the reply. I was not aware that MySQL itself does not need the length for varchar.
I'm going to open another issue/pull request in Oracle enhanced adapter group and will hear from users and developers.

@tenderlove tenderlove referenced this pull request in ManageIQ/manageiq Feb 18, 2015

Closed

limit description length to 255 #1775

@lukewendling

This comment has been minimized.

Show comment
Hide comment
@lukewendling

lukewendling Feb 24, 2015

Contributor

Just ran my first migration after 4.2 upgrade and rake db:schema:dump now adds limit: 255 to all string cols in db even though I'm using postgres. Is there an option to prevent this behavior?

Contributor

lukewendling commented Feb 24, 2015

Just ran my first migration after 4.2 upgrade and rake db:schema:dump now adds limit: 255 to all string cols in db even though I'm using postgres. Is there an option to prevent this behavior?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 25, 2015

Member

@lukewendling the db/schema.rb is used to exactly recreate your database. All your string columns were added before 4.2 with an implicit limit of 255. At that time it was not necessary to dump the limit to db/schema.rb because it was the default. Now that we've changed the default to no limit. We must dump these limits to recreate the database.

Member

senny commented Feb 25, 2015

@lukewendling the db/schema.rb is used to exactly recreate your database. All your string columns were added before 4.2 with an implicit limit of 255. At that time it was not necessary to dump the limit to db/schema.rb because it was the default. Now that we've changed the default to no limit. We must dump these limits to recreate the database.

@constantm

This comment has been minimized.

Show comment
Hide comment
@constantm

constantm Feb 25, 2015

I have a similar problem to @lukewendling. We have two dev boxes both running Rails 4.2, but as soon as I create a migration and it gets run on the other box, it adds limits to all the string fields in the schema file. When a migration is created on the other dev box, it does not do this.

I have a similar problem to @lukewendling. We have two dev boxes both running Rails 4.2, but as soon as I create a migration and it gets run on the other box, it adds limits to all the string fields in the schema file. When a migration is created on the other dev box, it does not do this.

@Envek

This comment has been minimized.

Show comment
Hide comment
@Envek

Envek Feb 25, 2015

Contributor

@constantm it means that databases in these boxes are become different: one does have limits and another does not (for example someone have run migrations from scratch on another box after upgrade).

You should stick to the configuration you have in production. Dump database schema from production database, restore it on one of dev boxes, run rake db:schema:dump and commit db/schema.rb into version control system.

If you do not want to have limits on string columns, create migration that will remove them:

class RemoveLimitFromStringColumns < ActiveRecord::Migration
  def up
    # repeat for all your tables and all your columns
    execute 'ALTER TABLE "your_table" ALTER COLUMN "your_column" TYPE character varying;'
  end

  def down
    # repeat for all your tables and all your columns
    execute 'ALTER TABLE "your_table" ALTER COLUMN "your_column" TYPE character varying(255);'
  end
end

If you do not have production yet, just drop database and rerun all migrations from scratch.

Anyway, current behaviour is correct, and previous Rails versions were buggy by creating columns with implicit limit and not displaying it in db/schema.rb.

Contributor

Envek commented Feb 25, 2015

@constantm it means that databases in these boxes are become different: one does have limits and another does not (for example someone have run migrations from scratch on another box after upgrade).

You should stick to the configuration you have in production. Dump database schema from production database, restore it on one of dev boxes, run rake db:schema:dump and commit db/schema.rb into version control system.

If you do not want to have limits on string columns, create migration that will remove them:

class RemoveLimitFromStringColumns < ActiveRecord::Migration
  def up
    # repeat for all your tables and all your columns
    execute 'ALTER TABLE "your_table" ALTER COLUMN "your_column" TYPE character varying;'
  end

  def down
    # repeat for all your tables and all your columns
    execute 'ALTER TABLE "your_table" ALTER COLUMN "your_column" TYPE character varying(255);'
  end
end

If you do not have production yet, just drop database and rerun all migrations from scratch.

Anyway, current behaviour is correct, and previous Rails versions were buggy by creating columns with implicit limit and not displaying it in db/schema.rb.

@constantm

This comment has been minimized.

Show comment
Hide comment
@constantm

constantm Feb 25, 2015

@Envek Okay great thanks, I'll dump the schema from production then.

@Envek Okay great thanks, I'll dump the schema from production then.

@arthurnn arthurnn deleted the senny:pg/remove_string_limit branch Feb 25, 2015

@fphilipe

This comment has been minimized.

Show comment
Hide comment
@fphilipe

fphilipe Jun 12, 2015

Contributor

Out of curiosity, why did you go with character varying and not text?

Contributor

fphilipe commented Jun 12, 2015

Out of curiosity, why did you go with character varying and not text?

@Envek

This comment has been minimized.

Show comment
Hide comment
@Envek

Envek Jun 12, 2015

Contributor

@fphilipe It's kind of convention to use character varying columns for single line values and text for multiline. For example libraries like simple_form are rendering input tags for character varying type attributes and textarea tags for text type values. It's quite convenient.

Contributor

Envek commented Jun 12, 2015

@fphilipe It's kind of convention to use character varying columns for single line values and text for multiline. For example libraries like simple_form are rendering input tags for character varying type attributes and textarea tags for text type values. It's quite convenient.

@fphilipe

This comment has been minimized.

Show comment
Hide comment
@fphilipe

fphilipe Jun 12, 2015

Contributor

@Envek Makes perfect sense! Thanks for the explanation 👍

Contributor

fphilipe commented Jun 12, 2015

@Envek Makes perfect sense! Thanks for the explanation 👍

@cmc333333 cmc333333 referenced this pull request in 18F/C2 Jun 24, 2015

Merged

upgrade to Rails 4.2 #382

lilliealbert added a commit to doubleunion/arooo that referenced this pull request Feb 6, 2016

Remove the 255 character limit from all the string columns
 - Rails 4.2 no longer defaults strings to 255 characters long.
 - This change make it so that a fresh DB and an old DB should have the same schema dumped when running rake db:migrate!
 - Rails commit that created that change: rails/rails#14579

@kkelleey kkelleey referenced this pull request in ifmeorg/ifme Jul 4, 2016

Merged

Adding latest version of schema #233

@Gargron Gargron referenced this pull request in tootsuite/mastodon May 21, 2018

Open

ActiveRecord::StatementInvalid errors on 2.4.0rc4 #7559

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment