Skip to content
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

ActiveRecord::Persistence#upsert_all throws PG::CardinalityViolation: ERROR: ON CONFLICT DO UPDATE command cannot affect row a second time #35519

Closed
vishaltelangre opened this issue Mar 7, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@vishaltelangre
Copy link
Contributor

commented Mar 7, 2019

Steps to reproduce

  1. Create upsert_all_issue database on PostgreSQL using createdb upsert_all_issue command.
  2. Use the following script to reproduce the issue.
# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails"
  gem "pg"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "upsert_all_issue", username: "postgres", password: "")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.string :title, null: false
    t.string :slug, null: false
    t.string :author, null: false

    t.index :slug, unique: true
  end
end

class Article < ActiveRecord::Base; end

class BugTest < Minitest::Test
  def test_upsert_all
    Article.upsert_all(
      [
        {title: "Article 1", slug: "article", author: "Jane"},
        {title: "Article 2", slug: "article", author: "John"}
      ],
      unique_by: { columns: %w[ slug ] }
    )

    assert_equal("Article 2", Article.first.title)
  end
end

Expected behavior

As per #35077, Article.upsert_all should insert just only one article record with title Article 2 instead of inserting two separate records since the slug attribute in both of the attribute hashes is same (or duplicate).

Actual behavior

The upsert_all query

Article.upsert_all(
  [
    {title: "Article 1", slug: "article", author: "Jane"},
    {title: "Article 2", slug: "article", author: "John"}
  ], 
  unique_by: { columns: %w[ slug ] }
)

panics with the following exception.

  Bulk Insert (4.9ms)  INSERT INTO "articles"("title","slug","author") VALUES ('Article 1', 'article', 'Jane'), ('Article 2', 'article', 'John') ON CONFLICT ("slug") DO UPDATE SET "title"=excluded."title","author"=excluded."author" RETURNING "id"
Traceback (most recent call last):
        1: from (irb):1
ActiveRecord::StatementInvalid (PG::CardinalityViolation: ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time)
HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.

System configuration

Rails version:
rails 6.0.0.beta2 (at master@a62683f)

Ruby version:
2.6.0

PostgreSQL version:
10.5

@vishaltelangre

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

The same script (below one) with SQLite3 works just fine. So it seems that the issue is only related to PostgreSQL.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: "upsert_all_issue")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.string :title, null: false
    t.string :slug, null: false
    t.string :author, null: false

    t.index :slug, unique: true
  end
end

class Article < ActiveRecord::Base; end

class BugTest < Minitest::Test
  def test_upsert_all
    Article.upsert_all(
      [
        {title: "Article 1", slug: "article", author: "Jane"},
        {title: "Article 2", slug: "article", author: "John"}
      ],
      unique_by: { columns: %w[ slug ] }
    )

    assert_equal("Article 2", Article.first.title)
    assert_equal(1, Article.count)
  end
end
Output of the above script
[...]

Finished in 0.012828s, 77.9545 runs/s, 155.9090 assertions/s.
1 runs, 2 assertions, 0 failures, 0 errors, 0 skips
@boblail

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Did you notice how it works on Postgres if you split up the commands?

Article.insert(
  { title: "Article 1", slug: "article", author: "Jane" }
)
Article.upsert(
  { title: "Article 2", slug: "article", author: "John" },
  unique_by: { columns: %w[ slug ] }
)

Postgres deliberately takes the position that

  1. It should not update a row twice in one command (i.e. inserting it and updating it)
  2. Users can and should de-dupe input to the INSERT command

There's a good comment in Postgres' source where the exception is raised, explaining the database's rationale.

@vishaltelangre

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@boblail Thank you for sharing the link to the Postgres' source. It is really helpful.

I reported this issue in the first place since I couldn't get the #upsert_all working by following the documentation of ActiveRecord::Persistence#upsert_all here -

# # Insert multiple records, performing an upsert when records have duplicate ISBNs
# # ('Eloquent Ruby' will overwrite 'Rework' because its ISBN is duplicate)
# Book.upsert_all([
# { title: 'Rework', author: 'David', isbn: '1' },
# { title: 'Eloquent Ruby', author: 'Russ', isbn: '1' }
# ],
# unique_by: { columns: %w[ isbn ] })
.

The specified example in the documentation for upserting two book values in a single query with duplicate isbn doesn't work. The script here (https://gist.github.com/vishaltelangre/ba7ad59509f1140631ca45e6d7d48595) uses the book example to reproduce the same issue.

Do you think that we should update the documentation accordingly so that people will not run into the same issue?

@boblail

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@vishaltelangre, ah! Thank you so much for pointing that out! 🤦‍♂️ I'll create a PR to adjust the docs.

@boblail

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

For the record, I did the script with MySQL (can't use unique_by with MySQL, but it's not strictly necessary in this test) and the tests passed:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails"
  gem "mysql2"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "mysql2", database: "activerecord_unittest")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.string :title, null: false
    t.string :slug, null: false
    t.string :author, null: false

    t.index :slug, unique: true
  end
end

class Article < ActiveRecord::Base; end

class BugTest < Minitest::Test
  def test_upsert_all
    Article.upsert_all(
      [
        {title: "Article 1", slug: "article", author: "Jane"},
        {title: "Article 2", slug: "article", author: "John"}
      ]
    )

    assert_equal("Article 2", Article.first.title)
  end
end
@boblail

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

🤔 So of the three, only Postgres seems to want to avoid this double-update scenario; but I think it's OK for lower-level methods like this to introduces fewer layers of padding between you and your database — i.e. OK for Postgres users experience this exception.

I definitely agree that the examples in the docs should just work out of the box!

boblail added a commit to boblail/rails that referenced this issue Mar 8, 2019

Update documentation on upsert_all so that it is correct for Postgres
Details in rails#35519

In short, MySQL and Sqlite3 allow a record to be both inserted _and_ replaced in the same operation. Postgres (and the SQL-2003 rules for MERGE) do not.

Postgres's rationale seems to be that the operation would be nondeterministic.

I think it's OK for Rails users to have a different experience with this feature depending on their database; but I think you should be able to follow the examples in the docs on any database.
@kamipo

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

the docs is fixed by #35531.

@kamipo kamipo closed this Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.