Restart postgres if required when updating pgParameters #568

prabhu43 · 2018-09-21T15:48:07Z

Alternate approach that. Fixes #88

Context:

If some postgres configurations (pgParameters) that require restart are updated, a restart is triggered

Approach:

For Postgres versions >= 9.5,

After reloading pgParameters, we check if pg_settings table has pending_restarts

pending_restart | boolean | true if the value has been changed in the configuration file but needs a restart; or false otherwise.

9.5 pg_settings reference

For older versions of postgres,

Figure out the changed Parameters.
Check if any of the changedParameters requires restart by checking in pg_settings's context

postmaster: These settings can only be applied when the server starts, so any change requires restarting the server. Values for these settings are typically stored in the postgresql.conf file, or passed on the command line when starting the server. Of course, settings with any of the lower context types can also be set at server start time.

9.4 pg_settings reference

sgotti

@prabhu43 Thanks a lot for the PR! Just two comments (also some integration tests will be really appreciated)

Since I would like to make it opt-in (see the comments) I think this could be complementary to #561 (so users could manually request an instance restart)

sgotti · 2018-09-25T14:26:56Z

cmd/keeper/cmd/keeper.go

@@ -1588,6 +1589,17 @@ func (p *PostgresKeeper) postgresKeeperSM(pctx context.Context) {
 		if err := pgm.Reload(); err != nil {
 			log.Errorw("failed to reload postgres instance", err)
 		}
+		needsRestart, err := pgm.IsRestartRequired(changedParams)


Just a not for a future improvement (will prefer doing this in a separate PR): we could report the need restart in the PostgresState reported to the sentinel so it could be showed in the stolonctl status output for people that won't enable automatic restart (see below comment).

sgotti · 2018-09-25T14:41:24Z

cmd/keeper/cmd/keeper.go

+			log.Errorw("Failed to checked if restart is required", err)
+		}
+
+		if needsRestart {


I'll make this opt in adding an option in the cluster spec. I'll leave it disabled by default (at least for the moment, to not break current expected behavior and avoid possible issue when users provides a wrong parameter that will break instance restart)

@sgotti As you suggested, We will make the pgRestart optional(disbaled by default) and add some integration tests. We will try to close this by this weekend.
cc @aswinkarthik

sgotti · 2018-10-09T18:58:16Z

@prabhu43 gentle ping.

aswinkarthik · 2018-10-17T10:24:25Z

Hi @sgotti We added Integration test. However, we are not able to debug why it is failing exactly. The logs are too verbose. Could you give us some pointers? Also we found it difficult to run it locally. Are there any docs for doing so?

sgotti · 2018-10-17T14:02:06Z

We added Integration test.

Great!

However, we are not able to debug why it is failing exactly. The logs are too verbose. Could you give us some pointers?

You should fetch the output log from semaphore and search for --- FAIL :

--- FAIL: TestAutomaticPgRestart (28.41s)
[...]
    config_test.go:550: expected max_connections 150 is not equal to actual 100

Also we found it difficult to run it locally. Are there any docs for doing so?

We just do the same that is done on semaphore:

For example to test with the locally installed postgres using etcdv3 backend:

INTEGRATION=1 STOLON_TEST_STORE_BACKEND=etcdv3 bash -x ./test

You can also parallelize (if you have fast disks):

INTEGRATION=1 STOLON_TEST_STORE_BACKEND=etcdv3 PARALLEL=10 bash -x ./test

Or use a tmpfs if you have a lot of ram as I do:

mkdir /dev/shm/stolon
export TMPDIR=/dev/shm/stolon

rm -rf /dev/shm/stolon/* ; INTEGRATION=1 STOLON_TEST_STORE_BACKEND=etcdv3 PARALLEL=20 bash -x ./test

so the tests will complete in ~2 minutes.

Co-authored-by: Aswin Karthik <aswinkarthik93@gmail.com>

aswinkarthik · 2018-10-20T12:50:41Z

@sgotti The integartion test is done. Could you review the commit?

sgotti · 2018-10-24T18:54:36Z

@aswinkarthik Great! Will review tomorrow.

sgotti · 2018-10-25T08:10:03Z

@prabhu43 Looks good to me!

Just noticed a behavior that I'm not sure how to deal with:

If you have "automaticPgRestart": false and you update a parameter that needs a restart, postgres won't be restarted (correct). If you then set "automaticPgRestart": true it won't be restarted since pgm.curParameters has been updated and isn't changed. You have to update the parameter again (with a different value) to trigger the automatic restart.

I don't see this as a big deal and the behavior could be improved in a next PR if needed. What do you think?

We should also update the related doc:

https://github.com/sorintlab/stolon/blob/master/doc/cluster_spec.md: add the automaticPgRestart option.
https://github.com/sorintlab/stolon/blob/master/doc/postgres_parameters.md: describe the new behavior.

We can also add this in a separate PR. Let me know what you prefer.

prabhu43 · 2018-10-25T17:08:07Z

@sgotti We felt that the intent of "automaticPgRestart" is to do postgres restart only when it is required which is on changing some pgParameters.

We will add update the docs in this PR itself.

cc @aswinkarthik

Co-authored-by: Prabhu Jayakumar <j.prabhu91@gmail.com> Signed-off-by: Aswin Karthik <aswinkarthik93@gmail.com>

aswinkarthik · 2018-10-29T14:54:46Z

I have updated documentation @sgotti

aswinkarthik · 2018-10-29T15:04:59Z

If you have "automaticPgRestart": false and you update a parameter that needs a restart, postgres won't be restarted (correct). If you then set "automaticPgRestart": true it won't be restarted since pgm.curParameters has been updated and isn't changed. You have to update the parameter again (with a different value) to trigger the automatic restart.

Regarding this, there is a slight inconsistency here based on Postgres version.

9.5 and after:

If automaticPgRestart is false, and max_connections is changed, then automaticPgRestart is enabled, the postgres will be restarted. As in the database, pending_restart is a column in pg_settings and it will be marked true and hence triggering a restart.

For 9.4 and lower:

If automaticPgRestart is false, and max_connections is changed, then automaticPgRestart is enabled, the postgres will not be restarted. As in the database, context is a column in pg_settings that gives info if a restart is needed in case a change is made and is just a static data and does not change dynamically if pgParams are changed. We need the diff to check against it to see if restart is needed. Hence, we will not be able to restart.

prabhu43 · 2018-11-05T18:18:39Z

@sgotti We updated the docs with required details. Can you review those changes and merge the PR ?

sgotti · 2018-11-07T07:59:31Z

@prabhu43 @aswinkarthik LGTM. Merging. Thanks a lot!

Restart postgres if required when updating pgParameters

prabhu43 · 2018-11-07T18:01:15Z

@sgotti Thanks for merging. When can we expect this to be released?

prabhu43 force-pushed the restart-pg-for-necessary-params branch from 437b807 to 385a109 Compare September 24, 2018 05:12

sgotti requested changes Sep 25, 2018

View reviewed changes

aswinkarthik force-pushed the restart-pg-for-necessary-params branch 4 times, most recently from 7d4c530 to de1ad5e Compare October 15, 2018 13:58

prabhu43 force-pushed the restart-pg-for-necessary-params branch 4 times, most recently from ce79c57 to 659359c Compare October 16, 2018 04:27

aswinkarthik force-pushed the restart-pg-for-necessary-params branch 3 times, most recently from 9978016 to f9c1ba8 Compare October 20, 2018 07:27

prabhu43 force-pushed the restart-pg-for-necessary-params branch from f9c1ba8 to 4daa9a3 Compare October 20, 2018 08:26

Restart postgres if required when updating pgParameters

531df56

Co-authored-by: Aswin Karthik <aswinkarthik93@gmail.com>

prabhu43 force-pushed the restart-pg-for-necessary-params branch from 4daa9a3 to 531df56 Compare October 20, 2018 11:21

aswinkarthik force-pushed the restart-pg-for-necessary-params branch from e95bc59 to b5d34ae Compare October 29, 2018 14:22

Add documentation for automaticPgRestart

b62fc6e

Co-authored-by: Prabhu Jayakumar <j.prabhu91@gmail.com> Signed-off-by: Aswin Karthik <aswinkarthik93@gmail.com>

aswinkarthik force-pushed the restart-pg-for-necessary-params branch from b5d34ae to b62fc6e Compare October 29, 2018 14:24

sgotti merged commit b62fc6e into sorintlab:master Nov 7, 2018

sgotti added a commit that referenced this pull request Nov 7, 2018

Merge pull request #568 from prabhu43/restart-pg-for-necessary-params

979c44c

Restart postgres if required when updating pgParameters

sgotti added this to the v0.14.0 milestone Apr 19, 2019

maksm90 mentioned this pull request Jun 6, 2019

Release v0.14.0 #667

Merged

maksm90 mentioned this pull request Oct 2, 2019

Concerns about stability of automatic restart of nodes feature #707

Open

Restart postgres if required when updating pgParameters #568

Restart postgres if required when updating pgParameters #568

Uh oh!

Conversation

prabhu43 commented Sep 21, 2018

Context:

Approach:

Uh oh!

sgotti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgotti Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

sgotti Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

prabhu43 Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

sgotti commented Oct 9, 2018

Uh oh!

aswinkarthik commented Oct 17, 2018

Uh oh!

sgotti commented Oct 17, 2018

Uh oh!

aswinkarthik commented Oct 20, 2018

Uh oh!

sgotti commented Oct 24, 2018

Uh oh!

sgotti commented Oct 25, 2018

Uh oh!

prabhu43 commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aswinkarthik commented Oct 29, 2018

Uh oh!

aswinkarthik commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prabhu43 commented Nov 5, 2018

Uh oh!

sgotti commented Nov 7, 2018

Uh oh!

prabhu43 commented Nov 7, 2018

Uh oh!

Uh oh!

sgotti left a comment •

edited

Loading

prabhu43 commented Oct 25, 2018 •

edited

Loading

aswinkarthik commented Oct 29, 2018 •

edited

Loading