Skip to content

💄 Surface better error when a publication is in an invalid state #1844

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acco
Copy link
Contributor

@acco acco commented Jun 18, 2025

No description provided.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 18, 2025
Copy link
Contributor Author

acco commented Jun 18, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@acco acco force-pushed the acco/06-18-_surface_better_error_when_a_publication_is_in_an_invalid_state branch from addb683 to 57f6055 Compare June 18, 2025 17:10
@dosubot dosubot bot added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 18, 2025
@acco acco force-pushed the acco/06-18-_surface_better_error_when_a_publication_is_in_an_invalid_state branch from 57f6055 to 8d5f6c0 Compare June 18, 2025 17:13
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jun 18, 2025
Copy link
Contributor

@RTLS RTLS left a comment

Choose a reason for hiding this comment

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

Nice


This is a [known Postgres issue](https://www.postgresql.org/message-id/18683-a98f79c0673be358%40postgresql.org).

**Fix**: drop and re-create the replication slot *after* the publication exists:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to prevent missing any changes, you should create a new slot, move Sequin to the new slot, then drop the old slot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RTLS The old slot will contain changes that the new slot will not when you create it :( so data loss feels certain. It's a big bummer, as dropping a publication feels non-deleterious, but it can be problematic.

Interestingly, by happenstance, it might work as when we connect to the new slot we might send back an old LSN first (cached) and possibly traverse backward - but this would be basically unsupported behavior. We might consider ways to make this operation "first class"

Comment on lines +123 to +127
def to_existing_atom_safe(string) do
String.to_existing_atom(string)
rescue
_ -> string
end
Copy link
Contributor

Choose a reason for hiding this comment

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

should this return a status tuple? I feel like this will lead to raises later due to an expected string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will change!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants