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

Improve replication object clean-up on failure. #59

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

kathia-barahona
Copy link
Contributor

@kathia-barahona kathia-barahona commented Jan 20, 2025

Added some improvements on error handling whenever pg_migrate is cleaning any replication object (publication, subscription, replication slot). As our current implementation does not drop objects whenever create_<repl_object_type> fails (raises an exception). Current clean-up is dependent on the object name returned by those functions, but if nothing is returned then nothing will be cleaned. The improvement makes the cleanup independent from the return of such functions, as it nows fetches the replication object name and verifies it exists or not, instead of relying on return values.

Also updated git workflow, for testing against newer versions (PG13-PG17) and use >= Python 3.10

Proposed changes in this pull request

Type (put an x where ever applicable)

  • Bug fix: Link to the issue
  • Feature (Non-breaking change)
  • Feature (Breaking change)
  • Documentation Improvement
  • Other

Checklist

Please put an x against the checkboxes. Write a small comment explaining if its N/A (not applicable)

  • All the tests are passing after the introduction of new changes.
  • Added tests respective to the part of code I have written.
  • Added proper documentation where ever applicable (in code and README.md).

Optional extra information

@kathia-barahona kathia-barahona force-pushed the kathiabarahona/improve_exception_handling_on_cleanup branch 24 times, most recently from b5e7be4 to cb5e018 Compare January 27, 2025 10:38
@kathia-barahona kathia-barahona force-pushed the kathiabarahona/improve_exception_handling_on_cleanup branch from cb5e018 to edf4e64 Compare January 27, 2025 10:40
@kathia-barahona kathia-barahona marked this pull request as ready for review January 27, 2025 11:00
@kathia-barahona kathia-barahona requested a review from a team as a code owner January 27, 2025 11:00
@kathia-barahona kathia-barahona force-pushed the kathiabarahona/improve_exception_handling_on_cleanup branch from edf4e64 to 424c9b1 Compare January 27, 2025 11:17
aiven_db_migrate/migrate/pgmigrate.py Show resolved Hide resolved
aiven_db_migrate/migrate/pgmigrate.py Outdated Show resolved Hide resolved
aiven_db_migrate/migrate/pgmigrate.py Outdated Show resolved Hide resolved
Added some improvements on error handling whenever pg_migrate is cleaning any replication object (publication, subscription, replication slot). As our current implementation does not drop objects whenever create_<repl_object_type> fails (raises an exception). Current clean-up is dependent on the object name returned by those functions, but if nothing is returned then nothing will be cleaned. The improvement makes the cleanup independent from the return of such functions, as it nows fetches the replication object name and verifies it exists or not, instead of relying on return values.
@kathia-barahona kathia-barahona force-pushed the kathiabarahona/improve_exception_handling_on_cleanup branch from 424c9b1 to 0159aaf Compare February 6, 2025 09:39
Comment on lines +621 to +637
try:
if replication_object_type is ReplicationObjectType.PUBLICATION:
delete_query = f"DROP PUBLICATION {rep_obj_name};"
args = ()
elif replication_object_type is ReplicationObjectType.REPLICATION_SLOT:
delete_query = f"SELECT 1 FROM pg_catalog.pg_drop_replication_slot(%s)"
args = (rep_obj_name, )
else:
# cleanup only handles publications and replication slots in source.
return

self.log.info(
"Dropping %r %r from database %r",
rep_obj_type_display_name,
rep_obj_name,
dbname,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try:
if replication_object_type is ReplicationObjectType.PUBLICATION:
delete_query = f"DROP PUBLICATION {rep_obj_name};"
args = ()
elif replication_object_type is ReplicationObjectType.REPLICATION_SLOT:
delete_query = f"SELECT 1 FROM pg_catalog.pg_drop_replication_slot(%s)"
args = (rep_obj_name, )
else:
# cleanup only handles publications and replication slots in source.
return
self.log.info(
"Dropping %r %r from database %r",
rep_obj_type_display_name,
rep_obj_name,
dbname,
)
if replication_object_type is ReplicationObjectType.PUBLICATION:
delete_query = f"DROP PUBLICATION {rep_obj_name}"
args = ()
elif replication_object_type is ReplicationObjectType.REPLICATION_SLOT:
delete_query = f"SELECT 1 FROM pg_catalog.pg_drop_replication_slot(%s)"
args = (rep_obj_name, )
else:
# cleanup only handles publications and replication slots in source.
return
self.log.info(
"Dropping %r %r from database %r",
rep_obj_type_display_name,
rep_obj_name,
dbname,
)
try:

@ettanany ettanany merged commit fb0b936 into main Feb 11, 2025
12 checks passed
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