Additional replication slots #434

ghost · 2018-02-08T21:38:24Z

@sgotti please review the pull request. If it is ok then let me create auto tests. This PR is the continuance of PR #410 started from the scratch

sgotti

@saranhada Thanks for the PR. First review pass.

sgotti · 2018-02-09T08:24:38Z

pkg/cluster/cluster.go

@@ -397,6 +399,23 @@ func (os *ClusterSpec) Validate() error {
 	if s.InitMode == nil {
 		return fmt.Errorf("initMode undefined")
 	}
+	if len(s.AdditionalReplicationSlotNames) > 0 {


no need to check the length > 0. The range will just take care of it.

sgotti · 2018-02-09T08:29:04Z

pkg/cluster/cluster.go

+		symbolsNameFilter := regexp.MustCompile("[_0-9]")
+
+		for _, replSlotName := range s.AdditionalReplicationSlotNames {
+			if trimmedSlotName := strings.TrimSpace(replSlotName); len(trimmedSlotName) == 0 ||


I think you can simplify it:

Check using using IsValidReplSlotName: https://github.com/sorintlab/stolon/blob/master/pkg/postgresql/utils.go#L328 ?

Check that the replslotname isn't a reserved stolon name using IsStolonName: https://github.com/sorintlab/stolon/blob/master/common/common.go#L65

sgotti · 2018-02-09T08:29:33Z

pkg/cluster/cluster.go

@@ -397,6 +399,23 @@ func (os *ClusterSpec) Validate() error {
 	if s.InitMode == nil {
 		return fmt.Errorf("initMode undefined")
 	}
+	if len(s.AdditionalReplicationSlotNames) > 0 {


please put this in its own function so we can add unit tests.

sgotti · 2018-02-09T08:30:01Z

pkg/cluster/cluster.go

@@ -529,7 +548,8 @@ type DBSpec struct {
 	UsePgrewind bool `json:"usePgrewind,omitempty"`
 	// AdditionalWalSenders defines the number of additional wal_senders in
 	// addition to the ones internally defined by stolon
-	AdditionalWalSenders uint16 `json:"additionalWalSenders"`
+	AdditionalWalSenders           uint16   `json:"additionalWalSenders"`
+	AdditionalReplicationSlotNames []string `json:"additionalReplicationSlotNames"`


missing comment

sgotti · 2018-02-09T08:30:51Z

cmd/keeper/keeper.go

+	}
+
+	// drop unnecessary slots
+	if len(slotsToDrop) > 0 {


no need to check the length > 0. The range will just take care of it.

sgotti · 2018-02-09T08:40:20Z

cmd/keeper/keeper.go

+	// create required slots
+	if len(slotsToCreate) > 0 {
+		for _, createSlotName := range slotsToCreate {
+			log.Infow("Create slot", "createSlotName", createSlotName)


log.Infow("creating replication slot", "createSlotName", createSlotName)

sgotti · 2018-02-09T08:44:08Z

cmd/keeper/keeper.go

+		if !slotShouldSurvive { // slot not detected as additional, so schedule the removal
+			slotsToDrop = append(slotsToDrop, removeCandidateSlot)
+		}
+	}


This could be simplified in (not tested):

for _, rs := range notStolonSlots { if !util.StringInSlice(addonSlotNames, replSlot) { slotsToDrop = append(slotsToDrop, rs) } }

sgotti · 2018-02-09T08:50:37Z

cmd/keeper/keeper.go

+		if !slotExists {
+			slotsToCreate = append(slotsToCreate, strings.TrimSpace(addReplicationSlotName))
+		}
+	}


This could be simplified in (not tested):

for _, rs := range addonSlotNames { if !util.StringInSlice(notStolonSlots, replSlot) { slotsToCreate = append(slotsToCreate, rs) } }

sgotti · 2018-02-09T08:56:37Z

cmd/sentinel/sentinel.go

@@ -400,6 +400,7 @@ func (s *Sentinel) setDBSpecFromClusterSpec(cd *cluster.ClusterData) {
 			db.Spec.FollowConfig.StandbySettings = clusterSpec.StandbySettings
 		}
 		db.Spec.AdditionalWalSenders = *clusterSpec.AdditionalWalSenders
+		db.Spec.AdditionalReplicationSlotNames = clusterSpec.AdditionalReplicationSlotNames


This isn't needed and misleading since it's adding this to all the clusterdata dbs.

Since the replication slots are created only on the master instance (primary postgres instance or master postgres standby instance in a Standby stolon cluster) the keeper could just read the clusterspec.

sgotti · 2018-02-09T08:57:47Z

doc/cluster_spec.md

@@ -25,6 +25,7 @@ Some options in a running cluster specification can be changed to update the des
 | 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                                                                                                                                   |
 | 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                                                                                                                                   |
+| additionalReplicationSlotNames | a list of additional replication slots which are always created by the master postgres instance                                                                                                                                                                                                                                                                                                                                                                              | no                        | []string          | null                                                                                                                                |


a list of additional replication slots to be created on the master postgres instance. Replication slots not defined here will be dropped.

sgotti · 2018-02-13T09:23:33Z

cmd/keeper/keeper.go

@@ -842,6 +835,53 @@ func (p *PostgresKeeper) updateReplSlots(dbLocalState *DBLocalState, followersUI
 	return nil
 }

+func (p *PostgresKeeper) recreateAddonReplicationSlots(existingReplicationSlots []string, addonSlotNames []string) error {


s/recreateAddonReplicationSlots/updateAdditionalReplSlots/

sgotti · 2018-02-13T09:24:16Z

cmd/keeper/keeper.go

+		log.Infow("Dropping replication slot", "dropSlotName", dropSlotName)
+		if err := p.pgm.DropReplicationSlot(dropSlotName); err != nil {
+			log.Errorw("Failed to drop replication slot", "slotName", dropSlotName, zap.Error(err))
+			return err


ignore dropping errors like done in updateReplSlot since they can be in use and this will fail.

sgotti · 2018-02-13T09:25:40Z

cmd/keeper/keeper.go

+
+	// create required slots
+	for _, createSlotName := range slotsToCreate {
+		log.Infow("Creatint replication slot", "createSlotName", createSlotName)


log.Infow("creating replication slot", "slotName", createSlotName)

sgotti · 2018-02-13T09:25:55Z

cmd/keeper/keeper.go

+
+	// drop unnecessary slots
+	for _, dropSlotName := range slotsToDrop {
+		log.Infow("Dropping replication slot", "dropSlotName", dropSlotName)


Don't use capital letters in logs.

sgotti

There were some unhandled comments from previous review, readded them.

sgotti · 2018-02-13T09:26:02Z

cmd/keeper/keeper.go

+	for _, createSlotName := range slotsToCreate {
+		log.Infow("Creatint replication slot", "createSlotName", createSlotName)
+		if err := p.pgm.CreateReplicationSlot(createSlotName); err != nil {
+			log.Errorw("Failed to create replication slot", "slotName", createSlotName, zap.Error(err))


Don't use capital letters in logs.

sgotti · 2018-02-13T09:27:12Z

cmd/keeper/keeper.go

+
+	// drop unnecessary slots
+	for _, dropSlotName := range slotsToDrop {
+		log.Infow("Dropping replication slot", "dropSlotName", dropSlotName)


log.Infow("dropping replication slot", "slotName", dropSlotName)

sgotti · 2018-02-13T09:27:49Z

cmd/keeper/keeper.go

+					return
+				}
+
+				if err = p.updateReplSlots(currentReplicationSlots, dbls, followersUIDs); err != nil {
 					log.Errorw("error updating replication slots", zap.Error(err))
 					return
 				}


you should call updateAdditionalReplSlots also here

sgotti · 2018-02-13T09:28:14Z

pkg/cluster/cluster.go

@@ -224,7 +225,8 @@ type ClusterSpec struct {
 	MaxSynchronousStandbys *uint16 `json:"maxSynchronousStandbys,omitempty"`
 	// AdditionalWalSenders defines the number of additional wal_senders in
 	// addition to the ones internally defined by stolon
-	AdditionalWalSenders *uint16 `json:"additionalWalSenders"`
+	AdditionalWalSenders           *uint16  `json:"additionalWalSenders"`
+	AdditionalReplicationSlotNames []string `json:"additionalReplicationSlotNames"`


missing comment

sgotti · 2018-02-13T09:29:57Z

pkg/cluster/cluster.go

+		return nil
+	}
+
+	rp := regexp.MustCompile("[_0-9a-z]+")


put it in a global var like done here: https://github.com/sorintlab/stolon/blob/master/pkg/postgresql/utils.go#L39

sgotti · 2018-02-13T09:31:39Z

pkg/cluster/cluster.go

+
+	for _, replSlotName := range s.AdditionalReplicationSlotNames {
+		if trimmedSlotName := strings.TrimSpace(replSlotName); len(trimmedSlotName) == 0 ||
+			len(rp.ReplaceAllString(trimmedSlotName, "")) > 0 ||


isn't utils.IsValidReplSlotName(replSlotName) enough? I don't see the need to trim space, \n etc... it should already be catched by it. If it's not enough then utils.IsValidReplSlotName should be fixed since the required check belong there.

I don't see the check for IsStolonName to error on repl slots starting with _stolon see my previous review comment: this function should just call two functions: #434 (comment)

An unit test is needed to prove that all works correctly.

sgotti · 2018-02-14T10:43:48Z

@saranhada to be speed up things, since I noticed some of my comments weren't addresses as I expected I changed your commit to fix it:

validateReplicationSlot is now a generic function that takes one replslot and only uses util.IsValidReplSlotName and IsStolonName, not trimming or other uneeded things: Additional replication slots #434 (comment)
added a test for validateReplicationSlot to check that it works correcly
fixed the dbSpec option since it wasn't needed. Now it's a generic DBSpec option where we ask the keeper which additional replslots to use. Please note (written in the TODOs) that now they are managed only on the master instance.
fixed missing logging after call to refreshReplicationSlots
simplified updateAdditionalReplSlots.
On postgres >= 10 there's the concept of temporary replication slot. We should ignore them.

You can see and take the commit here: 00007b2

sgotti · 2018-02-15T09:05:52Z

@saranhada I also added integration test to verify that all is working as expected. Please take a look and take the commit here: sgotti@c3ab6ea

* Add a cluster spec option `additionalMasterReplicationSlots` to let the user define additional custom replication slots on the stolon master instance. All the replication slots not defined here (i.e manually created) will be removed. So user should rely on this option when they need to create new replication slots. * Ignore temporary replication slots (PostgreSQL >= 10). * Add unit tests and integration tests.

sgotti · 2018-02-23T08:32:31Z

@saranhada Thanks! Merging.

Additional replication slots

sgotti requested changes Feb 9, 2018

View reviewed changes

sgotti reviewed Feb 13, 2018

View reviewed changes

sgotti requested changes Feb 13, 2018

View reviewed changes

sgotti approved these changes Feb 23, 2018

View reviewed changes

sgotti merged commit 15164a3 into sorintlab:master Feb 23, 2018

sgotti added a commit that referenced this pull request Feb 23, 2018

Merge pull request #434 from saranhada/ADD_REPLICATION_SLOTS

61b834d

Additional replication slots

sgotti added this to the 0.10.0 milestone Mar 21, 2018

Additional replication slots #434

Additional replication slots #434

Uh oh!

Conversation

ghost commented Feb 8, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgotti commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgotti commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgotti commented Feb 23, 2018

Uh oh!

Uh oh!

ghost commented Feb 8, 2018 •

edited by ghost

Loading

sgotti commented Feb 14, 2018 •

edited

Loading

sgotti commented Feb 15, 2018 •

edited

Loading