It is probably not a good idea to restore the latest backup when it was not made from the current PostgreSQL version. If there is no backup after a stanza-upgrade then replicas might be built with a prior version leading to failures.
Add an error in this case if the latest backup would be used, i.e. --set or --type=time/lsn is not specified.
The prior range checking was done based on the valid values for gz. While this worked it was a subset of what is available for lz4 and zst.
Allow the range to be specified for each compress-type. Adding this functionality to the parse module would be a better solution but that is a bigger project than this fix deserves, at least for now.
Calculate a checksum of the data stored in the repository when a file is transformed (e.g. compressed). This allows resume and verify to operate without needing to decompress/decrypt the data.
This can also be used to verify more complex formats such as block incremental and allow backups from the repository without needing to decompress the data to verify the checksum.
Add some basic encrypted tests to maintain coverage. These will be expanded in a future commit.
Manifest checksums were stored as hex-encoded strings due to legacy compatibility with Perl. Storing the checksums as binary in memory uses half the space and avoids many conversions.
There is no change to the on-disk manifest format which stores the checksum as a hex-encoded string.
Our new policy is to support ten versions of PostgreSQL, the five supported releases and the last five EOL releases. As of PostgreSQL 15, that means 9.0/9.1/9.2 are no longer supported by pgBackRest.
Remove all logic associated with 9.0/9.1/9.2 and update the tests.
Document the new support policy.
Update InfoPg to read/write control versions for the history in backup.info, since we can no longer rely on the mappings being available. In theory this could have been an issue after removing 8.3/8.4 if anybody was using a version that old.
This avoids constructs such as decodeToBin(encodeBase64, ...) which are confusing since decode and encode are used in the same function call. decodeToBin(encodingBase64, ...) makes it clearer what is happening.
The storageNewItrP() permissions test was running twice with the errorOnMissing flag set to false. Fix by setting to true for one test.
Also update the comments to be clearer about what the tests are doing and fix minor formatting.
This allows test backups to be run in other test modules.
It is likely that more logic will be moved here but for now this suffices to get test backups working in the restore module.
When loading prior manifests without the new reference list, the code failed to add the current backup to the reference list. Since the current backup is never explicitly referenced, building references from the file list was not sufficient to generate a complete list.
The main problem here was a bad test, fixed in 28f6604. This masked the issue and prevented it from being found. Now it is clear in the test that the current label is missing from the reference list.
Fix by adding the current label to the reference list if a reference list is not stored in the manifest.
Changing the label of a manifest that already had a label was not a good test and it ended up masking a bug where the current backup label was not being added to the reference list on manifest load, since manifestBackupLabelSet() added the label to the reference list. In fact, manifestBackupLabelSet() should never be called after a manifest load or even after the label has been set.
Add an assertion to prevent manifestBackupLabelSet() being called when the label is already set.
The bug exposed here will be fixed in a subsequent commit.
Hopefully this will make it a little clearer to the user what is wrong when they specify an indexed option without an index.
Also fix an ambiguous use of cfgParseOptionP(). The prior code worked in that it set prefixMatch = true but it was not very readable.
Bug Fixes:
* Fix memory leak in file bundle backup/restore. (Reviewed by John Morris, Oscar. Reported by Oscar.)
* Fix protocol error on short read of remote file. (Reviewed by Stephen Frost.)
Improvements:
* Do not store references for zero-length files when bundling. (Reviewed by Stefan Fercot.)
* Use more generic descriptions for pg_start_backup()/pg_stop_backup(). (Reviewed by Greg Sabino Mullane, David Christensen. Suggested by Greg Sabino Mullane.)
Test Suite Improvements:
* Update test.pl --psql-bin option to match command-line help. (Contributed by Koshi Shibagaki. Reviewed by David Steele.)
The option to specify the path to psql was shown in the command-line help as --psql-bin but the option was actually named --pgsql-bin.
Rename to match the help so they are consistent.
The magic in the header is only required so that command-line openssl will recognize the file as being encrypted. In cases where the encrypted data cannot be read with the command-line tool it makes sense to omit the header magic to save some space.
Unfortunately this cannot be enabled for file bundling because it would break backward compatibility. However, it should be possible to enable it for the combination of bundling and block incremental.
The prior code required coverage in the storage/remote module for all filters that could be used remotely.
Now the filter handlers are set at runtime so any filter list can be used with a remote. This is more flexible and makes coverage testing easier. It also resolves a test dependency.
Move the command/remote unit test near the end so it will have access to all filters without using depends.
This flag skips truncation when opening a file for write on drivers that support it, currently Posix and CIFS. This is convenient for cases where the file needs to be manipulated directly using the file descriptor. Using the file descriptor is not ideal and additional functionality should be added to the storage interface, but for now at least this avoids code duplication, especially on close which updates owners, the timestamp, syncs, etc.
The remote driver forbids no truncate because a file descriptor is never available for a remote storage write object.
Update two instances in the current code which benefit from this new functionality, but the primary reason for the change is to support more complex restore deltas in the upcoming block incremental feature.
If a remote file read was stopped before the read was complete or if an error occurred in the middle of the read then the protocol would end up in a bad state and produce this error:
ProtocolError: client state is 'data-get' but expected 'idle'
Prevent this by reading the rest of the file on close() or free() to leave the protocol in an idle state for the next command.
This was a possible issue for bundling because the amount to read is known in advance and therefore eof may not be reached. However, I was only able to reproduce this issue with unreleased code.
On error this issue would cause the original error to be lost. The process may still fail with this fix (if the error comes from another source) but hopefully we'll get better information about the original error.
The names were changed in PostgreSQL 15, so update the code and docs to make the naming more generic where needed to avoid using a version-specific name in the logs and documentation.
The prior result was hex-encoded, which is not optimal. This was legacy from the interface with Perl and then the JSON protocol. The new binary protocol natively supports binary so it makes sense to use it and convert to hex where needed.
A number of these hex conversions can now be removed but that will need to be handled in another commit.
This makes it more efficient to read/write (especially read) varint-128 to/from IO.
Update the Pack type to take advantage of the more efficient read and remove some duplicate code.
The reference list was previously built at load time from whichever references existed in the file list. This was sufficient since the list was for informational purposes only.
The block incremental feature will require a reference list that contains all prior backups, even those that are not explicitly referenced from the manifest. Therefore it makes sense to build and persist a manifest list rather than building it at load time.
This list can still be used for informational purposes, though it needs to be sorted since the list it sill built for older manifest versions and may not be in sorted order.
Add strLstFindIdx() to find references in the list.
The prior method was to check a combination of fields to determine if a file needed to be copied, delta'd, or resumed. This was complicated and ultimately imposed a limitation on the number of operations that could be performed.
Introduce copy, delta, and resume flags in the manifest to make it clearer which operations need to be performed and to reduce complex and duplicated logic.
This also allows zero-length bundled files to be completed during manifest build rather than later on during backup processing.
The prior manifestFileUpdate() function was pretty difficult to use since all the parameters had to specified. Instead, pass a ManifestFile struct that has all members set as needed.
When new struct members are added the manifestFileUpdate() call sites will still need to be reviewed, but this should make the process of adding members a bit simpler.
This appears to have been an oversight in 34d6495. Storing the reference is not really correct since the file is not stored in a prior backup. It also uses more space.
There is no real harm in storing the reference, since it is always ignored on restore, but the code is simpler if the zero-length files can be dealt with during the manifest and don't need additional handling later on. This is also an important part of some upcoming optimizations.
It is possible to log the bundle info correctly but the information is useless with the backup reference, which does not appear until later. For now just omit the bundle info so we are not logging something incorrect.
This test exposes a small logging issue. The bundle information for the matched delta on PG_VERSION is not correct. This issue will be fixed in the next commit.
The information stored in the manifest *is* correct so this bug is essentially cosmetic.
The current call site, manifestFileUnpack(), does not know the total buffer size but the buffer has always been maintained in memory so there should be no corruption. However, there are upcoming use cases where the buffer will be read from IO, the buffer size will be known, and additional sanity checking on buffer overruns will be valuable.
Also rename params to align better with cvtUInt64ToVarInt128().
Direct link creation via Posix functions has been moved to the Posix driver.
This change allows adding SFTP softlink creation in the SFTP driver using the standard interface.
Ninja produces quite a bit of output so error messages are often truncated by the default error/log buffers. Use large buffers in the test harness to capture the error even when there is a lot of output.
Ninja has introduced a --quiet option, but it is currently too new to be in any of our test distributions.
Bug Fixes:
* Fix incorrect time expiration being used for non-default repositories. (Reviewed by Stefan Fercot. Reported by Adam Brusselback.)
* Fix issue when listing directories recursively with a filter. (Reviewed by Stephen Frost. Reported by Efremov Egor.)
Features:
* Backup key/value annotations. (Contributed by Stefan Fercot. Reviewed by David Steele. Suggested by Adam Berlin.)
Improvements:
* Support --set in JSON output for info command. (Contributed by Stefan Fercot. Reviewed by David Steele. Suggested by Anton Kurochkin.)
* Update archive.info timestamps after a successful backup. (Reviewed by Stefan Fercot. Suggested by Alex Richman.)
* Move standby timeline check after checkpoint. (Reviewed by Stefan Fercot, Keith Fiske. Suggested by Keith Fiske.)
* Improve warning message on backup resume. (Suggested by Cynthia Shang.)
Documentation Improvements:
* Add absolute path for kill in pgbackrest.service. (Suggested by Don Seiler.)
While recursing and filtering, if the last entry in a directory was another directory containing entries then the parent list would get freed too early, causing a double free error or segfault.
Fix by ensuring that the completed list is at the top of the stack before freeing it. This will defer freeing parent lists until the contents of paths have been processed.
Lifecycle policies can cause the archive.info file and its copy to be removed since they are only updated on a stanza-upgrade. Update the timestamps after a successful backup to prevent this.
This does not mean that lifecycle policies should be used as a replacement for expiration. However, in some cases there may be policies in place that are out of admin control. If the lifecycle expiration is less than pgbackrest expiration then corruption of the earliest backup will occur at the very least and there might be other corruption which would make the repo unrecoverable.
An error that gets raised all the way to the top TRY block might need to free a lot of resources and any of these callbacks could throw an error and mask the original error. In fact this is pretty likely since we are already in an error state. For example, the Db object will try to close the remote db connection, but if the protocol is in a bad state it will not be able to do so.
Solve this, for now, by not freeing memory or calling callbacks in the CATCH_FATAL() block. This gives us a better chance if being able to report the error without encountering another error first.
For the most part, we don't need to worry about freeing resources (file handles, TLS contexts, etc.) if the program is going to exit immediately. However, it is important to attempt to terminate all active protocol connections, which is done by protocolFree() in main() since the protocol objects live in the top context.
Another way to handle this would be to implement an error stack and that is probably something we will do in the future. But, in the case of a segfault the original error would still be lost. Yet another option would be to still do cleanup but defer it until after the CATCH_FATAL() block.
If a repo is not specified for the expire command then the lowest repo becomes the default. The repo-retention-full value for time was being retrieved from the default rather than a specific repo which led to an incorrect expiration being applied.
Get the value from the specific repo and add a test.
It would be better if the default repo could not be queried in this case but it is not clear how to do that since the repo option is valid for expire (unlike, e.g., archive-push).
Allow key/value annotations to be added with the backup command and added/modified/removed with the new annotate command.
Annotations can be viewed with the info command in text mode when --set is specified and are always included in JSON output.
There are performance benefits to increasing the upload chunk size as long as the tradeoff with additional memory consumption is acceptable.
Make the chunk size configurable for S3, GCS, and Azure, but don't attempt to do any validation of the chunk size beyond some sane limits. The defaults remain as is for each storage type to avoid any unintentional regressions.
Catching individual fatal errors was only used in testing so the tests have been updated to use other errors instead. CATCH_FATAL() is now the only way to catch fatal errors.
This simplifies the logic a bit for upcoming changes to error handling and cleanup.
Also fix an issue where passing errorMessage() directly to THROW*() would attempt to copy the message buffer instead of preserving it, which is undefined behavior. Since there were no instances of this behavior before this commit, this was not a live bug.
All unit and performance tests are now built by the C harness.
Remove all unit/performance test build code from Perl.
Remove code from C harness that is no longer used. This code was included so the C harness could be run separately, but that is no longer needed with this full integration.
The C test harness is used for unit tests from the Perl harness where possible. Currently, unit tests can be run in the C harness when --no-coverage is specified and --profile is not specified.
C harness tests work on meson 0.45.
The C harness runs with valgrind by default. Valgrind can be disabled with --no-valgrind.
Also rebuild containers to add meson and update the documentation so that meson builds will work (even though we don't do them yet).
The standby timeline check was being performed using pg_control data loaded before the backup started. If the backup was started immediately after a promotion the standby might not have executed a checkpoint and written the new timeline to pg_control.
Instead perform the timeline check after the checkpoint is executed. This should ensure that the new timeline is in pg_control.
The prior warning made it sound as if some action was required on the part of the user.
The new message should make it clearer that this action will be performed by pgBackRest.
Build pgbackrest binary and auto-generated code automatically.
Remove --module option and allow modules to run by parameter. This is less verbose and multiple modules can be run at a time.
Allow filtering of modules. Multiple tests can be passed as parameters and if the module ends in / it will be used as a prefix filter. For example, common/ will run all the common modules.
If a test errors the remaining tests will still run but the test process will eventually exit with an error.
CI tests are included but unit tests remain on the development branch.
With these changes all unit tests run except those that specify the define (e.g. common/assert-off) or containerReq (e.g. protocol/protocol) keywords.
Building the C test harness has been simplified:
meson -Dwerror=true -Dfatal-errors=true -Dbuildtype=debug test/build/none pgbackrest
ninja -C test/build/none test/src/test-pgbackrest
To run all modules:
test/build/none/test/src/test-pgbackrest test
Just the common/error module:
test/build/none/test/src/test-pgbackrest test common/error
All info modules:
test/build/none/test/src/test-pgbackrest test info/
Add tzdata package so timezone tests in command/restore work correctly.
Mark default git path as safe. This is a security fix that is not applicable in this environment, but must be set.
Also remove package cleanup, which is inconvenient when new packages need to be installed. It makes sense for containers that will be downloaded from Dockerhub but not so much for a locally-maintained container.
This was clearly an attempt to set the mode when creating a directory, but it never worked and instead created a "750" directory in the current working directory.
Detected when running in an environment where the current working directory was read-only.
Add harness depends when present.
Include libyaml in all test builds.
Fix mode on paths before trying to remove and set test path with mode 770 to match the Perl test harness.
With these changes all unit tests run except those that specify the define (e.g. common/assert-off), binReq (e.g. command/archive-get), or containerReq (e.g. protocol/protocol) keywords.
Builds and code generation need to be done in advance. The following commands are required for setup:
meson setup -Dwerror=true -Dfatal-errors=true -Dbuildtype=debug build pgbackrest
ninja -C build test/src/test-pgbackrest
build/src/build-code help pgbackrest
build/src/build-code postgres pgbackrest
Now tests can be run, e.g.:
build/test/src/test-pgbackrest --module=postgres/interface
Creating new binaries was convenient at first but has now become a maintenance issue.
Solve this by combining that into a single binary that takes an additional parameter to indicate which code should be built.
Also clean up path handling to make it easier to build code from the command line.
This makes the test code a bit simpler where we are listing a path but not following links.
Links in the repository can be used for testing but should never be committed to the main branch.
NOTE TO PACKAGERS: An experimental meson build has been added but packagers should continue to use the autoconf/make build for the foreseeable future.
Improvements:
* OpenSSL 3 support. (Reviewed by Stephen Frost.)
* Create snapshot when listing contents of a path. (Reviewed by John Morris, Stephen Frost.)
* Force target-timeline=current when restore type=immediate. (Reviewed by Stephen Frost.)
* Truncate files during delta restore when they are larger than expected. (Reviewed by Stephen Frost.)
* Disable incremental manifest save when resume=n. (Contributed by Reid Thompson. Reviewed by David Steele.)
* Set backup percent complete to zero before copy start. (Contributed by Reid Thompson. Reviewed by David Steele.)
* Use S3 IsTruncated flag to determine list continuation. (Reviewed by John Morris, Soulou. Suggested by Christian Montagne.)
Documentation Bug Fixes:
* Skip internal options in the configuration reference. (Reported by Francisco Miguel Biete.)
Documentation Improvements:
* Add link to PostgreSQL configuration in repository host section. (Reviewed by Stefan Fercot. Suggested by Julien Cigar.)
Test Suite Improvements:
* Add experimental Meson build. (Reviewed by Eli Schwartz, Sam Bassaly.)
* Allow any path to be passed to the --test-path option. (Contributed by Andrey Sokolov. Reviewed by David Steele.)
* Fix compile error when DEBUG_EXEC_TIME is defined without DEBUG. (Contributed by Andrey Sokolov. Reviewed by David Steele.)
Explicitly set target timeline to "current" when type=immediate and PostgreSQL >= 12. We do this because type=immediate means there won't be any actual attempt to change timelines, but if we leave the target timeline as the default of "latest" then PostgreSQL might fail to restore because it can't reach the "latest" timeline in the repository from this backup.
This is really a PostgreSQL bug and will hopefully be addressed there, but we'll handle it here for older versions, at least until they aren't really seen in the wild any longer.
PostgreSQL < 12 defaults to "current" (but does not accept "current" as a parameter) so no need set it explicitly.
Previously a callback was used to list path contents and if no sort was specified then a snapshot was not required. When deleting files from the path some filesystems could omit files that still existed, which meant the path could not be removed.
Filter . out of lists in the Posix driver since this special entry was only used by test code (and filtered everywhere in the core code).
Also remove callbacks from the storage interface and replace with an iterator that should be easier to use and guarantees efficient use of the snapshots.
v0.45 ships with Ubuntu 18.04, which is currently the oldest distro we support. We may never do a Meson release on Ubuntu 18.04 but this allows us to start running unit tests with Meson in the meantime.
Some more granular options are not available so we use buildtype in more places.
The check for a in-tree autoconf/make build had to be removed since the filesystem APIs are not available.
Finally, alias_target was removed. This means that full paths must be used for build targets, which does not seem too bad. For instance, test/src/test-pgbackrest must now be used as a build target instead of simple test-pgbackrest.
Coverage for these checks was dependent on the order the files were read from disk, which made the tests fragile.
Rearrange the checks and add a test that won't depend on order.
Previously we were just checking for the existence of NextContinuationToken, which the S3 documentation indicates will not be present when the list is not truncated. However, recent versions of Scality send a blank NextContinuationToken when IsTruncated is false. Sending the blank continuation token back causes Scality to send another blank continuation token and an infinite loop occurs.
Instead use IsTruncated (which is required to be present) to determine whether NextContinuationToken should be present. Error if NextContinuationToken is then missing or empty, since an empty token caused an infinite loop with the Scality server (which arguably should have errored when passed an empty token).
The TEST_STORAGE_LIST() macro is more robust and hides the callback mechanism from the caller.
Add features to TEST_STORAGE_LIST() that hrnStorageInfoListCallback() had.
Update tests to use the abbreviated type output (e.g. path/) generated by TEST_STORAGE_LIST().
Having the test harness in C will allow us to remove duplicated Perl code and test on systems where Perl support is not present.
Custom harnesses and shims are currently not implemented, which means only the following tests in the common module will run: error, stack-trace, type-convert, assert-on, mem-context, time, encode, type-object, type-string, type-list, type-buffer, type-variant, reg-exp, log.
The experimental test harness is being committed with partial functionality so it can be used in Windows development. The remaining features will follow as needed.
The meson builds are still experimental so for now the configure/make build process is preferred for release builds. This message should help prevent any automated build systems from picking up meson instead.
Some of the replacements that were being done already existed as constants, so use the constants instead.
Also fix a minor formatting error introduced when testAdd() was renamed to hrnAdd().
This module has dependencies on command/command so it does not make sense for it to be in the common module. Also move protocolFree() to main() since this is a very large dependency.
Adjust the tests so command/exit can be tested later. This is a bit messy but will get adjusted as we improve the test harness.
Both have newer gcc and OpenSSL 3.
Fedora 36 runs horribly slow with valgrind enabled so run the valgrind tests on Ubuntu 22.04. Fedora 36 has a newer gcc so it is still worth testing on.
There are two changes:
* Suppress deprecation warnings so we can build with -Werror and -Wfatal-errors. At some point we'll need to migrate to the new APIs but there does not seem to be a good reason to support two sets of code right now.
* Update the handling for unexpected EOF to handle EOF or error. The error code for EOF has changed and become harder to identify, but we probably don't care whether it is an error or EOF.
Maintaining the version interfaces was complicated by the fact that each interface needed to be in separate compilation unit to avoid type conflicts. This also meant that various build/test files needed to be updated to add the new interfaces.
Solve these problems by auto-generating all the interfaces into a single file. This is made possible by parsing defines and types out of the header files and creating macros to rename the types. At the end of the version interface everything is undef'd. Another benefit is that the auto-generated interfaces can be static and included directly into postgres/interface.c.
Since some code generation is now always required for tests, change --no-gen to --min-gen in test.pl.
It would also make sense to auto-generate the version defines in postgres/version.h, but that will be left for a future commit.
Meson is a new build system that offers simpler syntax and superior performance to autoconf/make. In addition, Windows is supported natively.
The Meson build appears complete, but currently is used only for auto-generation of code and the host build of pgbackrest. Some container upgrades will be required before Meson can be used for container builds.
Also patch the Debian package to force autoconf/make rather than Meson.
Stopping the cluster has started consistently running out of memory on PostgreSQL 9.1. This seems to have happened after pulling in new packages at some point so it might be build related.
Stopping the cluster is not critical for 9.1 so skip it.
These files were never intended to be compiled on their own so the .c extension was a bit misleading. In particular Meson does not like .c files that are not intended to be compiled independently.
Leave header files as is since they are already protected against being included more than once and are never expected to be compiled.
The manifest is saved on a regular basis during a backup so a failed backup can be resumed. For backups that the user has configured/invoked as not resumable, skip the incremental save of the manifest.
Previously the behavior was to download the file from the repository when it was not exactly the same size in PGDATA. However, it may just be that the file was extended and the contents are the same up to the file size recorded in the manifest. This could also be very valuable for files that are always append only, like logs.
Change info.size to file->size in one place. Both are technically correct but file->size makes more sense.
Use the new fileName variable in a few existing places.
Also adjust some existing comments to make them clearer.
Remove VM_OS_REPO since it is no longer required.
Rebalance PostgreSQL versions for more efficient test times.
Always print version of PostgreSQL when testing. This helps verify that new minor releases are being used.
Each mem context can track child contexts, allocations, and a callback. Before this change memory was allocated for tracking all three even if they were not used for a particular context. This made mem contexts unsuitable for String and Variant objects since they are plentiful and need to be as small as possible.
This change allows mem contexts to be configured to track any combination of child contexts, allocations, and a callback. In addition, the mem context can be configured to track a single child context and/or allocation, which saves memory and is a common use case.
Another benefit is that Variants can own objects (e.g. KeyValue) that they encapsulate. All of this makes memory accounting simpler because mem contexts have names while allocations do not. No more memory is used than before since Variants and Strings still had to store the memory context they were originally allocated in so they could be easily freed.
Update the String and Variant objects to use this new functionality. The custom strFree() and varFree() functions are no longer required and can now be a wrapper around objFree().
Lastly, this will allow strMove() and varMove() to be implemented and used in cases where strDup() and varDup() are being used to move a String or Variant to a new context. Since this will be a bit noisy it is saved for a future commit.
Because there is a lot of repetition in this file, changes can look very jumbled with existing data in a diff. Also, if can be hard to tell what is being modified if the diff does not show enough lines before and after.
This change adds labels to the end of the line to localize the diff and make it easier to see what has been changed. Also, remove some linefeeds and make separators more consistent.
The change to parse.auto.c will be committed separately so it can be ignored in history/blame.
Bug Fixes:
* Fix error thrown from FINALLY() causing an infinite loop. (Reviewed by Stephen Frost.)
* Error on all lock failures except another process holding the lock. (Reviewed by Reid Thompson, Geir Råness. Reported by Geir Råness.)
Features:
* Backup file bundling for improved small file support. (Reviewed by Reid Thompson, Stefan Fercot, Chris Bandy.)
* Verify command to validate the contents of a repository. (Contributed by Cynthia Shang, Reid Thompson. Reviewed by David Steele, Stefan Fercot.)
* PostgreSQL 15 support. (Reviewed by Stefan Fercot.)
* Show backup percent complete in info output. (Contributed by Reid Thompson. Reviewed by David Steele.)
* Auto-select backup for restore command --type=lsn. (Contributed by Reid Thompson. Reviewed by Stefan Fercot, David Steele.)
* Suppress existing WAL warning when archive-mode-check is disabled. (Contributed by Reid Thompson. Reviewed by David Steele.)
* Add AWS IMDSv2 support. (Contributed by Nuno Pires. Reviewed by David Steele.)
Improvements:
* Allow repo-hardlink option to be changed after full backup. (Reviewed by Reid Thompson.)
* Increase precision of percent complete logging for backup and restore. (Contributed by Reid Thompson. Reviewed by David Steele.)
* Improve path validation for repo-* commands. (Contributed by Reid Thompson. Reviewed by David Steele.)
* Improve stop command to honor stanza option. (Contributed by Reid Thompson. Reviewed by David Steele. Suggested by ragaoua.)
* Improve error message for invalid repo-azure-key. (Contributed by Reid Thompson. Reviewed by David Steele. Suggested by Seth Daniel.)
* Add hint to check the log on archive-get/archive-push async error. (Reviewed by Reid Thompson.)
* Add ClockError for unexpected clock skew and timezone changes. (Reviewed by Greg Sabino Mullane, Stefan Fercot. Suggested by Greg Sabino Mullane.)
* Strip extensions from history manifest before showing in error message. (Reviewed by Stefan Fercot.)
* Add user:group to lock permission error. (Reviewed by Reid Thompson.)
Documentation Bug Fixes:
* Fix incorrect reference to stanza-update in the user guide. (Fixed by Abubakar Mohammed. Reviewed by David Steele.)
* Fix example for repo-gcs-key-type option in configuration reference. (Reviewed by Reid Thompson.)
* Fix tls-server-auth example and add clarifications. (Reviewed by Reid Thompson.)
Documentation Improvements:
* Simplify messaging around supported versions in the documentation. (Reviewed by Stefan Fercot, Reid Thompson, Greg Sabino Mullane.)
* Add option type descriptions. (Contributed by Reid Thompson. Reviewed by David Steele.)
* Add FAQ about backup types and restore speed. (Contributed by David Christensen. Reviewed by Reid Thompson.)
* Document required base branch for pull requests. (Contributed by David Christensen. Reviewed by Reid Thompson.)
If the user requested the exact repo path then strSub() would be passed an invalid start value leading to an assertion:
$ pgbackrest --stanza=test repo-ls /var/lib/pgbackrest
ASSERT: [025]: start <= this->pub.size (on dev builds)
ASSERT: [025]: string size must be <= 1073741824 bytes (on prod builds)
Fix this by checking if the requested path exactly equals the repo path and returning an empty relative path in this case.
Another issue was that invalid subpaths were not detected if they started with the repo path. For example, /var/lib/pgbackrestsub would not generate an error if the repo path was /var/lib/pgbackrest. Fix this by explictly checking for a / between the repo path and the subpath. This also requires special handling when the repo path is /.
This is not a live bug since the issues were found in an unreleased feature introduced in 5ae84d5.
The encrypted archive-push and repo tests were running very slowly on 32-bit with Valgrind enabled. This appears to be an issue with a newer version of Valgrind, but it has been going on long enough that bisecting does not seem to be worthwhile.
Reduce the size of the encrypted test segments where possible to improve overall test performance.
Integration expect log testing was originally used as a rough-and-ready way to make sure that certain code paths were being executed before the unit tests existed. Now that we have 100% unit test coverage (with expect log testing) the value of the integration expect tests seems minimal at best.
But they do cause numerous issues:
- Maintenance of the expect code and replacements that are required to keep logs reproducible.
- Even a trivial change can cause massive churn in the expect logs, e.g. d9088b2. These changes should be minutely audited but since the expect logs have little value now it is seldom worth the effort.
- The OS version used to do expect testing (RHEL7) can only be used to test one version of PostgreSQL. This makes it hard to balance the PostgreSQL version testing between OS versions.
- When a commit affects expect logs it is not clear (especially for new developers) how to regenerate them and our contributing guide is silent on the issue.
The goal is to migrate the integration tests to C and expect testing is not part of that plan. It seems best to get rid of them now.
Once upon a time the allocation array was allocated up front so this test was required for the top context, which did not allocate up front.
Now allocations are done on demand so this case is covered for every context that does not allocate memory.
This helps rebalance some of the tests that are running long, i.e. d9 and u20.
I would be better to move more PostgreSQL versions to d9, but the base VM does not contain more versions. New minor versions will be out later in the week so that seems a better time to be rebuilding containers.
The emulation is so slow that running all the unit tests would be too expensive, but this at least shows that the build works and some of the more complex tests run. In particular, it is good to test on one big-endian architecture to be sure that checksums are correct.
Update checksums in the tests where they had gotten out of date since the last time we were testing on s390x. Also use a different test in command/archivePushTest to show the name of the file when a checksum does not match to aid in debugging.
The command/archive-push test was updated but not included because there is also a permissions issue, which looks to be the same as what we see on MacOS/FreedBSD. Hopefully we'll be able to fix all of those at the same time.
The function worked fine, but Coverity was unable to determine that the finally block was run, which led to false positives about unfreed memory.
Using a boolean in the block makes it clear to Coverity that the finally block will always be run no matter what else happens.
We'll depend on the compiler to optimize away the boolean if it is not used in a finally block. The cost of the boolean is fairly low in comparison to everything else being done in these macros, so it does not seem worth having a separate block even if the compiler is not able to eliminate the boolean.
This reverts most of 9a271e9 that fixed a bug caused by c5b5b58, which was also attempting to help Coverity understand FINALLY() blocks.
Since the packSize field is 7 bits, it could never fail the check for > 127.
The compiler will catch any packs that are larger than 7 bits and then the pack size will need to be adjusted. For now just adjust the comment to reflect what the test does and give a clearer indication of what to do when a pack grows too large.
This saves a bit of space and should not affect processing speed.
On MacOS (clang) this unexpectedly reduces the size of the binary by 16kiB but on Linux (gcc) there are no savings at all.
The separator parameter in cfgParseCommandRoleName() was useless since it was always set to : and COLON_STR did not provide any clarity its the single other usage.
In cases where clock skew or timezone issues are preventing backup label generation the user could see an error like this:
new backup label '20220504-152308F' is not later than latest backup label '20220504-222042F_20220504-222141I.manifest.gz'
This will happen if the most recent label is drawn from the history. It is cleaner (and probably less confusing) to strip off the extensions so the user sees:
new backup label '20220504-152308F' is not later than latest backup label '20220504-222042F_20220504-222141I'
The order of callbacks and frees meant that memory needed during a callback (for logging in all known cases) might end up being freed before a callback needed it.
Requiring callbacks and logging to check the validity of their allocations is pretty risky and it is not clear that all possible cases have been accounted for.
Instead recursively execute all the callbacks first and then come back and recursively free the context. This is safer and it removes the need to check if a context is freeing so a simple active flag (in debug builds) will do. The caller no longer needs this information at all so remove memContextFreeing() and objMemContextFreeing().
In the JSON output the percent complete is storage as an integer of the percent complete * 100. So, before display it should be converted to double and divided by 100, or split using integer mod and div.
Note that percent complete will only be displayed on the host where the backup was executed. Remote hosts will show a backup/expire running with no percent complete.
PostgreSQL 15 drops support for exclusive backup and renames the start/stop backup commands.
This is based on the pgdg-testing repo since beta1 has not been released yet, but it seems unlikely that breaking changes will be made at this point. beta1 should be tagged just before our next release so we'll retest before the release.
This column has been removed in PostgreSQL 15. Rather than add a lot of special handling, it seems better just to update all versions to not depend on this column.
Add centralized functions to identify the type of database (i.e. system or user) by name and use FirstNormalObjectId when a name is not available.
The new query in the db module will still return the prior result for PostgreSQL <= 15, which will be stored in the manifest. This is important to preserve behavior when downgrading pgBackRest. There are no concerns here for PostgreSQL 15 since older versions of pgBackRest won't be able to restore backups for PostgreSQL 15 anyway.
Any error thrown resets execution to the last setjmp(), which means that parts of the try block need to make sure they don't get run again. FINALLY() was not doing this so if it threw an error it would end up back in the FINALLY() block, where the error would likely be thrown again, causing an infinite loop.
Fix this by tracking the state of FINALLY() and only running it once. This requires cleaning the error stack like CATCH*() and clearing the error like TRY_END() depending on the order of execution.
The archive-get/archive-push commands would not error for, .e.g permissions errors, when attempting to get a lock before launching the async process. Since the async process was not launched there would be no error status file and the user would get a generic failure message. Also, there would be no async log.
Refactor lockAcquireFile() to throw an error when failOnNoLock = false unless the file is locked by another process. This seems to be the original intent of this parameter and there may have been a mistake when porting from Perl. In any case it looks wrong enough to be considered a bug.
The mem context name is used to produce clearer debug errors but it has no purpose in production builds.
Also remove memContextName() and access the struct directly since the name is only used within the common/memContext module.
Note that a few errors that were thrown in production builds (and required the name) are now only thrown in debug builds. In practice we have not seen these errors in production builds due to extensive coverage so it does not seem worth modifying the error to work without the context name.
This saves some memory, which is worthwhile, but the goal is to refactor Strings and Variants to have their own mem contexts and this change will prevent them from using more memory than they are now, along with other changes that will be coming later.
If this error is thrown rather than a specific error returned from the async process, it means the async process is unable to write the status files for some reason and the only way to get the error is out of the async log.
This hint includes the exact async log path and name to make finding errors easier.
Only set -DDEBUG_MEM for the modules currently being tested rather than globally.
Also run tests in a temp mem context. Running in the top context can confuse memory accounting when a new context is created in the top context.
Reuse the section/key/value Strings by truncating them instead of creating a new one every time.
Also add an error for empty sections. This function is only used for loading info files (not config files), which should never contain an empty section.
These functions allow conversion from substrings without needing to create a String or a temporary buffer.
httpDateToTime() no longer requires a temp mem context. Also improve handling of month search to avoid an allocation.
httpUriDecode() no longer requires a temp mem context.
jsonReadStr() no longer requires a temp mem context.
pgLsnFromWalSegment() no longer requires a temp mem context.
pgVersionFromStr() no longer requires a temp mem context. Also do a bit of refactoring.
storageGcsCvtTime() no longer leaks six Strings per call.
storageS3CvtTime() no longer leaks six Strings per call.
Object variables were begin allocated in the calling context rather than the object context.
This is not a live bug because Exec objects are currently created and opened in a long-lived context.
It is not clear why these were split out, but it probably had something to do with testing before storageList() could return NULL for an empty directory.
Also remove the tests that depended on a boolean return, which are no longer needed for coverage.
Previously read/writing JSON required parsing/render via a variant, which add many more memory allocations and loops.
Instead allow JSON to be read/written serially to improve performance and simplify the code. This also allows us to get rid of many String and Variant constant which are no longer required.
The goal is to be able to read/write very large (e.g. gigabyte manifest) JSON structures, which would not be practical with the current code.
Note that external JSON (GCS, S3, etc) is still handled using variants. Converting these will require more consideration about key ordering since it cannot be guaranteed as in our own formats.
This allows code to run after the return type has been generated in the case where it is an expression.
No new functionality here yet, but this will be used by a future commit that audits memory usage.
All fields should be alphabetical. Currently the read code is tolerant of this, but that will not always be the case.
Fields are always written alphabetically so this is just a test issue introduced by d8d41321.
This is not a very realistic case since archive start/stop are always written, but it appears in many other unit tests so it should also be tested here.
