Only restart the cluster when absolutely necessary, otherwise reload

[gclough]

Only restart the cluster when absolutely necessary, otherwise do a reload. Default to mega safety, and even if a restart is needed, then only do so if the "postgresql_restart_on_config_change" variable is set to true (default false)

Most normal parameter changes will simply "reload" the cluster, meaning no downtime.

Fixes #291

[erdenezul]

Good point

[UnderGreen]

@gclough In my previous company we have solved issue with restarting PostgreSQL by creating template file with options, setting which requires restart. And then call handler to restart Postgres only if file changed and reload in other way.

[gclough]

I did consider that @UnderGreen , but I thought that leveraging the built-in functionality of the pg_settings(pending_restart) column would be more stable going forward. It also allows us to have all of the settings in a single template, just to keep things simple.

The only downside is that it doesn't work for v9.4 and earlier... although I figured that's an acceptable trade-off, and possibly an encouragement for those on older versions to upgrade. v9.4 is End of Life in Dec/2019, so there are only 20 months until it's obsolete.

[gclough]

Closing to re-trigger Travis

[gclough]

@sebalix , @UnderGreen, and @jlozadad ... this touches so much of the role so merging in recent changes has been a bit messy. I think I've squashed the commits as you wanted, but if I'm doing it wrong then please let me know.

I would appreciate a code review on this, and I will put in comments anywhere it's a bit unclear.

[aoyawale]

the vagrant changes you added are alrdy on master so those are not needed.

[aoyawale]

you might have to start making smaller PRs since it would be easier to go through.

[aoyawale]

i think it would be easier if you do a PR with only what fixed the travis build peer auth part. that way we can merge that part and fix it in the other PRs.

[aoyawale]

when you do include_tasks it does not grab the restart.yml automatically? i dont think u need to specify a role path.

[gclough]

@jlozadad

the vagrant changes you added are alrdy on master so those are not needed.

Yeah, I think I messed up my rebase, but after I had resolved the conflicts then it removed those from the "Changed Files" tab. Hopefully it's all looking better now.

you might have to start making smaller PRs since it would be easier to go through.

I think this one looked huge due to my rebase error, but now I've merged it with master it seems to be much smaller.

i think it would be easier if you do a PR with only what fixed the travis build peer auth part. that way we can merge that part and fix it in the other PRs.

After I fixed the rebase error, I think it's now only got the changes necessary for this fix, which was to avoid doing any restart unless it's necessary. Yes, it did fiddle with the handlers a lot, but that was needed to achieve the logic of restart/reload and multiple different versions with differing capabilities.

when you do include_tasks it does not grab the restart.yml automatically? i dont think u need to specify a role path.

That's what I tried, but it failed because it couldn't find the file. Initially I was using import_tasks without any path... but that part of Ansible seems to be a little bit broken.

[aoyawale]

no problem. i had that happen the other too :)

[aoyawale]

i think its better if you add what you have in reload.yml and put on main.yml and remove the role path. you can still call the handler from there.

[aoyawale]

for psql queries i think we can leverage https://github.com/rtshome/ansible_pgsql from the lib dir and use those and avoid shell/command since it can get dirty quickly.

[gclough]

@jlozadad ... the use of pgsql seems pretty common in this role, but I've opened #339 to try and make that module work. It seems to work locally, but we will then have a pre-requisite before folks can use the ANXS.postgresl role.

Is there a better way to do this?

[aoyawale]

so what we can do is not copy the role and everything they have but, create a library directory under this role and copy the *py files over. Its just a though at this moment since I have not tested using them with this role. But, it can be a great improvement since they are actual ansible modules.

[gclough]

@jlozadad ... I was trying not to copy in their role wholesale, as then I'd be responsible to maintain it and update ANXS.postgresql whenever it changes. If there's a standard way to just "import" a role, then I'd prefer to do it that way.

[UnderGreen]

Guys, did you think about tech diligence of that modules? I know python, but don't have a time to review. We should not use modules without any review.

[aoyawale]

that is why I mentioned It will need to be tested and checked more. Also as a potential solution.

[aoyawale]

@gclough you don't have to import role or do anything of that nature. If you are adding modules all you need is the *.py files in the libraries directory on this role. But, like mentioned this was just a though and not a "we need to do this now"

[gclough]

OK @UnderGreen and @jlozadad ... it sounds like we should continue with the "shell / command", and then potentially replace it with ansible_pgsql in a separate Pull Request, if and when we're comfortable with it.

Does that sound sensible? This is a reasonably large PR, so if you have any questions then please let me know. IMHO it's very beneficial, because as it stands any small change in the *.conf files will result in unnecessary downtime on the database.

[github-actions[bot]]

This pr has been marked 'stale' due to lack of recent activity. If there is no further activity, the issue will be closed in another 30 days. Thank you for your contribution!