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

Don't skip some columns in column_types on Postgres #42933

Merged
merged 1 commit into from Sep 16, 2022

Conversation

ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Aug 2, 2021

Fixes #41651, by partially reverting #39097. I just reverted the line that @terracatta highlighted as problematic.

I ran the same benchmark as #39097 and it seems like this change does not cause a perf regression.

require "bundler/inline"

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

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

  gem "rails", path: "/Users/alex/code/rails" # github: "rails/rails", branch: "main"
  gem "sqlite3"
  gem "benchmark-ips"
end

require "active_record"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table :active_storage_blobs do |t|
    t.string   :key,          null: false
    t.string   :filename,     null: false
    t.string   :content_type
    t.text     :metadata
    t.string   :service_name, null: false
    t.bigint   :byte_size,    null: false
    t.string   :checksum,     null: false
    t.datetime :created_at,   null: false

    t.index [ :key ], unique: true
  end
end

class ActiveStorageBlob < ActiveRecord::Base
end

Benchmark.ips do |x|
  x.report("find_by") { ActiveStorageBlob.find_by(id: 1) }
end

This branch:

Warming up --------------------------------------
             find_by     1.940k i/100ms
Calculating -------------------------------------
             find_by     17.928k (± 4.8%) i/s -     91.180k in   5.098301s

Main:

Warming up --------------------------------------
             find_by     1.912k i/100ms
Calculating -------------------------------------
             find_by     17.961k (± 4.8%) i/s -     89.864k in   5.015252s

So it's basically the same. cc @terracatta @boblail

expected = if current_adapter?(:PostgreSQLAdapter)
# Postgres returns the same name for each column in the given query, so each column is named "coalesce"
# As a result Rails cannot accurately type cast each value.
# To work around this, you should use aliases in your select statement (see test_pluck_functions_with_alias).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit yucky, but it only happens in very specific cases: if you have select two (or more) columns that don't have an alias and use the same SQL function (eg COALESCE). This happens because of how Postgres returns column names, see #36186 (comment) for a summary.

As far as I can tell we have a few options:

In my opinion this PR is the least bad option.

Fixes rails#41651, by partially reverting rails#39097

I ran the same benchmark as rails#39097 and it seems like this change does not cause a perf regression.

```ruby

require "bundler/inline"

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

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

  gem "rails", path: "/Users/alex/code/rails" # github: "rails/rails", branch: "main"
  gem "sqlite3"
  gem "benchmark-ips"
end

require "active_record"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table :active_storage_blobs do |t|
    t.string   :key,          null: false
    t.string   :filename,     null: false
    t.string   :content_type
    t.text     :metadata
    t.string   :service_name, null: false
    t.bigint   :byte_size,    null: false
    t.string   :checksum,     null: false
    t.datetime :created_at,   null: false

    t.index [ :key ], unique: true
  end
end

class ActiveStorageBlob < ActiveRecord::Base
end

Benchmark.ips do |x|
  x.report("find_by") { ActiveStorageBlob.find_by(id: 1) }
end
```

This branch:

```
Warming up --------------------------------------
             find_by     1.940k i/100ms
Calculating -------------------------------------
             find_by     17.928k (± 4.8%) i/s -     91.180k in   5.098301s
```

Main:

```
Warming up --------------------------------------
             find_by     1.912k i/100ms
Calculating -------------------------------------
             find_by     17.961k (± 4.8%) i/s -     89.864k in   5.015252s
```
@rails-bot
Copy link

rails-bot bot commented Dec 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 14, 2021
@rails-bot rails-bot bot closed this Dec 21, 2021
@rafaelfranca rafaelfranca reopened this Sep 16, 2022
@rails-bot rails-bot bot removed the stale label Sep 16, 2022
@rafaelfranca rafaelfranca merged commit 26ceb7f into rails:main Sep 16, 2022
rafaelfranca added a commit that referenced this pull request Sep 16, 2022
Don't skip some columns in `column_types` on Postgres
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActiveRecord::Result#column_types is often missing data
2 participants