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

Use longer identifier for DatabaseLimits when using Oracle 12.2 or higher #2128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koic
Copy link
Collaborator

@koic koic commented Jan 19, 2021

Follow #1703.

Oracle enhanced adapter supports longer identifier by #1703.
I encountered the following error when using rename_table in Oracle 12c.

New table name 'identifier_of_thirty_bytes_or_more' is too long; the limit is 30 characters

Because IDENTIFIER_MAX_LENGTH constant was fixed to 30 bytes.

This PR will use max_identifier_length method instead of the constant and accept longer identifier (max 128 bytes) when using Oracle 12.2 or higher.

I encountered the error in Rails 6.0. So I'd like to backport to 6.1 and 6.0 stable branches.

@koic koic force-pushed the use_longer_identifier_for_oracle_12c branch 2 times, most recently from 498c5d9 to ce00caa Compare January 19, 2021 06:05
…higher

Follow rsim#1703.

Oracle enhanced adapter supports longer identifier by rsim#1703.
I encountered the following error when using `rename_table` in Oracle 12c.

```console
New table name 'identifier_of_thirty_bytes_or_more' is too long; the limit is 30 characters
```

Because `IDENTIFIER_MAX_LENGTH` constant was fixed to 30 bytes.

This PR will use `max_identifier_length` method instead of the constant and
accept longer identifier (max 128 bytes) when using Oracle 12.2 or higher.

I encountered the error in Rails 6.0. So I'd like to backport to 6.1 and 6.0 stable branches.
@koic koic force-pushed the use_longer_identifier_for_oracle_12c branch from ce00caa to e605c2a Compare January 19, 2021 06:40
Copy link
Collaborator

@yahonda yahonda left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. I'm not sure if supports_longer_identifier or supports_longer_identifier? are good argument names yet. At least this argument name looks long.

Let me have some more time to consider that.

@@ -457,7 +457,7 @@ def write_lob(lob, value, is_binary = false)
end

# To allow private method called from `JDBCConnection`
def describe(name)
def describe(name, supports_longer_identifier)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean supports_longer_identifier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. It takes the result of ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter#supports_longer_identifier? as argument. Unfortunately no better name has come up yet.

@koic
Copy link
Collaborator Author

koic commented Jan 20, 2021

I found that there will probably be breaking change in this PR. Please wait for a while due to I will investigate it.

@yahonda
Copy link
Collaborator

yahonda commented Jan 21, 2021

Sure.

Although I have not checked this pull request in detail yet, In general, I prefer not to change method signature/arity for this requirement. Because the Oracle database version never changes per database connection. Also, I especially prefer not to change the method signature if it requires to backport to the older versions of Oracle enhanced adapter.

Let's have some discussion if you have questions. Thank you.

@gpg0
Copy link

gpg0 commented Apr 22, 2021

Any updates on this?

@stale
Copy link

stale bot commented Jun 26, 2021

This issue 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.

@stale stale bot added the wontfix label Jun 26, 2021
@koic koic removed the wontfix label Jun 28, 2021
@mattalat
Copy link

mattalat commented Nov 4, 2021

Is there any work that needs to be completed on this? If so, I'd be happy to help. I am eager to see this in a release!

@akostadinov
Copy link
Contributor

akostadinov commented Jan 13, 2022

@mattalat, the biggest complaint I see is changing of the method signatures. Since identifiers are same for the whole connection, I think that probably detecting their length within the connection and then accessing that value wherever needed would resolve that.

Also my suggestion would be to have a method that returns length of identifiers instead of long/short and other non-descriptive names. This may allow users to set some configuration option to force certain identifiers max length.

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.

5 participants