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

Postgres migrations do not respect functional defaults for columns #21627

Closed
kenaniah opened this Issue Sep 14, 2015 · 24 comments

Comments

Projects
None yet
@kenaniah

kenaniah commented Sep 14, 2015

PostgreSQL allows columns to have functional defaults, but Rails does not appear to support this when using the migrations API. The most notorious example of this is outlined below:

CREATE TABLE a ( t1 TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP ); -- works
CREATE TABLE b ( t1 TIMESTAMPTZ DEFAULT NOW() ); -- works
CREATE TABLE c ( t1 TIMESTAMPTZ DEFAULT 'NOW()' ); -- fails and uses a static timestamp

Tables A and B defined above will work as expected, allowing any new rows to have a proper default value that is calculated at the time of insert. Table C, however, will convert the string 'now' to the current time when the table is defined, causing all inserted default values to reflect the static timestamp instead.

When attempting to use the migration API, one might write code such as:

create_table :c do |t|
  t.timestamp default: 'NOW()'
end

This unfortunately yields the broken use-case of table C from the SQL above.

Proposed Solution

To ensure backwards-compatibility, database portability, and coverage of cases not listed here, I propose adding an option such as raw_default that could be used to pass unquoted defaults to the database in the DDL of a migration.

create_table :c do |t|
  t.timestamp raw_default: 'NOW()'
end

Which would yield the following DDL:

CREATE TABLE c ( t1 TIMESTAMPTZ DEFAULT NOW() );
@yawboakye

This comment has been minimized.

Contributor

yawboakye commented Sep 16, 2015

I had this issue too, brought it up on the #RubyOnRails IRC channel. I don't remember well but I think someone talked about a feature coming in Rails 5 that solves our problem. In the meantime I have to write raw SQL in order to add a functional default. But the problem with this solution is that since ActiveRecord's SQL returns only the id after successful insertion the default values generated by Postgres are ignored.

@kenaniah

This comment has been minimized.

kenaniah commented Sep 16, 2015

@yawboakye fortunately that's a non-issue since Postgres will return the entire row if you append RETURNING * onto the end of an INSERT or UPDATE statement. This can be accomplished at the connection adapter level and would be completely transparent from the perspective of userland.

@yawboakye

This comment has been minimized.

Contributor

yawboakye commented Sep 23, 2015

@kenaniah Sure, but it makes sense why the full row shouldn't be returned by default: there could be too much data, and ActiveRecord already knows both default and non-default values. (Or at least it thinks so.) But if the default value is generated by a function in PG then ActiveRecord doesn't know. Turns out that is not enough reason to make RETURNING * the default, and I'm ok with that. I think what we need is another parameter to tell ActiveRecord, that for a particular model, it should use RETURNING * instead.

@kenaniah

This comment has been minimized.

kenaniah commented Sep 23, 2015

@yawboakye in that case, the RETURNING clause could be used to only change the fields that used raw defaults as a targeted solution. I agree that there's no need to return the entire row if we already know the values of the other fields.

@yawboakye

This comment has been minimized.

Contributor

yawboakye commented Sep 23, 2015

@kenaniah 👍

@kamipo

This comment has been minimized.

Member

kamipo commented Oct 11, 2015

Related issue with #20005.

@rails-bot rails-bot added the stale label Jul 13, 2016

@rails-bot

This comment has been minimized.

rails-bot commented Jul 13, 2016

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 5-0-stable branch or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@jhirn

This comment has been minimized.

jhirn commented Jan 25, 2017

Hi. I wanted to +1 on this issue since it's stale but still happens. I'd be happy to talk through expected behavior and take a stab at a PR if it's welcomed. It is a bit confusing and arguably incorrect to have the model instance not reflect the DB record after create or update.

Currently resolving this by doing a reload after_create since it for a default. I could use a pluck but my my model is small enough. Using targeted returning would be ideal for dbs that support it.

I believe an option to set this explicitly in the model is a necessary although many cases can be automatically detected. If there are triggers in play, support for updates might also be nice although it complicates things in all but naive cases.

@maclover7

This comment has been minimized.

Member

maclover7 commented Jan 30, 2017

@jhirn Can you confirm whether the reproduction information in the original issue body works for you? If not, can you create an executable test script using one of the templates here, that would demonstrate the problem you are seeing?

@jhirn

This comment has been minimized.

jhirn commented Feb 15, 2017

Hi @maclover7

Here's a script to demonstrate the lack of default values for any database generated columns being set on the model without an explicit reload.

I'm not 100% sure that was the exact original issue and this might be out of AR's hands, especially for databases that don't support RETURNING. I just wanted to share it's potentially confusing to have a model in memory that doesn't match what the database values are after a save operation. This could also be a trigger on update.

Perhaps something to flag database managed columns and manage defaults or after update triggers? That might go against the philosophy of Rails keeping DB logic within the apps.

I'd be interested to participate in conversation about that might look like, or at least definitively rule it out.

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', github: 'rails/rails'
  gem 'arel', github: 'rails/arel'
  gem 'pg'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.

ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: 'template1')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  execute 'CREATE EXTENSION IF NOT EXISTS "uuid-ossp"'
  execute 'CREATE SEQUENCE IF NOT EXISTS default_power_level START 9000'

  create_table :payments, force: true do |t|
    t.uuid :uuid_default, default: 'uuid_generate_v4()'
    t.uuid :uuid_default_as_proc, default: -> { 'uuid_generate_v4()' }
    t.integer :non_id_sequence, default: -> { "nextval('default_power_level')" }
    t.time :time_as_default, default: -> { 'now()' }
    t.integer :default_literal, default: 8
    t.integer :default_ruby_proc, default: -> { 4 + 4 }
    t.string :default_string_as_proc, default: -> { "nextval('default_power_level')" }
  end
end

class Payment < ActiveRecord::Base
end

class BugTest < Minitest::Test
  extend Minitest::Spec::DSL

  let(:p) { Payment.create }

  def test_default_uuid
    refute_nil p.uuid_default
  end

  def test_default_uuid_as_proc
    refute_nil p.uuid_default_as_proc
  end

  def test_default_from_sequence
    refute_nil p.non_id_sequence
  end

  def test_default_now
    refute_nil p.time_as_default
  end

  def test_default_literal
    assert_equal 8, p.default_literal
  end

  def test_default_ruby_proc
    assert_equal 8, p.default_ruby_proc
  end

  def test_default_string_as_proc
    refute_equal "nextval('default_power_level')", p.default_string_as_proc
    refute_nil p.default_string_as_proc
  end

  def test_db_functions_not_null_after_reload
    p.reload
    refute_nil p.uuid_default
    refute_nil p.uuid_default_as_proc
    refute_nil p.non_id_sequence
    refute_nil p.time_as_default
  end
end

postgres-function-defaults.rb.txt

@januszm

This comment has been minimized.

januszm commented Mar 20, 2017

Also I think that there's a common confusion around the world about using this default: 'uuid_generate_v4()' with MySQL, which doesn't have this function. But still, this is passed to db engine as string, not function call.

ActiveRecord::StatementInvalid: 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 'uuid DEFAULT 'uuid_generate_v4' NOT NULL' at line 1: ALTER TABLE `households` ADD `uuid` uuid DEFAULT 'uuid_generate_v4' NOT NULL

@rails-bot rails-bot closed this Mar 21, 2017

@rails-bot

This comment has been minimized.

rails-bot commented Mar 21, 2017

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 5-0-stable branch or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@AndyKriger

This comment has been minimized.

AndyKriger commented Aug 30, 2017

Still seeing this in ActiveRecord 5.1.3 with Rails 5.1.3/Ruby 2.4.1 and Postgres 9.6.4
The object field that has an enum type with a default String value gets set
The object fields that have uuid_generate_v4() as a default value do get set in the database when the object is saved but not on the object itself (the id does get set though) .
A call to object.reload does populate the fields.

@stepheneb

This comment has been minimized.

Contributor

stepheneb commented Sep 8, 2017

Am having a related problem trying to add timestamps to an existing pg database:

class AddTimeStampsToAnnotationComments < ActiveRecord::Migration[5.1]
  change_table :annotation_comments do |t|
    t.timestamps
  end
end
-- change_table(:annotation_comments)
-- change_table(:annotation_comments)
rails aborted!
ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR:  column "created_at" contains null values
: ALTER TABLE "annotation_comments" ADD "created_at" timestamp NOT NULL
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:3:in `block in <class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:2:in `<class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:1:in `<top (required)>'
bin/rails:4:in `require'
bin/rails:4:in `<main>'
PG::NotNullViolation: ERROR:  column "created_at" contains null values
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:3:in `block in <class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:2:in `<class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:1:in `<top (required)>'
bin/rails:4:in `require'
bin/rails:4:in `<main>'
ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR:  column "created_at" contains null values
: ALTER TABLE "annotation_comments" ADD "created_at" timestamp NOT NULL
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:3:in `block in <class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:2:in `<class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:1:in `<top (required)>'
bin/rails:4:in `require'
bin/rails:4:in `<main>'
PG::NotNullViolation: ERROR:  column "created_at" contains null values
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:3:in `block in <class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:2:in `<class:AddTimeStampsToAnnotationComments>'
/Users/stephen/dev/z/zs/db/migrate/20170908041723_add_time_stamps_to_annotation_comments.rb:1:in `<top (required)>'

@rails-bot rails-bot bot removed the stale label Sep 8, 2017

@stepheneb

This comment has been minimized.

Contributor

stepheneb commented Sep 8, 2017

Had to delete all the data in the table to get the migration to work.

@Soliah

This comment has been minimized.

Soliah commented Oct 28, 2017

This is still happening in Rails 5.1.4 (in my case using pgcrypto's gen_random_uuid() as a default.

Working around by manually .plucking the fields out in an after_create callback.

@asmega

This comment has been minimized.

Contributor

asmega commented Sep 27, 2018

for future reference for anyone who lands here the following works in Rails 5.2.1, although I'm not sure when it was introduced

def change
  enable_extension 'pgcrypto'

  add_column :admins, :api_key, :text, null: false, default: -> { "digest((gen_random_uuid())::text, 'sha256'::text)" }
end
@jhirn

This comment has been minimized.

jhirn commented Sep 27, 2018

@davegson

This comment has been minimized.

davegson commented Oct 15, 2018

The issue still remains in Rails 5.2.1. You need to set an after_create callback to work around it.

I assume @asmega was referencing the feature that you could set functional defaults which is supported since Rails 5 and was already mentioned by @yawboakye

@jrochkind

This comment has been minimized.

Contributor

jrochkind commented Oct 15, 2018

So this is only a problem with postgres? It works with other dbs?

This is very annoying. It is definitely still a problem with Rails 5.2.

You can set a DB function as a default in a migration:

 add_column :whatever, :whatever, default: -> { "some_db_func()"}

And it works such that it is there in the db. But the model does not know about it without a reload. :(

If you can still reproduce this error on the 5-0-stable branch or on master,
please reply with all of the information you have about it in order to keep the issue open.

Can this issue be re-opened? It is still a thing.

@jrochkind

This comment has been minimized.

Contributor

jrochkind commented Oct 15, 2018

@Soliah @davegson Can you share what your after_create callbacks look like? Do you avoid ActiveRecord thinking the column has changes that need to be saved, after you've managed to set the in-memory value to what was actually in the db all along?

@davegson

This comment has been minimized.

davegson commented Oct 16, 2018

@jrochkind my callback simply is: after_create :reload - dirty quick fix.

I'm not sure if this belongs here or in a new issue. The original issue described how a postgres function by default would not be supported. Such as:

add_column :whatever, :whatever, default: -> { "some_db_func()"}

Now it does support this, but not in a clean way: you have to reload newly created AR objects to retrieve the values generated by postgres. This blog post describes the issue wonderfully.

I feel we should open up another Issue describing this problem and only reference to this closed one. If others agree I will do so later today.

davegson added a commit to Safing/stamp.community that referenced this issue Oct 16, 2018

@jrochkind

This comment has been minimized.

Contributor

jrochkind commented Oct 16, 2018

Ah, right @davegson , I missed that, cause I found this issue googling for the current problem. It would be awesome if you created another ticket!

Here's my current work-around -- rather than an after_create hook to reload, I override the particular attribute method to pluck from the db on first access -- only if it's nil and the model is already persisted. That way the extra fetch is only made if a caller asks for the value. And I call the (public? private? Not sure) rails method clear_attribute_change to set the db value without rails thinking it's dirty and needs to be saved.

  # Due to rails bug, we don't immediately have the database-provided value after create. :(
  # If we ask for it and it's empty, go to the db to get it
  # https://github.com/rails/rails/issues/21627
  def friendlier_id(*_)
    in_memory = super

    if !in_memory && persisted? && !@friendlier_id_retrieved
      in_memory = self.class.where(id: id).limit(1).pluck(:friendlier_id).first
      write_attribute(:friendlier_id, in_memory)
      clear_attribute_change(:friendlier_id)
      # just to avoid doing it multiple times if it's still unset in db for some reason
      @friendlier_id_retrieved = true
    end

    in_memory
  end
@davegson

This comment has been minimized.

davegson commented Oct 17, 2018

↑↑↑
I opened a new issue for the bug. So this issue - initially a feature request for functional defaults - can rightfully slumber and stay closed ;)

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