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

Method to safely add index on empty table #93

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Conversation

rkrage
Copy link
Contributor

@rkrage rkrage commented Nov 17, 2023

This will raise an error if it detects that the table contains data and/or the total size on disk is greater than 10 megabytes. Otherwise, we safely acquire a SHARE lock and run standard CREATE INDEX ...

if options[:algorithm] == :concurrently
# Cannot be run inside a transaction so we are unable to use
# safely_acquire_lock_for_table to take out ShareUpdateExclusiveLock
execute_ancestor_statement(:add_index, table, column_names, **options)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting catch. This is a project philosophy question, but should we insist that the user call safe_add_index_concurrently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I'm following this one

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be from an earlier version that updated where we did locking. Since that's no longer true in the PR, I think we can wait on this (but I also don't quite remember the context of my question about project policy, and I'm struggling to find the version of the PR at the time I left the comment, so I can't see the full context of this change...)

spec/safe_statements_spec.rb Outdated Show resolved Hide resolved
@rkrage rkrage force-pushed the add-index-empty-table branch 2 times, most recently from 3232f29 to a7489d0 Compare November 17, 2023 21:51
Base automatically changed from lock-modes to master November 20, 2023 15:21
@rkrage rkrage force-pushed the add-index-empty-table branch from a7489d0 to e55f9f1 Compare November 20, 2023 15:23
@rkrage
Copy link
Contributor Author

rkrage commented Dec 29, 2023

I'm also wondering if maybe we want this method to just be called safe_add_index? Or do you like that the validation it's doing is in the name?

@rkrage rkrage force-pushed the add-index-empty-table branch from e202818 to d934b92 Compare January 11, 2024 16:25
@rkrage rkrage force-pushed the add-index-empty-table branch from d934b92 to 6fffef8 Compare January 11, 2024 16:56
def _ensure_empty_table!(table)
table = PgHaMigrations::Table.from_table_name(table)

if connection.select_value("SELECT EXISTS (SELECT 1 FROM #{table.fully_qualified_name} LIMIT 1)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also ensure the table size is less than, I don't know, 100 MB? Even that's insanely high for what it should be in this case, but it gives some buffer if it's not entirely clean (a bunch of recent deletes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 058c4d9

lib/pg_ha_migrations/safe_statements.rb Outdated Show resolved Hide resolved
@rkrage rkrage merged commit 2b8dc9e into master Jan 11, 2024
109 checks passed
@rkrage rkrage deleted the add-index-empty-table branch January 11, 2024 21:53
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.

2 participants