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.

Sign up for GitHub

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

Disable supports_expression_index unconditionally #250

Merged
merged 2 commits into from
May 11, 2022
Merged

Disable supports_expression_index unconditionally #250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d80aa5
Disable supports_expression_index unconditionally
ecwall May 2, 2022
03918de
Prepare v6.1.10 release
ecwall May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## 6.1.10 - 2022-05-06

- Disable supports_expression_index regardless of CockroachDB version until
`ON CONFLICT expression` is supported.

See https://github.com/cockroachdb/cockroach/issues/67893.

## 6.1.9 - 2022-04-26

- Fix bug where duplicate `rowid` columns would be created when loading
Expand Down
5 changes: 4 additions & 1 deletion lib/active_record/connection_adapters/cockroachdb_adapter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,10 @@ def supports_partial_index?
end

def supports_expression_index?
@crdb_version >= 2122
# Expression indexes are partially supported by CockroachDB v21.2,
Copy link
Contributor

Choose a reason for hiding this comment

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

if we do support it partially, then i would argue we should skip the test instead, or add a new condition for ON CONFLICT support and use that to skip the test / anything relevant in the ORM

otherwise, is this is a "regression" in ORM behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to disable supports_insert_conflict_target? instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

we didn't need the public_send stuff right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it is already there for the ones I overrode.

%w[
  supports_savepoints?
  supports_partial_index?
  supports_partitioned_indexes?
  supports_expression_index?
  supports_insert_returning?
  supports_insert_on_duplicate_skip?
  supports_insert_on_duplicate_update?
  supports_insert_conflict_target?
  supports_optimizer_hints?
  supports_datetime_with_precision?
].each do |method_name|
  define_method method_name do
    ActiveRecord::Base.connection.public_send(method_name)
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean it will also disable ON CONFLICT column_name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it looks like there is no way to say we support expression indexes and ON CONFLICT column_name, but not ON CONFLICT expression without modifying activerecord itself.

# but activerecord requires "ON CONFLICT expression" support.
# See https://github.com/cockroachdb/cockroach/issues/67893
false
end

def supports_datetime_with_precision?
Expand Down
2 changes: 1 addition & 1 deletion lib/version.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# frozen_string_literal: true

module ActiveRecord
COCKROACH_DB_ADAPTER_VERSION = "6.1.9"
COCKROACH_DB_ADAPTER_VERSION = "6.1.10"
end