Enforce limit on table names length

[fatkodima]

Fixes #45130

For long table names, to generate primary key index name, postgres truncates table name even further and adds _pkey suffix and this all should fit into 63 bytes.

Active Record already has constants for different name limits https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/abstract/database_limits.rb and even checks index names in several places, like

rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

Line 341 in 9e03207

validate_index_length!(table_name, new_name)

and several more places (but not everywhere).

I suggest to implement similar checks for table/(foreign key)/etc names, or at least for table names, as a followup in a separate PR, taking into account backwards compatibility with older migration files.

cc @nvasilevski

[nvasilevski]

It's a nitpick and totally up to you but I really find the "#{table_name}_pkey" very readable and was wondering if we could somehow keep it, something like:

Suggested change

	idx = "#{table_name[0, index_name_length - '_pkey'.size]}_pkey"
	table_name_max_length = index_name_length - '_pkey'.size
	truncated_table_name = table_name[0,table_name_max_length]
	idx = "#{truncated_table_name}_pkey"

Not sure about the truncated_table_name name though as it's not always truncated, perhaps limited_table_name?

[nvasilevski]

It's not necessary and might be personal preference but perhaps it would be reasonable to add an assertion or just a check like flunk("wasn't truncated") unless <how can we check for truncation> to make sure that truncation actually happens. Generally we are pretty confident because of relying on max_identifier_length but what worries me is that the test isn't going to fail if max_identifier_length accidentally changes to something like 10, or an actual database limit becomes higher like 128 but the test still going to be green

[fatkodima]

For postgres, we dynamically get a config (compared to hardcoded value for of 64 for other adapters)

rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

Lines 492 to 495 in 77e6bfa

	# Returns the configured supported identifier length supported by PostgreSQL
	def max_identifier_length
	@max_identifier_length ||= query_value("SHOW max_identifier_length", "SCHEMA").to_i
	end

Wont' this be enough to detect changes?

[nvasilevski]

Sorry, "go to definition" confused me and I didn't check the method definition for postgresql adapter 🙈
Nvm my comment, thanks for pointing out!

[byroot]

we dynamically get a config (compared to hardcoded value for of 64 for other adapters)

This kinda confirm my hunch that we shouldn't automatically truncate, otherwise a migration could produce different index names in dev and prod if the postgres is configured differently.

[fatkodima]

True, and it could also produce different table names, but it will be able to correctly determine this truncated name in both envs.

[byroot]

I'm a bit on the fence here, truncating index names makes for weird results. Maybe it would make more sense to raise an error instead, with a message indicating that the default name is too long and that a custom name must be set?

[fatkodima]

It would be also hard to tell which index name is too long, since dev and prod can have different values for this configuration.

[byroot]

since dev and prod can have different values for this configuration.

It's fine. If that's the case the migration will fail in one env, as long as the error message is clear we're good I think.

I prefer for us to bail out saying we don't know what to do, rather than risking doing the wrong thing.

[fatkodima]

1. So what do you think about my initial suggestion in the description:

I suggest to implement similar checks for table/(foreign key)/etc names, or at least for table names?

2. And should we take into account backwards compatibility with older migration files?

[byroot]

1. So what do you think about my initial suggestion in the description:

👍

And should we take into account backwards compatibility with older migration files?

It would be safer yes.

[byroot]

🤦 sorry wrong PR...

[fatkodima]

@byroot I am having troubles on how to make these changes backwards compatible.
For example, if we add the check for table_name to create_table, how to make it compatible in compatibility.rb without duplicating all the code for create_table (except the new check for name length) or without hacks similar to this

rails/activerecord/lib/active_record/migration/compatibility.rb

Line 185 in 21651b0

options[:_uses_legacy_reference_index_name] = true

?

[byroot]

without hacks similar to this

I'd say try this way, we'll see how it looks. If it's a bit too disgusting we can fallback to copying as last resort. Duplication a bit annoying for future maintenance, but not the end of the world.

[fatkodima]

@byroot Updated.
Index names length is already validated for all adapters. I added only checks for table names.
Other identifiers, like column names, foreign keys, check constraints etc probably should be also checked.
But since for foreign keys and check constraints the auto generated names are already truncated and it is very unlikely that the user will specify a long name manually, I think checking only index names and table names should be enough.