added: sync standby name, additional named replication slot#410
Conversation
|
This patch allows us to use additional replication slots for backup (RPO=0). For example, we can implement two options for working with a Barman. Classic Stolon Architecture and Barman Backup
Barman offers 2 options for backup and archiving. Traditionally, Barman has supported standard WAL file shipping through PostgreSQL’s archive_command (usually via rsync/SSH). With this method, WAL files are archived only when PostgreSQL switches to a new WAL file. To keep it simple, this normally happens every 16MB worth of data changes. Synchronous WAL streamingThis feature is available only from PostgreSQL 9.5 and above. Barman can also reduce the Recovery Point Objective to zero, by collecting the transaction WAL files like a synchronous standby server would. Stolon Architecture with 2 Nodes and Barman as 3rd Nodes
|
There was a problem hiding this comment.
@saranhada @Vadim0908 thanks for you PR and detailed explanation. Comments inline. There's also the need for some integration tests.
But I don't consider this as a fix for #330:
This PR will just add additional sync standbys but, if the internal good standbys are less then minSynchronousStandbys, than a fakesyncstandby will be added anyway. And I won't change this behavior because if you define minSynchronousStandbys stolon must ensure that there're at least minSynchronousStandbys synchronous stanbys and these stanbys must be controlled by stolon (not external).
The fix for #330 will be the ability to not make stolon add fakesyncstanbys when there less than minSynchronousStandbys good keepers. I haven't tests this and it should be better discussed but probably just defining minSynchronousStandbys to 0 will make this happen without any code change (or if not a PR, different than this, should be submitted to discuss this).
| log.Errorw("failed to create replication slot", "slotName", backupSlotName, zap.Error(err)) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
won't this drop and recreate the replication slot at every loop?
| // warning reloading the parameters) with postgresql < 9.6 | ||
| if len(synchronousStandbys) > 1 { | ||
| parameters["synchronous_standby_names"] = fmt.Sprintf("%d (%s)", len(synchronousStandbys), strings.Join(synchronousStandbys, ",")) | ||
| if syncStandbyNumber > 1 { |
There was a problem hiding this comment.
Probably here there's some logic that needs to be detailed in a comment. If I got it right you want to define the additionalsyncstandbynames only if there's at least one defined synchronous standby?
I won't do all of this here. This is something that should be handled by the sentinel. So, inside the sentinel I'll just add the additional synchronous standby in db.Spec.SynchronousStandby.
| | synchronousReplication | use synchronous replication between the master and its standbys | no | bool | false | | ||
| | minSynchronousStandbys | minimum number of required synchronous standbys when synchronous replication is enabled (only set this to a value > 1 when using PostgreSQL >= 9.6) | no | uint16 | 1 | | ||
| | maxSynchronousStandbys | maximum number of required synchronous standbys when synchronous replication is enabled (only set this to a value > 1 when using PostgreSQL >= 9.6) | no | uint16 | 1 | | ||
| | additionalSyncStandbyNames| additional names for possible standbys not included into stolon cluster, but used as replication destinations | mo | string | null | |
There was a problem hiding this comment.
Will do this an []string or you'll have to explicitly define that they should be comma separated and parse/validate them.
| | maxSynchronousStandbys | maximum number of required synchronous standbys when synchronous replication is enabled (only set this to a value > 1 when using PostgreSQL >= 9.6) | no | uint16 | 1 | | ||
| | additionalSyncStandbyNames| additional names for possible standbys not included into stolon cluster, but used as replication destinations | mo | string | null | | ||
| | additionalWalSenders | number of additional wal_senders in addition to the ones internally defined by stolon, useful to provide enough wal senders for external standbys (changing this value requires an instance restart) | no | uint16 | 5 | | ||
| | additionalReplicationSlotName | name of the additional replication slot which is always created by the master postgres instance | no | string | null | |
There was a problem hiding this comment.
Cannot be this a []string as above so users can define multiple additional replication slots?
| } | ||
| if s.AdditionalReplicationSlotName == nil { | ||
| s.AdditionalReplicationSlotName = StringP(DefaultReplicationSlotName) | ||
| } |
There was a problem hiding this comment.
After making them a []string the values should be validated to check that they're valid sync standby names and replication slot names.
There was a problem hiding this comment.
AdditionalReplicationSlotName - is the name of a single replication slot, not an array
There was a problem hiding this comment.
yes, I was asking to make it an array so multiple additional replication slots can be defined like done for additionalSyncStandbyNames.
There was a problem hiding this comment.
Simon, in our task we need one definite replication slot. But if you insist we can extend the basic idea. Do you?
There was a problem hiding this comment.
@saranhada yes, since we are providing a new option to define an additional replication slot lets make it useful for everyone giving the ability to define N additional replication slots if needed.
| db.Spec.FollowConfig.StandbySettings = clusterSpec.StandbySettings | ||
| } | ||
| db.Spec.AdditionalWalSenders = *clusterSpec.AdditionalWalSenders | ||
| db.Spec.AdditionalSyncStandbyNames = *clusterSpec.AdditionalSyncStandbyNames |
There was a problem hiding this comment.
I'll remove this and just add the additional synchronous standbys in db.Spec.SynchronousStandby.
| // addition to the ones internally defined by stolon | ||
| AdditionalWalSenders uint16 `json:"additionalWalSenders"` | ||
|
|
||
| AdditionalSyncStandbyNames string `json:"additionalSyncStandbyNames"` |
| // addition to the ones internally defined by stolon | ||
| AdditionalWalSenders *uint16 `json:"additionalWalSenders"` | ||
|
|
||
| AdditionalSyncStandbyNames *string `json:"additionalSyncStandbyNames"` |
| AdditionalWalSenders *uint16 `json:"additionalWalSenders"` | ||
|
|
||
| AdditionalSyncStandbyNames *string `json:"additionalSyncStandbyNames"` | ||
| AdditionalReplicationSlotNames *string `json:"additionalReplicationSlotNames"` |
| synchronousStandbys = append(synchronousStandbys, syncStandByName) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I honestly can't understand the logic here. As I tried to explain in my previous comment: https://github.com/sorintlab/stolon/pull/410/files#r161692823 just drop this and let the sentinel add the AdditionalSyncStandbyNames to the db.Spec.SynchronousStandbys.
| db.Spec.FollowConfig.StandbySettings = clusterSpec.StandbySettings | ||
| } | ||
| db.Spec.AdditionalWalSenders = *clusterSpec.AdditionalWalSenders | ||
| db.Spec.AdditionalSyncStandbyNames = clusterSpec.AdditionalSyncStandbyNames |
There was a problem hiding this comment.
Don't define db.Spec.AdditionalSyncStandbyNames but just add the additional synchronous standbys to db.Spec.SynchronousStandby.
There was a problem hiding this comment.
@sgotti We want these additional standbys not to be taken into account here keeper.go:303
parameters["synchronous_standby_names"] = fmt.Sprintf("%d (%s)", syncStandbyNumber, strings.Join(synchronousStandbys, ","))
} else {
parameters["synchronous_standby_names"] = strings.Join(synchronousStandbys, ",")
}
There was a problem hiding this comment.
@saranhada I don't understand why. As the name states they are AdditionalSyncStandbyNames so they should be added to the list of "synchronous_standby_names" no?
There was a problem hiding this comment.
@saranhada If you want them only as a "fallback" standby when one or more keeper standby isn't working then that's another story and it should be handled differently since this will break the current concistency logic that uses a fake standby as this change will skip it.
As I explained previously these are 2 distinct changes. One is to set additional sync standbys (not replacement standbys) that will be counted in the number of requires sync standbys. Another is to disable sync repl when you don't have enough standbys.
There was a problem hiding this comment.
@sgotti if I add the additional standby names into db.Spec.SynchronousStandby then the provided piece of will be generated like (for instance) = "2 (stolon_fdg4gs, add_name)", and this is what I want to avoid. We want to implement additional synchronous standbys for needs of backups as it was mentioned on the schema (before). It is supposed that PgBarman will serve the required needs. Thus we add PgBarman into 2 nodes stolon cluster with enabled sync replication. In a moment the master fails the standby keeper will be promoted into new Master with sync replication still enabled. The single stolon node will still operate and the PgBarman will be used as sync standby destination.
Later when a new 2nd node of stolon is restored/created the cluster will be switched to use it. But PgBarman will serve the needs of sync backup.
|
@saranhada Looks like you have two requirements: a. You don't want that a primary blocks when there are less than Note that this isn't #330 (deactivate synchronous replication for cluster consisting of 2 nodes in case the slave fails). and b. You want an external synchronous standby for backups with rpo = 0 but only when there are less than But I'm quite sure they can be summed in only one requirement:
The main issue with your implementation is that you can't control all of this using the FIRST method of the This PR will define for the primary instance something like this: This means that if the standby keeper If in a small time window a standby dies then the primary dies and the failed standby comes back before the sentinel notices this, it may happen that the standby is elected as master also if it's not in sync with the dead primary causing the loss of all the changes contained in the missed wal records. That's the reason why we are forced to have FIRST value equal to the total number sync standbys. Will open a PR to document it better. That's the reason why I was asking to just add the Proposed implementationSo the starting point is that it must be the sentinel to control all of this. And let's change the option name to some more clear like A possible solution is to handle all of this in the sentinel (like we already do adding a fakestandby when there aren't enough keeper to force the primary to block): if the sentinel detects that the good standbys are less than So, in a two node cluster, you'll have Of course the switch isn't instantaneus as if it's managed by the postgres instance but will permit the cluster to unblock after some seconds/minutes. |
|
@sgotti I'm agree the name ExternalFallbackSyncStandbys sounds good. It really reflects the idea of the backup synchronous standbys. Also I have reviewed the stolon source code again, of course I'm not an deep expert in it. You proposed to migrate the logic into sentinel. I think I see a piece of code can be responsible for the creation of the required configuration. It seems to start here sentinel.go:952 => if !masterOK { I have the following proposal:
How do you think? |
|
@saranhada Without testing a start to see if it works or there're some corner cases could be:
|
|
@sgotti Please see the code in keeper.go:275:
Every sync name which we add in sentinel will be prefixed by "stolon_" in keeper. I think we should implement the mentioned by you logic (add required the insufficient number of sync standby names) in keeper and take into consideration the MinSynchronousStandbys & MaxSynchronousStandbys |
So lets remove this from the keeper and make the sentinel to generate a Also remove all the prefix checks and removal in the keeper: Line 595 in 90a16f5 to report them as is (so also the external stanbys are reported). Perhaps there's something else to change in the sentinel. Let's see after these changes.
The keeper just tries to converge to the clusterdata db spec and for this reason must not implement any cluster logic. The clusterdata db spec is calculated by the sentinel so all of this logic pertains to the sentinel. |
|
@saranhada I rebased you branch on master and changed some parts to make it work. You can find it here (just one commit): https://github.com/sgotti/stolon/tree/saranhada-ADD_PARAMS_SYNC
Please take a look. You can just take my commit and put it in your branch and force push to update this PR (no need to open a new PR). |
|
This is completely unrelated to this issue....but does Barman work with Stolon and is configuration any different than a regular Barman Postgres setup? |
@marct83 can you please ask this question on the gitter channel so more people can help you and we don't pollute this PR? Thanks. |
|
No problem. Delete my comments. Thanks |
|
@saranhada I saw you imported my commit but also added another commit (2f25a26) that is adding unneeded things ( You said on gitter that you completed manual tests but before this PR can be merged we need some automatic tests. We need some unit tests in If you have some problems adding them I can do it for you in the next days when I have some time. |
|
@sgotti sorry for the late response. |
| } | ||
|
|
||
| if len(s.AdditionalReplicationSlotNames) > 0 { | ||
| rp := regexp.MustCompile("[_0-9a-z]+") |
There was a problem hiding this comment.
| symbolsNameFilter := regexp.MustCompile("[_0-9]") | ||
| if len(s.ExternalFallbackSyncStandbys) > 0 { | ||
| correctSymbolsNameFilter := regexp.MustCompile("[_0-9A-Za-z]") | ||
| for _, standbyName := range s.ExternalFallbackSyncStandbys { |
There was a problem hiding this comment.
Can you put this in its own function and add some unit tests to it?
|
@saranhada The logic related to the Additionally:
|
|
@sgotti do you suggest to split the PR into the following 2?
Regarding option 2 I understand the integration tests are required. But how about option 1? DO you require it? I think unit tests will be enough. |
yes.
Now the current But I'd say to just start splitting it in its own PR, then let's see which kind of tests are needed. I could write something myself to let you see how this test could be done. |
ghost
deleted the
|
Reopened |
ghost
reopened this
ghost
closed this
ghost
reopened this
ghost
closed this
ghost
reopened this
| for _, synchronousStandby := range db.Spec.SynchronousStandbys { | ||
| synchronousStandbys = append(synchronousStandbys, common.StolonName(synchronousStandby)) | ||
| } | ||
| numberSyncStandbys := len(synchronousStandbys) |
There was a problem hiding this comment.
If not do it then then the following code will result in forming of incorrect "synchronous_standby_names", it will will be like "2 (standby1, standby2)":
for _, synchronousStandby := range db.Spec.ExternalSynchronousStandbys {
synchronousStandbys = append(synchronousStandbys, synchronousStandby)
}
By saving the size of synchronousStandbys we are avoiding the for compilation of the incorrect value
There was a problem hiding this comment.
2 (standby1, standby2) is correct. As the comment below in the code says all the provided synchronous standbys must be synchronous. This is what I explained previously here: #410 (comment)
There was a problem hiding this comment.
@sgotti I'm just wanted to notify you that we are currently considering your point of view. Will return to you in a couple of days....
| // master. And choosing the non synchronous one will cause the loss of | ||
| // the transactions contained in the wal records not transmitted. | ||
| if len(synchronousStandbys) > 1 { | ||
| if numberSyncStandbys > 1 { |
| } | ||
| func (p dbSlice) Swap(i, j int) { | ||
| p[i], p[j] = p[j], p[i] | ||
| } |
| log.Debugf("synchronousStandbys not changed", "masterDB", masterDB.UID, "prevSynchronousStandbys", prevSynchronousStandbys, "synchronousStandbys", synchronousStandbys) | ||
| } | ||
|
|
||
| // If there're not enough real synchronous standbys add a fake synchronous standby because we have to be strict and make the master block transactions until MinSynchronousStandbys real standbys are available |
There was a problem hiding this comment.
This comment shouldn't be removed.
| | maxSynchronousStandbys | maximum number of required synchronous standbys when synchronous replication is enabled (only set this to a value > 1 when using PostgreSQL >= 9.6) | no | uint16 | 1 | | ||
| | additionalWalSenders | number of additional wal_senders in addition to the ones internally defined by stolon, useful to provide enough wal senders for external standbys (changing this value requires an instance restart) | no | uint16 | 5 | | ||
| | additionalMasterReplicationSlots | a list of additional replication slots to be created on the master postgres instance. Replication slots not defined here will be dropped from the master instance (i.e. manually created replication slots will be removed). | no | []string | null | | ||
| | externalFallbackSyncStandbys | a list of additional names for possible standbys not included into stolon cluster, but used as replication destinations. | no | []string | null | |
There was a problem hiding this comment.
The description doesn't look very clear. I'll prefer something more detailed and also document this in https://github.com/sorintlab/stolon/blob/master/doc/syncrepl.md
| } | ||
|
|
||
| func validateExternalSyncStandby(externalSyncStandbyName string) error { | ||
| symbolsNameFilter := regexp.MustCompile("[_0-9]") |
There was a problem hiding this comment.
please add an unit test to this function.
| if filteredSymbolsOnlyName := symbolsNameFilter.ReplaceAllString(externalSyncStandbyName, ""); len(filteredSymbolsOnlyName) == 0 { | ||
| return fmt.Errorf("external fallback synchronous standby name \"%s\" has an incorrect format, every name should start with a symbol", externalSyncStandbyName) | ||
| } | ||
|
|
There was a problem hiding this comment.
the above code look too much complicated. The regexp should already catch trailing spaces and \n.
|
@sgotti I fixed your last comment. I'm agree with your logic. But there is one question left. Lets consider a stolon cluster:
In this cluster I kill "standby keeper", but I see that master keeper doesn't rebuild synchronous_standby_names attribute. At leaves it as untouched. May I ask you to point a piece code which is responsible for the specified scenario? |
Perhaps I lost something but wasn't the purpose of this patch to do this? And this should be done here: https://github.com/sorintlab/stolon/pull/410/files#diff-9961d4546ef09e0152c558e820699523R1324 When the sentinel declares the standby as not good it'll catch this code and add an external synchronous standby. |
|
Hi @sgotti, can we continue the conversation on this PR? |
|
@saranhada yes |
3 participants
This pull request relates to the stream #330
The ideas of the 2 new parameters:
The additional synchronous name will be used in 2 nodes synchronous stolon cluster. If the master or standby fails then postgres will start using additional synchronous name for the synchronous replication, and therefore the stolon cluster (now it consists of the one survived single node) will continue operate.
As the value for "AdditionalSyncStandbyNames" parameter we want to use PgBarman's application name and since it requires exact replication slot we have implemented the "AdditionalReplicationSlotName" parameter
From the consistency point of view the schema seems to be ok because in a moment of a failure the PgBarman will be engaged as the synchronous destination and the data will be saved by the additional synchronous standby (e.g. PgBarman).
The pull request is the continue of the discussion: #330