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

Override the max_index_name_size to keep local shortening algorithm. (8) #2373

Merged
merged 13 commits into from
Aug 13, 2024

Conversation

andynu
Copy link
Contributor

@andynu andynu commented Mar 25, 2024

Rails now defaults to its own index name shortening (limited to 62 bytes).
This change overrides the rails max to exceed this gems shortening
method in order to preserve the historical behavior.
names

See rails/rails@3682aa1

So the shortening of index names that was added to rails itself
was almost immediately changed to use an 'idx_' prefix instead of an 'ix_'
prefix.

See rails/rails@59f2b06

However, now that rails does shortening. It is an opportunity to
consider simplifying this library and just using that.

Stacked on #2374

Introduce the _connection method which returns whichever is defined
between @unconfigured_connection or @raw_connection.
verify! ensures that @raw_connection is defined.
Rails commit bd19d1baf1 [1] added a check of logger.level which was not
supported by the MockLogger.

This commit adds a mock level method. And limits the missing_method to
respond only to logging calls, so any other calls will raise a
NoMethodError to make future debugging easier.

[1]
rails/rails@bd19d1b
The SchemaMigration object no longer inherits from ActiveRecord::Base.

See "Move SchemaMigration to an independent object"
rails/rails@436277d

Several methods were renamed, and are no longer available as class
methods on SchemaMigration, but as an instance of SchemaMigration
per connection.

The object per connection seems to remove the need to `reset_table_name`
in the tests. That method is not available on the new object.
@andynu andynu changed the title Fix test of index names (now limited to 62 bytes by rails) Fix test of index names (now limited to 62 bytes by rails) (7/10) Mar 25, 2024
@andynu andynu force-pushed the stack-06-index-name-shortening branch 5 times, most recently from 5465ea7 to 236b8a2 Compare March 25, 2024 19:56
@andynu andynu changed the title Fix test of index names (now limited to 62 bytes by rails) (7/10) Fix test of index names (now limited to 62 bytes by rails) (8/10) Mar 25, 2024
@andynu andynu force-pushed the stack-06-index-name-shortening branch from 236b8a2 to 131e39b Compare March 25, 2024 21:39
@andynu andynu changed the title Fix test of index names (now limited to 62 bytes by rails) (8/10) Fix test of index names (now limited to 62 bytes by rails) (8) Mar 26, 2024
@davinlagerroos
Copy link
Contributor

Something to consider: it appears oracle-enhanced has a practice of overriding these kinds of defaults. Following that pattern still gets spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:330 to pass and avoids the possibility of causing upgrade problems for someone who might have an existing schema that contains an index that is longer than 62 bytes.

Rails commit e6da3ebd6c6 [1] introduces ColumnDefinition validations for
the keywords used in database commands. This commit adds the additional
keywords supported by this gem.

[1] rails/rails@e6da3eb
@andynu andynu force-pushed the stack-06-index-name-shortening branch from 131e39b to 7b36a5c Compare April 1, 2024 15:14
See rails/rails@07d2407

Both `table_name` and `column_name` were removed in favor of doing
o.left in the visitor pattern. This is just mimicing the adjustments
from upstream.
@andynu andynu force-pushed the stack-06-index-name-shortening branch from 7b36a5c to 131e39b Compare April 1, 2024 15:57
This extends the existing class level auto_retry functionality to
optionally allow an allow_retry boolean kwarg to allow retry of specific
queries.

This PR preserves the requirement that `autocommit?` be enabled which is the
safer choice.

```
  def with_retry(allow_retry: false) # :nodoc:
    should_retry = (allow_retry || self.class.auto_retry?) && autocommit?
```

See issue rsim#2310
@andynu andynu force-pushed the stack-06-index-name-shortening branch from 131e39b to 8df3f95 Compare April 1, 2024 16:12
@andynu
Copy link
Contributor Author

andynu commented Apr 1, 2024

I like this idea, but just overriding the max only works for newer oracle versions and isn't sufficient for oracle 11.
7b36a5c

@davinlagerroos
Copy link
Contributor

Thanks for considering my suggestion! I think approach of aliasing the method still works with oracle 11. The method that would be aliased, max_identifier_length calls suports_long_identifier? which includes an oracle version check. For older versions of oracle suports_long_identifier? would return false and max_identifier_length would return 30.

It looks like in the test that breaks, the else branch of the test reflects this - when we are not testing with @oracle12cr2_or_higher the test's expectation is that the long index gets truncated to 30 characters.

@andynu
Copy link
Contributor Author

andynu commented Apr 3, 2024

@davinlagerroos I believe I tried what you suggest here 7b36a5c
But it didn't bear out in the tests: https://github.com/rsim/oracle-enhanced/actions/runs/8509850693/job/23306215927

I would welcome a review. Thanks!

@davinlagerroos
Copy link
Contributor

@andynu My bad, I missed the test output! Thanks for making it more obvious. That is definitely not a good outcome.

hmm, when index_name calls super, activerecord uses the max_index_name_size to see if the name is too long and needs to be shortened. The change I suggested means that for oracle < 12.2, this is 30 and super returns a name shortened using activerecord's strategy.

However, oracle enhanced looks to be expecting to get the full activerecord index_name so it can shorten the name using its own strategy (where needed).

Would it work to define max_index_name_size as 128? Even though that is too long for oracle < 12.2, it should mean the call to super would return the full name and then the oracle enhanced could shorten it the way it wants to get to 30.

Or you can follow your original plan. That works too, it just might cause an issue if people have an index name that is longer than the new activerecord default. OTOH I do not know how many people would have an index name that long.

@andynu
Copy link
Contributor Author

andynu commented Apr 4, 2024

@davinlagerroos good idea! That has worked well and does a better job preserving existing behavior. I will rebase that into this PR tomorrow. andynu@6963192

Rails now defaults to its own index name shortening (limited to 62 bytes).
This change overrides the rails max to exceed this gems shortening
method in order to preserve the historical behavior.
names

See rails/rails@3682aa1

So the shortening of index names that was added to rails itself
was almost immediately changed to use an 'idx_' prefix instead of an 'ix_'
prefix.

See rails/rails@59f2b06

However, now that rails does shortening. It is an opportunity to
consider simplifying this library and just using that.
@andynu andynu force-pushed the stack-06-index-name-shortening branch from 8df3f95 to 28d7d9c Compare April 4, 2024 14:03
@andynu andynu changed the title Fix test of index names (now limited to 62 bytes by rails) (8) Override the max_index_name_size to keep local shortening algorithm. (8) Apr 4, 2024
@davinlagerroos
Copy link
Contributor

@andynu Thanks for trying that out! I am glad it worked

@yahonda yahonda merged commit 9917f9d into rsim:master Aug 13, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants