Additional replication slots · Pull Request #434 · sorintlab/stolon

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

 	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

+		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

 	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

 	// 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

+	}
+
+	// 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

+	// 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

+		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

+		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

 			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

 | 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

 	return nil
 }

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


s/recreateAddonReplicationSlots/updateAdditionalReplSlots/

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

+		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

+
+	// 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

+
+	// 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

+	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

+
+	// 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

+				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

 	// 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

+		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

+
+	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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost commented Feb 8, 2018 •

edited by ghost

Loading

sgotti commented Feb 14, 2018 •

edited

Loading

sgotti commented Feb 15, 2018 •

edited

Loading