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

Fixes #36850 - Avoid duplicate database indexes #10805

Closed

Conversation

hao-yu
Copy link
Contributor

@hao-yu hao-yu commented Nov 17, 2023

Some downstream users have already created the indexes manually to relieve the issue. When upgrading Katello, the migration script will create duplicate indexes to the table with different index names or the migration will fail with PG duplicate index error due to index name conflict with the one migration script creates.

Thus, modify this migration script to check for existing indexes in the table and then drop them before creating the new indexes.

Some downstream users have already created the indexes manually
to relieve the issue. When upgrading Katello, the migration
script will create duplicate indexes to the table with
different index names or the migration will fail with PG
duplicate index error due to index name conflict with the one
migration script creates.

Thus, modify this migration script to check for existing indexes
in the table and then drop them before creating the new indexes.
@hao-yu
Copy link
Contributor Author

hao-yu commented Nov 17, 2023

@jeremylenz As discussed, I submitted the changes I proposed in PR 10773

@lfu
Copy link
Member

lfu commented Nov 17, 2023

This is not necessary.
You can remove a migration using:

rake db:migrate:down VERSION=20230816154510

Replace the Migration ID with yours.

@hao-yu
Copy link
Contributor Author

hao-yu commented Nov 17, 2023

@lfu we don't want to do a manual job here. Besides, if we rollback the migration as you suggested it will remove the indexes including the one created manually.

@lfu
Copy link
Member

lfu commented Nov 20, 2023

Could you try something like:

add_index(:suppliers, :name, if_not_exists: true)

@hao-yu
Copy link
Contributor Author

hao-yu commented Nov 22, 2023

add_index(:suppliers, :name, if_not_exists: true)

@lfu Hmm...because of my mistake the indexes that we manually created have different name as what rails will be created so I am thinking to drop them and let rails re-create them. Thus, the index names will be consistent. If you think that is ok, I am fine to implement your suggestion above.

@lfu
Copy link
Member

lfu commented Nov 22, 2023

@hao-yu Can you tell us how you have created the index manually for customers?

@hao-yu
Copy link
Contributor Author

hao-yu commented Nov 23, 2023

Can you tell us how you have created the index manually for customers?

@lfu

su - postgres -c "psql foreman -c \"create index index_katello_installed_packages_nvra on katello_installed_packages(nvra);\""
su - postgres -c "psql foreman -c \"create index index_katello_installed_packages_nvra_epoch on katello_installed_packages(nvra, epoch);\""

@hao-yu
Copy link
Contributor Author

hao-yu commented Nov 30, 2023

how is this review going?

@lfu
Copy link
Member

lfu commented Nov 30, 2023

In my opinion, the best way to solve the issue here is to have the users manually remove the index that was manually added in the first place if any issue comes up due to that change.

To find duplicate indexes

    SELECT
        indrelid::regclass AS TableName
        ,array_agg(indexrelid::regclass) AS Indexes
    FROM pg_index
    GROUP BY
        indrelid
        ,indkey
    HAVING COUNT(*) > 1; 
                   tablename               |                                            indexes
---------------------------------------+-----------------------------------------------------------------------------------------------
 katello_content_views                 | {index_content_views_on_name_and_organization_id,katello_content_views_name_unique_index}
 katello_smart_proxy_sync_history      | {index_katello_smart_proxy_sync_history_on_repository_id,index_spsh_repository_id}
 katello_smart_proxy_sync_history      | {index_katello_smart_proxy_sync_history_on_smart_proxy_id,index_spsh_smart_proxy_id}
 katello_content_view_repositories     | {cvd_repo_index,katello_content_view_repositories_unique_index}
 katello_installed_packages            | {index_katello_installed_packages_on_nvra,index_katello_installed_packages_nvra}
 katello_content_facet_applicable_debs | {katello_cf_applicable_debs_cf_idx,index_k_content_facet_applicable_debs_on_content_facet_id}
(6 rows)

Then DROP INDEX index_name;

@hao-yu
Copy link
Contributor Author

hao-yu commented Feb 19, 2024

I am not sure how user friendly we are asking users to remove duplicate indexes manually.

Unfortunately, the proposed PR is sitting here for 4 months without proper or motivation to progress

It is no point and too late to fix this any more because #10773 has been released in downstream Satellite 6.14. For affected users who already upgraded to 6.14 will have duplicate indexes.

@hao-yu hao-yu closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants