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

Retry connections to PostgreSQL before starting martin #1545

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

teyotan
Copy link

@teyotan teyotan commented Oct 21, 2024

Trying to solve (#1539)

There are still a lot of improvements that can be done, such as making the retry params configurable and adding test for unreachable db instance. But would love to get feedbacks first before I proceed on those.

@nyurik
Copy link
Member

nyurik commented Oct 21, 2024

Thanks, looks good! We should discuss the configuration for this too as part of this PR. Jitter is a good idea, thx.

At some point we should also test/evaluate what happens when PG restarts (drops connections, switches to another primary, etc) while Martin is running - in theory PG pool should simply reconnect, but I have never tested that. Probably good for another PR.

@CommanderStorm CommanderStorm changed the title Add retry mechanism to connect to PostgreSQL before starting maplibre… retry connections to PostgreSQL before starting martin Oct 27, 2024
@CommanderStorm CommanderStorm changed the title retry connections to PostgreSQL before starting martin Retry connections to PostgreSQL before starting martin Oct 27, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I moved a bit of code around and think this works better now.

Yes, the 20s total time for PG to accept our connection is a bit much, but as long as this saves someone from debugging something really stupid like adding a bit too long running startup sql..

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

The code looks good, but I do have a big concern:

if the database is dead / incorrect config string, we now will wait for up to 30s, without giving an indication of why it is taking so long, and the fact that we are trying to re-establish the connection?

We cannot merge this until:

  • we give clear indication of the status, but without flooding the output, and only if it is taking more than 2s (?) from the start of the program
  • we offer a clear way to bypass or configure this with a cli + config params, notifying of this method on the first printed warning

Configuration ideas

Add pg-connection-retries (<number>|infinite) as a CLI parameter, as well as in the config block

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.

3 participants