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.

Sign up for GitHub

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

Jump to bottom

Disable WAL archiving #908

Closed
wants to merge 1 commit into from
Closed

Disable WAL archiving #908

wants to merge 1 commit into from

Conversation

spohner
Copy link
Contributor

@spohner spohner commented Apr 7, 2020

@spohner
Copy link
Contributor Author

spohner commented Apr 7, 2020

Resolves #907

@spohner spohner mentioned this pull request Apr 7, 2020
Closed
@spohner
Copy link
Contributor Author

spohner commented May 12, 2020

Did a rebase to align with current master. Any comments on this PR by the maintainers?

@FxKu
Copy link
Member

FxKu commented May 13, 2020 •
edited

@spohner thanks for your contribution. We have this fashion to name our flags "Enable..." and I would like to stick to that. The default should then be true which requires the *bool type to distinguish between false and nil setting. Could you change the CRD validation in the helm chart, too?

@spohner
Copy link
Contributor Author

spohner commented May 15, 2020 •
edited

Did the changes that you requested, @FxKu. Let me know if there is anymore I can do to get this merged. I can see that the CI is failing due to out-of-date codegen files, but dont know why as it works fine on my machine. Did I miss something?

@FxKu
Copy link
Member

FxKu commented May 15, 2020

Thanks. You have to update the generated code. Simply run ./hack/update-codegen.sh. It should update this file.

@MaxFedotov
Copy link

Hello @FxKu, @spohner. Are there any plans to continue with this PR? This looks like a very useful thing.

@derekjanni
Copy link

@spohner @FxKu I second that, very useful feature here, would be cool to make it official

@spohner
Copy link
Contributor Author

spohner commented Mar 18, 2021

Revisited this one and improved it. The previous attempt did not work when editing enableWALArchiving after the cluster was created. Now this should also work. @FxKu

@fourstepper
Copy link

fourstepper commented Apr 18, 2021 •
edited

Would it be considerably difficult to make it possible to configure backup paths/locations/overall Spill pod environment variables per cluster instead of per-operator?

@Menzorg
Copy link
Contributor

Menzorg commented Apr 22, 2021

We really need it. Wish it be merged soon.

machine424
machine424 reviewed Jun 30, 2021
View reviewed changes
pkg/cluster/k8sres.go
@@ -769,7 +769,7 @@ func (c *Cluster) generateSpiloPodEnvVars(uid types.UID, spiloConfiguration stri
envVars = append(envVars, customPodEnvVarsList...)
}

if c.OpConfig.WALES3Bucket != "" {
if c.OpConfig.WALES3Bucket != "" && enableWALArchiving {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm mistaken, not setting WAL_S3_BUCKET will not enable physical backups as well (https://github.com/zalando/spilo/blob/2.0-p7/postgres-appliance/scripts/configure_spilo.py#L862) so I think enableWALArchiving should be more generic.

Copy link
Contributor

Choose a reason for hiding this comment

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

And shouldn't we do the same for c.OpConfig.WALGSBucket as well?

@machine424
Copy link
Contributor

How can I help to get this merged? Thanks.

@pawanku2
Copy link

Hi , Is this PR merged?

@ibotty
Copy link

ibotty commented Sep 7, 2021

What's the status on that PR? It looks very useful.

@modzilla99
Copy link

Any news?

@FxKu
Copy link
Member

FxKu commented Apr 13, 2022

With #1794 merged we can close this PR. With the next release you will be able to disable archiving by overwriting WAl location to an empty string.

@FxKu FxKu closed this Apr 13, 2022
@FxKu
Copy link
Member

FxKu commented Apr 13, 2022

Thanks again @spohner for bringing it up. It's def. useful if you can turn off archiving for single clusters, but not all. Hope the new release will help you then.

@spohner
Copy link
Contributor Author

spohner commented Apr 19, 2022

Great news! Thanks.

@Menzorg
Copy link
Contributor

Menzorg commented Apr 19, 2022 •
edited

Thanks a lot!

@eberlep eberlep mentioned this pull request Aug 19, 2022
Open
@mug3n451
Copy link

mug3n451 commented Jul 7, 2023

Hi,
i'm trying to disable the WAL in the way described this thread, but even if i set, as a env in the postgresql's cluster manifest, WAL_S3_BUCKET to empty string, the wal at the first installtion is enabled and work.
May i do something wrong?
thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FxKu FxKu FxKu left review comments

@machine424 machine424 machine424 left review comments

@avaczi avaczi Awaiting requested review from avaczi

@CyberDem0n CyberDem0n Awaiting requested review from CyberDem0n

@erthalion erthalion Awaiting requested review from erthalion

@Jan-M Jan-M Awaiting requested review from Jan-M Jan-M is a code owner

@RafiaSabih RafiaSabih Awaiting requested review from RafiaSabih

@sdudoladov sdudoladov Awaiting requested review from sdudoladov sdudoladov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet