From b2f43b56014f3d43404a17531887a09a78eb0db1 Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 4 Apr 2017 21:17:19 -0400 Subject: [PATCH] Allow functions to accept optional parameters as a hash. Refactor File->list() and fileList() to accept optional parameters. --- doc/xml/release.xml | 10 +++++ lib/pgBackRest/Archive/ArchiveCommon.pm | 8 ++-- lib/pgBackRest/Archive/ArchiveInfo.pm | 15 ++++--- lib/pgBackRest/Archive/ArchivePush.pm | 6 +-- lib/pgBackRest/Backup.pm | 13 +++--- lib/pgBackRest/BackupInfo.pm | 2 +- lib/pgBackRest/Common/Log.pm | 45 ++++++++++++++++--- lib/pgBackRest/Expire.pm | 13 +++--- lib/pgBackRest/File.pm | 12 ++--- lib/pgBackRest/FileCommon.pm | 10 ++--- lib/pgBackRest/Info.pm | 8 ++-- lib/pgBackRest/Stanza.pm | 4 +- .../pgBackRestTest/Archive/ArchiveStopTest.pm | 6 ++- .../Common/Host/HostBackupTest.pm | 3 +- test/lib/pgBackRestTest/File/FileListTest.pm | 4 +- 15 files changed, 109 insertions(+), 50 deletions(-) diff --git a/doc/xml/release.xml b/doc/xml/release.xml index f7bcf4788..ee0715d07 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -172,6 +172,16 @@

Added validation of pgbackrest.conf to display warnings if options are not valid or are not in the correct section.

+ + + +

Allow functions to accept optional parameters as a hash.

+ + + +

Refactor File->list() and fileList() to accept optional parameters.

+ + diff --git a/lib/pgBackRest/Archive/ArchiveCommon.pm b/lib/pgBackRest/Archive/ArchiveCommon.pm index 8f2beca6b..5c6b8eb8e 100644 --- a/lib/pgBackRest/Archive/ArchiveCommon.pm +++ b/lib/pgBackRest/Archive/ArchiveCommon.pm @@ -299,7 +299,8 @@ sub walSegmentFind else { @stryTimelineMajor = $oFile->list( - PATH_BACKUP_ARCHIVE, $strArchiveId, '[0-F]{8}' . substr($strWalSegment, 0, 8), undef, true); + PATH_BACKUP_ARCHIVE, $strArchiveId, + {strExpression => '[0-F]{8}' . substr($strWalSegment, 0, 8), bIgnoreMissing => true}); } # Search each timelin/major path @@ -311,9 +312,10 @@ sub walSegmentFind # Get the name of the requested WAL segment (may have hash info and compression extension) push(@stryWalFileName, $oFile->list( PATH_BACKUP_ARCHIVE, "${strArchiveId}/${strTimelineMajor}", - "^${strWalSegmentFind}" . (walIsPartial($strWalSegment) ? "\\.partial" : '') . + {strExpression => + "^${strWalSegmentFind}" . (walIsPartial($strWalSegment) ? "\\.partial" : '') . "-[0-f]{40}(\\." . COMPRESS_EXT . "){0,1}\$", - undef, true)); + bIgnoreMissing => true})); } } while (@stryWalFileName == 0 && waitMore($oWait)); diff --git a/lib/pgBackRest/Archive/ArchiveInfo.pm b/lib/pgBackRest/Archive/ArchiveInfo.pm index 8fe44cc4a..664c12d42 100644 --- a/lib/pgBackRest/Archive/ArchiveInfo.pm +++ b/lib/pgBackRest/Archive/ArchiveInfo.pm @@ -250,7 +250,8 @@ sub reconstruct my $strInvalidFileStructure = undef; - my @stryArchiveId = fileList($self->{strArchiveClusterPath}, REGEX_ARCHIVE_DIR_DB_VERSION, 'forward', true); + my @stryArchiveId = fileList( + $self->{strArchiveClusterPath}, {strExpression => REGEX_ARCHIVE_DIR_DB_VERSION, bIgnoreMissing => true}); my %hDbHistoryVersion; # Get the db-version and db-id (history id) from the upper level directory names, e.g. 9.4-1 @@ -267,8 +268,9 @@ sub reconstruct my $strVersionDir = $strDbVersion . "-" . $iDbHistoryId; # Get the name of the first archive directory - my $strArchiveDir = - (fileList($self->{strArchiveClusterPath} . "/${strVersionDir}", REGEX_ARCHIVE_DIR_WAL, 'forward', true))[0]; + my $strArchiveDir = (fileList( + $self->{strArchiveClusterPath} . "/${strVersionDir}", + {strExpression => REGEX_ARCHIVE_DIR_WAL, bIgnoreMissing => true}))[0]; # Continue if any file structure or missing files info if (!defined($strArchiveDir)) @@ -278,9 +280,10 @@ sub reconstruct } # ??? Should probably make a function in ArchiveCommon - my $strArchiveFile = - (fileList($self->{strArchiveClusterPath} . "/${strVersionDir}/${strArchiveDir}", - "^[0-F]{24}(\\.partial){0,1}(-[0-f]+){0,1}(\\.$oFile->{strCompressExtension}){0,1}\$", 'forward', true))[0]; + my $strArchiveFile = (fileList( + $self->{strArchiveClusterPath} . "/${strVersionDir}/${strArchiveDir}", + {strExpression => "^[0-F]{24}(\\.partial){0,1}(-[0-f]+){0,1}(\\.$oFile->{strCompressExtension}){0,1}\$", + bIgnoreMissing => true}))[0]; # Continue if any file structure or missing files info if (!defined($strArchiveFile)) diff --git a/lib/pgBackRest/Archive/ArchivePush.pm b/lib/pgBackRest/Archive/ArchivePush.pm index 5d220c3fc..2e4fa8faa 100644 --- a/lib/pgBackRest/Archive/ArchivePush.pm +++ b/lib/pgBackRest/Archive/ArchivePush.pm @@ -185,7 +185,7 @@ sub walStatus my $bResult = false; # Find matching status files - my @stryStatusFile = fileList($strSpoolPath, '^' . $strWalFile . '\.(ok|error)$', undef, true); + my @stryStatusFile = fileList($strSpoolPath, {strExpression => '^' . $strWalFile . '\.(ok|error)$', bIgnoreMissing => true}); if (@stryStatusFile > 0) { @@ -272,7 +272,7 @@ sub readyList if (defined($self->{strSpoolPath})) { - foreach my $strOkFile (fileList($self->{strSpoolPath}, '\.ok$', undef, true)) + foreach my $strOkFile (fileList($self->{strSpoolPath}, {strExpression => '\.ok$', bIgnoreMissing => true})) { $strOkFile = substr($strOkFile, 0, length($strOkFile) - length('.ok')); $hOkFile->{$strOkFile} = true; @@ -281,7 +281,7 @@ sub readyList # Read the .ready files my $strWalStatusPath = "$self->{strWalPath}/archive_status"; - my @stryReadyFile = fileList($strWalStatusPath, '\.ready$'); + my @stryReadyFile = fileList($strWalStatusPath, {strExpression => '\.ready$'}); # Generate a list of new files my @stryNewReadyFile; diff --git a/lib/pgBackRest/Backup.pm b/lib/pgBackRest/Backup.pm index 64e839dc2..27b80470b 100644 --- a/lib/pgBackRest/Backup.pm +++ b/lib/pgBackRest/Backup.pm @@ -950,13 +950,14 @@ sub process # clocks. In practice this is most useful for making offline testing faster since it allows the wait after manifest build to # be skipped by dealing with any backup label collisions here. if (fileList($oFileLocal->pathGet(PATH_BACKUP_CLUSTER), - ($strType eq BACKUP_TYPE_FULL ? '^' : '_') . - timestampFileFormat(undef, $lTimestampStop) . - ($strType eq BACKUP_TYPE_FULL ? 'F' : '(D|I)$')) || + {strExpression => + ($strType eq BACKUP_TYPE_FULL ? '^' : '_') . timestampFileFormat(undef, $lTimestampStop) . + ($strType eq BACKUP_TYPE_FULL ? 'F' : '(D|I)$')}) || fileList($oFileLocal->pathGet(PATH_BACKUP_CLUSTER, PATH_BACKUP_HISTORY . '/' . timestampFormat('%4d', $lTimestampStop)), - ($strType eq BACKUP_TYPE_FULL ? '^' : '_') . - timestampFileFormat(undef, $lTimestampStop) . - ($strType eq BACKUP_TYPE_FULL ? 'F' : '(D|I)\.manifest\.' . $oFileLocal->{strCompressExtension}), undef, true)) + {strExpression => + ($strType eq BACKUP_TYPE_FULL ? '^' : '_') . timestampFileFormat(undef, $lTimestampStop) . + ($strType eq BACKUP_TYPE_FULL ? 'F' : '(D|I)\.manifest\.' . $oFileLocal->{strCompressExtension}), + bIgnoreMissing => true})) { waitRemainder(); $strBackupLabel = backupLabelFormat($strType, $strBackupLastPath, time()); diff --git a/lib/pgBackRest/BackupInfo.pm b/lib/pgBackRest/BackupInfo.pm index 4ad4e470d..eb6360a8f 100644 --- a/lib/pgBackRest/BackupInfo.pm +++ b/lib/pgBackRest/BackupInfo.pm @@ -209,7 +209,7 @@ sub reconstruct ); # Check for backups that are not in FILE_BACKUP_INFO - foreach my $strBackup (fileList($self->{strBackupClusterPath}, backupRegExpGet(true, true, true))) + foreach my $strBackup (fileList($self->{strBackupClusterPath}, {strExpression => backupRegExpGet(true, true, true)})) { my $strManifestFile = "$self->{strBackupClusterPath}/${strBackup}/" . FILE_MANIFEST; diff --git a/lib/pgBackRest/Common/Log.pm b/lib/pgBackRest/Common/Log.pm index fdbf7dbca..88dcd5bb4 100644 --- a/lib/pgBackRest/Common/Log.pm +++ b/lib/pgBackRest/Common/Log.pm @@ -124,7 +124,7 @@ sub logFileSet $bLogFileExists = -e $strFile ? true : false; $bLogFileFirst = true; - $hLogFile = fileOpen($strFile, O_WRONLY | O_CREAT | O_APPEND, '0660'); + $hLogFile = fileOpen($strFile, O_WRONLY | O_CREAT | O_APPEND); # Write out anything that was cached before the file was opened if (defined($strLogFileCache)) @@ -303,6 +303,7 @@ sub logDebugProcess # Process each parameter hash my $oParam = shift; + my $bOptionalBlock = false; # Strip the package name off strFunction if it's pgBackRest $strFunction =~ s/^pgBackRest[^\:]*\:\://; @@ -310,14 +311,43 @@ sub logDebugProcess while (defined($oParam)) { my $strParamName = $$oParam{name}; + my $bParamOptional = defined($oParam->{optional}) && $oParam->{optional}; + my $bParamRequired = !defined($oParam->{required}) || $oParam->{required}; my $oValue; + # If param is optional then the optional block has been entered + if ($bParamOptional) + { + if (defined($oParam->{required})) + { + confess &log(ASSERT, "cannot define 'required' for optional parameter '${strParamName}'"); + } + + $bParamRequired = false; + $bOptionalBlock = true; + } + + # Don't allow non-optional parameters once optional block has started + if ($bParamOptional != $bOptionalBlock) + { + confess &log(ASSERT, "non-optional parameter '${strParamName}' invalid after optional parameters"); + } + # Push the return value into the return value array if ($strType eq DEBUG_PARAM) { - if (defined($$oyParamRef[$iIndex])) + if ($bParamOptional) { - push(@oyResult, $$oyParamRef[$iIndex]); + $oValue = $$oyParamRef[$iIndex]->{$strParamName}; + } + else + { + $oValue = $$oyParamRef[$iIndex]; + } + + if (defined($oValue)) + { + push(@oyResult, $oValue); } else { @@ -327,7 +357,7 @@ sub logDebugProcess $oValue = $oyResult[@oyResult - 1]; - if (!defined($oValue) && (!defined($${oParam}{required}) || $${oParam}{required})) + if (!defined($oValue) && $bParamRequired) { confess &log(ASSERT, "${strParamName} is required in ${strFunction}"); } @@ -355,7 +385,6 @@ sub logDebugProcess if (!defined($$oParam{log}) || $$oParam{log}) { - # If the parameter is a hash but not blessed then represent it as a string # ??? This should go away once the inputs to logDebug can be changed if (ref($oValue) eq 'HASH' && !blessed($oValue)) @@ -378,7 +407,11 @@ sub logDebugProcess # Get the next parameter hash $oParam = shift; - $iIndex++; + + if (!$bParamOptional) + { + $iIndex++; + } } if (defined($strDetail) && $iIndex == 0) diff --git a/lib/pgBackRest/Expire.pm b/lib/pgBackRest/Expire.pm index d172d0053..29b9b5842 100644 --- a/lib/pgBackRest/Expire.pm +++ b/lib/pgBackRest/Expire.pm @@ -222,7 +222,8 @@ sub process $oBackupInfo->save(); # Remove backups from disk - foreach my $strBackup ($oFile->list(PATH_BACKUP_CLUSTER, undef, backupRegExpGet(true, true, true), 'reverse')) + foreach my $strBackup ($oFile->list( + PATH_BACKUP_CLUSTER, undef, {strExpression => backupRegExpGet(true, true, true), strSortOrder => 'reverse'})) { if (!$oBackupInfo->current($strBackup)) { @@ -263,7 +264,8 @@ sub process if ($iBackupTotal > 0) { my $oArchiveInfo = new pgBackRest::Archive::ArchiveInfo($oFile->pathGet(PATH_BACKUP_ARCHIVE), true); - my @stryListArchiveDisk = fileList($oFile->pathGet(PATH_BACKUP_ARCHIVE), REGEX_ARCHIVE_DIR_DB_VERSION, 'forward', true); + my @stryListArchiveDisk = fileList( + $oFile->pathGet(PATH_BACKUP_ARCHIVE), {strExpression => REGEX_ARCHIVE_DIR_DB_VERSION, bIgnoreMissing => true}); # Make sure the current database versions match between the two files if (!($oArchiveInfo->test(INFO_ARCHIVE_SECTION_DB, INFO_ARCHIVE_KEY_DB_VERSION, undef, @@ -397,7 +399,8 @@ sub process } # Get all major archive paths (timeline and first 32 bits of LSN) - foreach my $strPath ($oFile->list(PATH_BACKUP_ARCHIVE, $strArchiveId, REGEX_ARCHIVE_DIR_WAL)) + foreach my $strPath ($oFile->list( + PATH_BACKUP_ARCHIVE, $strArchiveId, {strExpression => REGEX_ARCHIVE_DIR_WAL})) { logDebugMisc($strOperation, "found major WAL path: ${strPath}"); $bRemove = true; @@ -431,8 +434,8 @@ sub process elsif ($strPath le substr($strArchiveExpireMax, 0, 16)) { # Look for files in the archive directory - foreach my $strSubPath ($oFile->list(PATH_BACKUP_ARCHIVE, - "${strArchiveId}/${strPath}", "^[0-F]{24}.*\$")) + foreach my $strSubPath ($oFile->list( + PATH_BACKUP_ARCHIVE, "${strArchiveId}/${strPath}", {strExpression => "^[0-F]{24}.*\$"})) { $bRemove = true; diff --git a/lib/pgBackRest/File.pm b/lib/pgBackRest/File.pm index 104228d57..f80e4b867 100644 --- a/lib/pgBackRest/File.pm +++ b/lib/pgBackRest/File.pm @@ -966,9 +966,9 @@ sub list __PACKAGE__ . '->list', \@_, {name => 'strPathType'}, {name => 'strPath', required => false}, - {name => 'strExpression', required => false}, - {name => 'strSortOrder', default => 'forward'}, - {name => 'bIgnoreMissing', default => false} + {name => 'strExpression', optional => true}, + {name => 'strSortOrder', optional => true, default => 'forward'}, + {name => 'bIgnoreMissing', optional => true, default => false} ); # Set operation variables @@ -979,12 +979,14 @@ sub list if ($self->isRemote($strPathType)) { @stryFileList = $self->{oProtocol}->cmdExecute( - OP_FILE_LIST, [$strPathOp, $strExpression, $strSortOrder, $bIgnoreMissing]); + OP_FILE_LIST, + [$strPathOp, {strExpression => $strExpression, strSortOrder => $strSortOrder, bIgnoreMissing => $bIgnoreMissing}]); } # Run locally else { - @stryFileList = fileList($strPathOp, $strExpression, $strSortOrder, $bIgnoreMissing); + @stryFileList = fileList( + $strPathOp, {strExpression => $strExpression, strSortOrder => $strSortOrder, bIgnoreMissing => $bIgnoreMissing}); } # Return from function and log return values if any diff --git a/lib/pgBackRest/FileCommon.pm b/lib/pgBackRest/FileCommon.pm index 3fe095be7..f7fb2aa57 100644 --- a/lib/pgBackRest/FileCommon.pm +++ b/lib/pgBackRest/FileCommon.pm @@ -264,15 +264,15 @@ sub fileList $strPath, $strExpression, $strSortOrder, - $bIgnoreMissing + $bIgnoreMissing, ) = logDebugParam ( __PACKAGE__ . '::fileList', \@_, {name => 'strPath', trace => true}, - {name => 'strExpression', required => false, trace => true}, - {name => 'strSortOrder', default => 'forward', trace => true}, - {name => 'bIgnoreMissing', default => false, trace => true} + {name => 'strExpression', optional => true, trace => true}, + {name => 'strSortOrder', optional => true, default => 'forward', trace => true}, + {name => 'bIgnoreMissing', optional => true, default => false, trace => true}, ); # Working variables @@ -390,7 +390,7 @@ sub fileManifestRecurse } # Get a list of all files in the path (including .) - my @stryFileList = fileList($strPathRead, undef, undef, $iDepth != 0); + my @stryFileList = fileList($strPathRead, {bIgnoreMissing => $iDepth != 0}); unshift(@stryFileList, '.'); my $hFileStat = fileManifestList($strPathRead, \@stryFileList); diff --git a/lib/pgBackRest/Info.pm b/lib/pgBackRest/Info.pm index 6d096c7ff..a6b033f15 100644 --- a/lib/pgBackRest/Info.pm +++ b/lib/pgBackRest/Info.pm @@ -345,7 +345,7 @@ sub stanzaList # Run locally else { - my @stryStanza = $oFile->list(PATH_BACKUP, CMD_BACKUP, undef, undef, true); + my @stryStanza = $oFile->list(PATH_BACKUP, CMD_BACKUP, {bIgnoreMissing => true}); foreach my $strStanzaFound (@stryStanza) { @@ -389,14 +389,14 @@ sub stanzaList if ($oFile->exists(PATH_BACKUP, $strArchivePath)) { - my @stryWalMajor = $oFile->list(PATH_BACKUP, $strArchivePath, '^[0-F]{16}$'); + my @stryWalMajor = $oFile->list(PATH_BACKUP, $strArchivePath, {strExpression => '^[0-F]{16}$'}); # Get first WAL segment foreach my $strWalMajor (@stryWalMajor) { my @stryWalFile = $oFile->list( PATH_BACKUP, "${strArchivePath}/${strWalMajor}", - "^[0-F]{24}-[0-f]{40}(\\." . COMPRESS_EXT . "){0,1}\$"); + {strExpression => "^[0-F]{24}-[0-f]{40}(\\." . COMPRESS_EXT . "){0,1}\$"}); if (@stryWalFile > 0) { @@ -410,7 +410,7 @@ sub stanzaList { my @stryWalFile = $oFile->list( PATH_BACKUP, "${strArchivePath}/${strWalMajor}", - "^[0-F]{24}-[0-f]{40}(\\." . COMPRESS_EXT . "){0,1}\$", 'reverse'); + {strExpression => "^[0-F]{24}-[0-f]{40}(\\." . COMPRESS_EXT . "){0,1}\$", strSortOrder => 'reverse'}); if (@stryWalFile > 0) { diff --git a/lib/pgBackRest/Stanza.pm b/lib/pgBackRest/Stanza.pm index eff50688d..0c3084549 100644 --- a/lib/pgBackRest/Stanza.pm +++ b/lib/pgBackRest/Stanza.pm @@ -119,8 +119,8 @@ sub stanzaCreate my $strParentPathBackup = $self->parentPathGet($oFile, PATH_BACKUP_CLUSTER); # Get a listing of files in the directory, ignoring if any are missing - my @stryFileListArchive = fileList($strParentPathArchive, undef, 'forward', true); - my @stryFileListBackup = fileList($strParentPathBackup, undef, 'forward', true); + my @stryFileListArchive = fileList($strParentPathArchive, {bIgnoreMissing => true}); + my @stryFileListBackup = fileList($strParentPathBackup, {bIgnoreMissing => true}); # If force not used and at least one directory is not empty, then check to see if the info files exist if (!optionGet(OPTION_FORCE) && (@stryFileListArchive || @stryFileListBackup)) diff --git a/test/lib/pgBackRestTest/Archive/ArchiveStopTest.pm b/test/lib/pgBackRestTest/Archive/ArchiveStopTest.pm index b4fca4245..0d0aa35be 100644 --- a/test/lib/pgBackRestTest/Archive/ArchiveStopTest.pm +++ b/test/lib/pgBackRestTest/Archive/ArchiveStopTest.pm @@ -96,7 +96,8 @@ sub run #--------------------------------------------------------------------------------------------------------------------------- $self->testResult( - sub {$oFile->list(PATH_BACKUP_ARCHIVE, PG_VERSION_94 . '-1/0000000100000001', '^(?!000000010000000100000002).+')}, + sub {$oFile->list( + PATH_BACKUP_ARCHIVE, PG_VERSION_94 . '-1/0000000100000001', {strExpression => '^(?!000000010000000100000002).+'})}, "000000010000000100000001-72b9da071c13957fb4ca31f05dbd5c644297c2f7${strCompressExt}", 'segment 2-4 not pushed (2 is pushed sometimes when remote but ignore)', 5); @@ -104,7 +105,8 @@ sub run $oHostDbMaster->archivePush($strXlogPath, $strArchiveTestFile, 5); $self->testResult( - sub {$oFile->list(PATH_BACKUP_ARCHIVE, PG_VERSION_94 . '-1/0000000100000001', '^(?!000000010000000100000002).+')}, + sub {$oFile->list( + PATH_BACKUP_ARCHIVE, PG_VERSION_94 . '-1/0000000100000001', {strExpression => '^(?!000000010000000100000002).+'})}, "(000000010000000100000001-72b9da071c13957fb4ca31f05dbd5c644297c2f7${strCompressExt}, " . "000000010000000100000005-72b9da071c13957fb4ca31f05dbd5c644297c2f7${strCompressExt})", 'segment 5 is pushed', 5); diff --git a/test/lib/pgBackRestTest/Common/Host/HostBackupTest.pm b/test/lib/pgBackRestTest/Common/Host/HostBackupTest.pm index daf93e6d3..76cc51d88 100644 --- a/test/lib/pgBackRestTest/Common/Host/HostBackupTest.pm +++ b/test/lib/pgBackRestTest/Common/Host/HostBackupTest.pm @@ -587,7 +587,8 @@ sub backupLast my $self = shift; my @stryBackup = $self->{oFile}->list( - PATH_BACKUP_CLUSTER, undef, '[0-9]{8}-[0-9]{6}F(_[0-9]{8}-[0-9]{6}(D|I)){0,1}', 'reverse'); + PATH_BACKUP_CLUSTER, undef, + {strExpression => '[0-9]{8}-[0-9]{6}F(_[0-9]{8}-[0-9]{6}(D|I)){0,1}', strSortOrder => 'reverse'}); if (!defined($stryBackup[0])) { diff --git a/test/lib/pgBackRestTest/File/FileListTest.pm b/test/lib/pgBackRestTest/File/FileListTest.pm index 7188bdd5e..d2bc8741e 100644 --- a/test/lib/pgBackRestTest/File/FileListTest.pm +++ b/test/lib/pgBackRestTest/File/FileListTest.pm @@ -74,7 +74,9 @@ sub run eval { - @stryFileList = $oFile->list(PATH_BACKUP_ABSOLUTE, $strPath, $strExpression, $strSort, $bIgnoreMissing); + @stryFileList = $oFile->list( + PATH_BACKUP_ABSOLUTE, $strPath, + {strExpression => $strExpression, strSortOrder => $strSort, bIgnoreMissing => $bIgnoreMissing}); return true; } or do