Skip to content

Commit

Permalink
Fixed an issue where the db-port option specified on the backup serve…
Browse files Browse the repository at this point in the history 
…r would not be properly passed to the remote unless it was from the first configured database.

Reported by Michael Vitale.
  • Loading branch information
@dwsteele
dwsteele committed Dec 10, 2016
1 parent 1a5fa92 commit b4884e5
Show file tree
Hide file tree
Showing 15 changed files with 75 additions and 13 deletions.
16 changes: 16 additions & 0 deletions doc/xml/release.xml
View file
Expand Up @@ -139,6 +139,10 @@

<release-bug-list>
<release-item>
<release-item-contributor-list>
<release-item-ideator id="kulkarni.nikhilchandra"/>
</release-item-contributor-list>

<p>Fixed an issue where options that were invalid for the specified command could be provided on the command-line without generating an error. The options were ignored and did not cause any change in behavior, but it did lead to some confusion. Invalid options will now generate an error.</p>
</release-item>

Expand All @@ -147,8 +151,20 @@
</release-item>

<release-item>
<release-item-contributor-list>
<release-item-ideator id="vondendriesch.adrian"/>
</release-item-contributor-list>

<p>Fixed an issue that prevented errors from being output to the console before the logging system was initialized, i.e. while parsing options. Error codes were still being returned accurately so this would not have made a process look like it succeeded when it did not.</p>
</release-item>

<release-item>
<release-item-contributor-list>
<release-item-ideator id="vitale.michael"/>
</release-item-contributor-list>

<p>Fixed an issue where the <br-option>db-port</br-option> option specified on the backup server would not be properly passed to the remote unless it was from the first configured database.</p>
</release-item>
</release-bug-list>

<release-feature-list>
Expand Down
1 change: 0 additions & 1 deletion lib/pgBackRest/Config/Config.pm
View file
Expand Up @@ -1520,7 +1520,6 @@ my %oOptionRule =
{
&CMD_BACKUP => true,
&CMD_CHECK => true,
&CMD_REMOTE => true,
},
},

Expand Down
15 changes: 15 additions & 0 deletions lib/pgBackRest/Protocol/Protocol.pm
View file
Expand Up @@ -124,6 +124,7 @@ sub protocolGet
my $strOptionConfig = OPTION_BACKUP_CONFIG;
my $strOptionHost = OPTION_BACKUP_HOST;
my $strOptionUser = OPTION_BACKUP_USER;
my $strOptionDbPort = undef;
my $strOptionDbSocketPath = undef;

if ($strRemoteType eq DB)
Expand All @@ -134,6 +135,14 @@ sub protocolGet
$strOptionUser = optionIndex(OPTION_DB_USER, $iRemoteIdx);
}

# Db socket is not valid in all contexts (restore, for instance)
if (optionValid(optionIndex(OPTION_DB_PORT, $iRemoteIdx)))
{
$strOptionDbPort =
optionSource(optionIndex(OPTION_DB_PORT, $iRemoteIdx)) eq SOURCE_DEFAULT ?
undef : optionGet(optionIndex(OPTION_DB_PORT, $iRemoteIdx));
}

# Db socket is not valid in all contexts (restore, for instance)
if (optionValid(optionIndex(OPTION_DB_SOCKET_PATH, $iRemoteIdx)))
{
Expand All @@ -156,8 +165,14 @@ sub protocolGet
&OPTION_TYPE => {value => $strRemoteType},
&OPTION_LOG_PATH => {},
&OPTION_LOCK_PATH => {},
&OPTION_DB_PORT => {value => $strOptionDbPort},
&OPTION_DB_SOCKET_PATH => {value => $strOptionDbSocketPath},

# ??? Not very pretty but will work until there is nicer code in commandWrite to handle this case. It
# doesn't hurt to pass these params but it's messy and distracting for debugging.
optionIndex(OPTION_DB_PORT, 2) => {},
optionIndex(OPTION_DB_SOCKET_PATH, 2) => {},

# Set protocol options explicitly so values are not picked up from remote config files
&OPTION_BUFFER_SIZE => {value => optionGet(OPTION_BUFFER_SIZE)},
&OPTION_COMPRESS_LEVEL => {value => optionGet(OPTION_COMPRESS_LEVEL)},
Expand Down
6 changes: 3 additions & 3 deletions test/expect/backup-full-005.log
View file
Expand Up @@ -34,7 +34,7 @@ start-fast=y
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db

[global]
Expand Down Expand Up @@ -93,7 +93,7 @@ start-fast=y
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db

[db:restore]
Expand Down Expand Up @@ -144,7 +144,7 @@ start-fast=y
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db

[db:restore]
Expand Down
6 changes: 3 additions & 3 deletions test/expect/backup-full-006.log
View file
Expand Up @@ -29,7 +29,7 @@ repo-path=[TEST_PATH]/db-standby/repo
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db
db2-cmd=[BACKREST-BIN]
db2-config=[TEST_PATH]/db-master/pgbackrest.conf
Expand Down Expand Up @@ -88,7 +88,7 @@ repo-path=[TEST_PATH]/db-standby/repo
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db
db2-cmd=[BACKREST-BIN]
db2-config=[TEST_PATH]/db-master/pgbackrest.conf
Expand Down Expand Up @@ -139,7 +139,7 @@ repo-path=[TEST_PATH]/db-standby/repo
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db
db2-cmd=[BACKREST-BIN]
db2-config=[TEST_PATH]/db-master/pgbackrest.conf
Expand Down
4 changes: 4 additions & 0 deletions test/expect/backup-full-007.log
View file
Expand Up @@ -88,6 +88,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down Expand Up @@ -166,6 +167,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down Expand Up @@ -232,6 +234,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down Expand Up @@ -366,6 +369,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down
2 changes: 2 additions & 0 deletions test/expect/backup-full-008.log
View file
Expand Up @@ -31,6 +31,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down Expand Up @@ -75,6 +76,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down
2 changes: 2 additions & 0 deletions test/expect/backup-full-009.log
View file
Expand Up @@ -36,6 +36,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down Expand Up @@ -86,6 +87,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down
2 changes: 2 additions & 0 deletions test/expect/backup-full-010.log
View file
Expand Up @@ -35,6 +35,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down Expand Up @@ -83,6 +84,7 @@ db-cmd=[BACKREST-BIN]
db-config=[TEST_PATH]/db-master/pgbackrest.conf
db-host=db-master
db-path=[TEST_PATH]/db-master/db/base
db-port=[PORT-1]
db-user=[USER-2]

[global]
Expand Down
12 changes: 9 additions & 3 deletions test/expect/backup-full-011.log
View file
Expand Up @@ -29,7 +29,7 @@ repo-path=[TEST_PATH]/backup/repo
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db

[global]
Expand All @@ -52,11 +52,13 @@ db1-cmd=[BACKREST-BIN]
db1-config=[TEST_PATH]/db-master/pgbackrest.conf
db1-host=db-master
db1-path=[TEST_PATH]/db-master/db/base
db1-port=[PORT-1]
db1-user=[USER-2]
db2-cmd=[BACKREST-BIN]
db2-config=[TEST_PATH]/db-standby/pgbackrest.conf
db2-host=db-standby
db2-path=[TEST_PATH]/db-standby/db/base
db2-port=[PORT-2]
db2-user=[USER-2]

[global]
Expand Down Expand Up @@ -110,7 +112,7 @@ repo-path=[TEST_PATH]/backup/repo
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db

[db:restore]
Expand All @@ -135,11 +137,13 @@ db1-cmd=[BACKREST-BIN]
db1-config=[TEST_PATH]/db-master/pgbackrest.conf
db1-host=db-master
db1-path=[TEST_PATH]/db-master/db/base
db1-port=[PORT-1]
db1-user=[USER-2]
db2-cmd=[BACKREST-BIN]
db2-config=[TEST_PATH]/db-standby/pgbackrest.conf
db2-host=db-standby
db2-path=[TEST_PATH]/db-standby/db/base
db2-port=[PORT-2]
db2-user=[USER-2]

[global]
Expand Down Expand Up @@ -183,7 +187,7 @@ repo-path=[TEST_PATH]/backup/repo
-----------------------------------------------------------
[db]
db-path=[TEST_PATH]/db-standby/db/base
db-port=[PORT-1]
db-port=[PORT-2]
db-socket-path=[TEST_PATH]/db-standby/db

[db:restore]
Expand All @@ -208,11 +212,13 @@ db1-cmd=[BACKREST-BIN]
db1-config=[TEST_PATH]/db-master/pgbackrest.conf
db1-host=db-master
db1-path=[TEST_PATH]/db-master/db/base
db1-port=[PORT-1]
db1-user=[USER-2]
db2-cmd=[BACKREST-BIN]
db2-config=[TEST_PATH]/db-standby/pgbackrest.conf
db2-host=db-standby
db2-path=[TEST_PATH]/db-standby/db/base
db2-port=[PORT-2]
db2-user=[USER-2]

[global]
Expand Down
3 changes: 3 additions & 0 deletions test/expect/backup-synthetic-005.log
View file
Expand Up @@ -4260,3 +4260,6 @@ diff backup - protocol shutdown timeout (backup host)
P00 WARN: option retention-full is not set, the repository may run out of space
HINT: to retain full backups indefinitely (without warning), set option 'retention-full' to the maximum.
P00 TEST: PgBaCkReStTeSt-BACKUP-STOP-PgBaCkReStTeSt
P00 ERROR: [141]: remote process terminated on db-master host: unable to read line after 2 second(s)
P00 WARN: unable to shutdown protocol [141]: remote process terminated on db-master host: unable to read line after 2 second(s)
HINT: the process completed all operations successfully but protocol-timeout may need to be increased.
3 changes: 2 additions & 1 deletion test/lib/pgBackRestTest/Backup/BackupTest.pm
View file
Expand Up @@ -2118,7 +2118,8 @@ sub backupTestRun
OPTION_DEFAULT_RESTORE_SET, undef, \%oRemapHash, $bDelta, $bForce, $strType, $strTarget, $bTargetExclusive,
$strTargetAction, $strTargetTimeline, $oRecoveryHashRef, $strComment, $iExpectedExitStatus,
' --recovery-option=standby_mode=on' .
' --recovery-option="primary_conninfo=host=' . HOST_DB_MASTER . ' port=' . HOST_DB_PORT . ' user=replicator"');
' --recovery-option="primary_conninfo=host=' . HOST_DB_MASTER .
' port=' . $oHostDbMaster->dbPort() . ' user=replicator"');

$oHostDbStandby->clusterStart({bHotStandby => true});

Expand Down
12 changes: 12 additions & 0 deletions test/lib/pgBackRestTest/Backup/Common/HostBackupTest.pm
View file
Expand Up @@ -918,6 +918,12 @@ sub configCreate
$oParamHash{$strStanza}{optionIndex(OPTION_DB_USER, 1, $bForce)} = $oHostDb1->userGet();
$oParamHash{$strStanza}{optionIndex(OPTION_DB_CMD, 1, $bForce)} = $oHostDb1->backrestExe();
$oParamHash{$strStanza}{optionIndex(OPTION_DB_CONFIG, 1, $bForce)} = $oHostDb1->backrestConfig();

# Port can't be configured for a synthetic host
if (!$self->synthetic())
{
$oParamHash{$strStanza}{optionIndex(OPTION_DB_PORT, 1, $bForce)} = $oHostDb1->dbPort();
}
}

$oParamHash{$strStanza}{optionIndex(OPTION_DB_PATH, 1, $bForce)} = $oHostDb1->dbBasePath();
Expand All @@ -929,6 +935,12 @@ sub configCreate
$oParamHash{$strStanza}{optionIndex(OPTION_DB_CMD, 2)} = $oHostDb2->backrestExe();
$oParamHash{$strStanza}{optionIndex(OPTION_DB_CONFIG, 2)} = $oHostDb2->backrestConfig();
$oParamHash{$strStanza}{optionIndex(OPTION_DB_PATH, 2)} = $oHostDb2->dbBasePath();

# Only test explicit ports on the backup server. This is so locally configured ports are also tested.
if (!$self->synthetic() && $self->nameTest(HOST_BACKUP))
{
$oParamHash{$strStanza}{optionIndex(OPTION_DB_PORT, 2)} = $oHostDb2->dbPort();
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/lib/pgBackRestTest/Backup/Common/HostDbTest.pm
View file
Expand Up @@ -93,7 +93,7 @@ sub new
$self->paramSet(HOST_PARAM_DB_BIN_PATH, $oHostGroup->paramGet(HOST_PARAM_DB_BIN_PATH));
$self->paramSet(HOST_PARAM_DB_VERSION, $strDbVersion);
$self->paramSet(HOST_PARAM_DB_SOCKET_PATH, $self->dbPath());
$self->paramSet(HOST_PARAM_DB_PORT, HOST_DB_PORT);
$self->paramSet(HOST_PARAM_DB_PORT, defined($$oParam{bStandby}) && $$oParam{bStandby} ? 6544 : 6543);

$self->paramSet(HOST_PARAM_DB_LOG_PATH, $self->testPath());
$self->paramSet(HOST_PARAM_DB_LOG_FILE, $self->dbLogPath() . '/postgresql.log');
Expand Down
2 changes: 1 addition & 1 deletion test/lib/pgBackRestTest/Common/LogTest.pm
View file
Expand Up @@ -358,7 +358,7 @@ sub regExpReplaceAll
$strLine = $self->regExpReplace($strLine, 'USER', '[^ ]+\@db\-master', '^[^\@]+');
$strLine = $self->regExpReplace($strLine, 'USER', '[\( ]{1}' . TEST_USER . '[\,\)]{1}', TEST_USER);

$strLine = $self->regExpReplace($strLine, 'PORT', 'db-port=[0-9]+', '[0-9]+$');
$strLine = $self->regExpReplace($strLine, 'PORT', 'db[1-9]{0,1}-port=[0-9]+', '[0-9]+$');

# Replace year when it falls on a single line when executing ls -1R
$strLine = $self->regExpReplace($strLine, 'YEAR', '^20[0-9]{2}$');
Expand Down

0 comments on commit b4884e5

Please sign in to comment.