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

fix(postgres-config): always restart if --no-restart is omitted #2874

Closed

Conversation

avallete
Copy link
Member

What kind of change does this PR introduce?

With the new api changes: https://github.com/supabase/infrastructure/pull/20462

We must always the restart_database setting to the endpoint. Otherwise we go from the logic of "always restart except if --no-restart is specified" to a "restart only if the changed settings require a restart and --no-restart is not specified".

Which in the case of maintenance_work_mem might sometimes don't work as expected

Related: https://app.hubspot.com/live-messages/19953346/inbox/8348874005#comment

PS: An alternative would be to mark maintenance_work_mem as a parameter requiring restart on api side, but according to this documentation: https://postgresqlco.nf/doc/en/param/maintenance_work_mem/ it isn't the case.

What is the current behavior?

Please link any relevant issues here.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Add any other context or screenshots.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11795862893

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 59.77%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/postgresConfig/update/update.go 0 2 0.0%
Totals Coverage Status
Change from base Build 11794104619: 0.03%
Covered Lines: 6381
Relevant Lines: 10676

💛 - Coveralls

@sweatybridge
Copy link
Contributor

sweatybridge commented Nov 12, 2024

Which in the case of maintenance_work_mem might sometimes don't work as expected

I think a better fix for this is on infra worker, by sending a post request to reload config to admin api running on both primary and read replicas. This is needed whenever we update config without restarting database. More context https://supabase.slack.com/archives/C02FHG9QQAF/p1729922433212149?thread_ts=1728485737.099679&cid=C02FHG9QQAF

@avallete
Copy link
Member Author

Which in the case of maintenance_work_mem might sometimes don't work as expected

I think a better fix for this is on infra worker, by sending a post request to reload config to admin api running on both primary and read replicas. This is needed whenever we update config without restarting database. More context https://supabase.slack.com/archives/C02FHG9QQAF/p1729922433212149?thread_ts=1728485737.099679&cid=C02FHG9QQAF

Closing in favor of: https://github.com/supabase/infrastructure/pull/20575

@avallete avallete closed this Nov 12, 2024
@sweatybridge sweatybridge deleted the fix/postgrest-config-udpate-restart-default branch November 13, 2024 05:28
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