Merge branch '229852-update-index-database-guides' into 'master'

Update database docs for new index name rules

Closes #229852

See merge request gitlab-org/gitlab!40892

doc/development/adding_database_indexes.md

@@ -121,3 +121,71 @@ may be affected by factors such as (but not limited to):
 In other words, this data is only reliable for a frequently used database with
 plenty of data and with as many GitLab features enabled (and being used) as
 possible.
+
+## Requirements for naming indexes
+
+Indexes with complex definitions need to be explicitly named rather than
+relying on the implicit naming behavior of migration methods. In short,
+that means you **must** provide an explicit name argument for an index
+created with one or more of the following options:
+
+- `where`
+- `using`
+- `order`
+- `length`
+- `type`
+- `opclass`
+
+### Considerations for index names
+
+Index names don't have any significance in the database, so they should
+attempt to communicate intent to others. The most important rule to
+remember is that generic names are more likely to conflict or be duplicated,
+and should not be used. Some other points to consider:
+
+- For general indexes, use a template, like: `index_{table}_{column}_{options}`.
+- For indexes added to solve a very specific problem, it may make sense
+  for the name to reflect their use.
+- Identifiers in PostgreSQL have a maximum length of 63 bytes.
+- Check `db/structure.sql` for conflicts and ideas.
+
+### Why explicit names are required
+
+As Rails is database agnostic, it generates an index name only
+from the required options of all indexes: table name and column name(s).
+For example, imagine the following two indexes are created in a migration:
+
+```ruby
+def up
+  add_index :my_table, :my_column
+
+  add_index :my_table, :my_column, where: 'my_column IS NOT NULL'
+end
+```
+
+Creation of the second index would fail, because Rails would generate
+the same name for both indexes.
+
+This is further complicated by the behavior of the `index_exists?` method.
+It considers only the table name, column name(s) and uniqueness specification
+of the index when making a comparison. Consider:
+
+```ruby
+def up
+  unless index_exists?(:my_table, :my_column, where: 'my_column IS NOT NULL')
+    add_index :my_table, :my_column, where: 'my_column IS NOT NULL'
+  end
+end
+```
+
+The call to `index_exists?` will return true if **any** index exists on
+`:my_table` and `:my_column`, and index creation will be bypassed.
+
+The `add_concurrent_index` helper is a requirement for creating indexes
+on populated tables. Since it cannot be used inside a transactional
+migration, it has a built-in check that detects if the index already
+exists. In the event a match is found, index creation is skipped.
+Without an explicit name argument, Rails can return a false positive
+for `index_exists?`, causing a required index to not be created
+properly. By always requiring a name for certain types of indexes, the
+chance of error is greatly reduced.

doc/development/migration_style_guide.md

@@ -465,13 +465,17 @@ class MyMigration < ActiveRecord::Migration[6.0]
   disable_ddl_transaction!
 
   def up
-    remove_concurrent_index :table_name, :column_name
+    remove_concurrent_index :table_name, :column_name, name: :index_name
   end
 end
 ```
 
 Note that it is not necessary to check if the index exists prior to
-removing it.
+removing it, however it is required to specify the name of the
+index that is being removed. This can be done either by passing the name
+as an option to the appropriate form of `remove_index` or `remove_concurrent_index`,
+or more simply by using the `remove_concurrent_index_by_name` method. Explicitly
+specifying the name is important to ensure the correct index is removed.
 
 For a small table (such as an empty one or one with less than `1,000` records),
 it is recommended to use `remove_index` in a single-transaction migration,
@@ -512,11 +516,16 @@ class MyMigration < ActiveRecord::Migration[6.0]
   end
 
   def down
-    remove_concurrent_index :table, :column
+    remove_concurrent_index :table, :column, name: index_name
   end
 end
 ```
 
+You must explicitly name indexes that are created with more complex
+definitions beyond table name, column name(s) and uniqueness constraint.
+Consult the [Adding Database Indexes](adding_database_indexes.md#requirements-for-naming-indexes)
+guide for more details.
+
 If you need to add a unique index, please keep in mind there is the possibility
 of existing duplicates being present in the database. This means that should
 always _first_ add a migration that removes any duplicates, before adding the
@@ -526,6 +535,42 @@ For a small table (such as an empty one or one with less than `1,000` records),
 it is recommended to use `add_index` in a single-transaction migration, combining it with other
 operations that don't require `disable_ddl_transaction!`.
 
+## Testing for existence of indexes
+
+If a migration requires conditional logic based on the absence or
+presence of an index, you must test for existence of that index using
+its name. This helps avoids problems with how Rails compares index definitions,
+which can lead to unexpected results. For more details, review the
+[Adding Database Indexes](adding_database_indexes.md#why-explicit-names-are-required)
+guide.
+
+The easiest way to test for existence of an index by name is to use the
+`index_name_exists?` method, but the `index_exists?` method can also
+be used with a name option. For example:
+
+```ruby
+class MyMigration < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  INDEX_NAME = 'index_name'
+
+  def up
+    # an index must be conditionally created due to schema inconsistency
+    unless index_exists?(:table_name, :column_name, name: INDEX_NAME)
+      add_index :table_name, :column_name, name: INDEX_NAME
+    end
+  end
+
+  def down
+    # no op
+  end
+end
+```
+
+Keep in mind that concurrent index helpers like `add_concurrent_index`,
+`remove_concurrent_index`, and `remove_concurrent_index_by_name` already
+perform existence checks internally.
+
 ## Adding foreign-key constraints
 
 When adding a foreign-key constraint to either an existing or a new column also