It makes sense to do this check right after the first compression so any issues are caught early.
Also, none of the current compression formats omit decompressCmd so make the test mandatory.
Coverity was concerned that regExpError() might return and lead to an invalid reference of "this". This was unlikely since the function should never return but Coverity didn't know that. Also, a difference in error-handling logic at the two sites could cause the issue Coverity reported if they were to get out of sync.
Fix by refactoring out the core error function so that it is clear it will never return.
this->input is set to NULL when the read input goes to EOF but it was possible that this->input could be used again in a subsequent loop, according to Coverity.
In fact this would really only be a problem if EOF suddenly went back to false, which is not allowed. However, checking this->input is cheaper than calling ioReadEofDriver() driver on each loop so this change makes sense as an optimization and it makes Coverity happy, too.
If an option may not be valid for a command it should be checked with cfgOptionValid() or cfgOptionTest().
It appears this rule is followed pretty strictly since the only changes required were in unit tests.
Each option type enforced its own constraints but there was a lot of duplication. Centralize the enforcement to remove the duplication.
Also convert the option type assert to a production error. This is unlikely to happen in production but the test is quite cheap so it can't hurt.
Finally, add a NULL check. Most option types can never be NULL.
Make the restore clean process look more like manifest build, i.e. do cleanup of each target root directory outside the main cleanup callback. This means some code duplication but removes the logic handling "dot" paths.
Add tests for both restore and backup (which already worked but was not tested).
Bug Fixes:
* Remove empty subexpression from manifest regular expression. MacOS was not happy about this though other platforms seemed to work fine. (Fixed by David Raftis.)
Improvements:
* Non-blocking TLS implementation. (Reviewed by Slava Moudry, Cynthia Shang, Stephen Frost.)
* Only limit backup copy size for WAL-logged files. The prior behavior could possibly lead to postgresql.conf or postgresql.auto.conf being truncated in the backup. (Reviewed by Cynthia Shang.)
* TCP keep-alive options are configurable. (Suggested by Marc Cousin.)
* Add io-timeout option.
The documentation recommends clearing the error queue before each SSL_*() call.
Since we always check the results of SSL_*() for errors instead of blindly calling SSL_get_error() it's not clear this makes any difference, but it still seems like a good idea to be sure there are no stray errors in the queue.
Timeout used for connections and read/write operations.
Note that the entire read/write operation does not need to complete within this timeout but some progress must be made, even if it is only a single byte.
The prior blocking implementation seemed to be prone to locking up on some (especially recent) kernel versions. Since we were unable to reproduce the issue in a development environment we can only speculate as to the cause, but there is a good chance that blocking sockets were the issue or contributed to the issue.
So move to a non-blocking implementation to hopefully clear up these issues. Testing in production environments that were prone to locking shows that the approach is promising and at the very least not a regression.
The main differences from the blocking version are the non-blocking connect() implementation and handling of WANT_READ/WANT_WRITE retries for all SSL*() functions.
Timeouts in the tests needed to be increased because socket connect() and TLS SSL_connect() were not included in the timeout before. The tests don't run any slower, though. In fact, all platforms but Ubuntu 12.04 worked fine with the shorter timeouts.
select() is a bit old-fashioned and cumbersome to use. Since the select() code needed to be modified to handle write ready this seems like a good time to upgrade to poll().
poll() has been around for a long time so there doesn't seem to be any need to provide a fallback to select().
Also change the error on timeout from FileReadError to ProtocolError. This works better for read vs. write and failure to poll() is indicative of a protocol error or unexpected EOF.
The prior behavior introduced in dcddf3a5 could possibly lead to postgresql.conf or postgresql.auto.conf being truncated in the backup since they are copied via tmp files and could change size during the backup.
In general it seems safer to limit this feature to WAL-logged files which will be reconstructed during recovery.
Building on these platforms gives us better coverage for our build code. Cirrus CI was chosen because it is the only service that supports FreeBSD (that we could find).
The FreedBSD configuration for Vagrant is currently just enough to perform a build.
The MacOS configuration is not actually for Vagrant (yet) but does show the steps needed to setup the build environment on MacOS.
This allows us to add new configurations mostly without changing the behavior of vagrant from the command line, i.e. vagrant up and vagrant ssh will continue to bring up the default configuration.
However, vagrant destroy -f will remove all configurations. That's really only a change in behavior if more than one configuration is running, which is not currently possible.
It looks like Linux is tolerant of the BSD headers so remove the conditionals.
Eventually it might be a good idea to include these based on configure rules but that seems over overkill for now.
Github classifies many C header files as C++, perhaps because they don't contain anything definitively C-like.
Add an override to ensure .h files are always classified as C since this project contains no C++.
A session looks much the same whether it is initiated from the client or the server, so use the session objects to implement the TLS, HTTP, and S3 test servers.
For TLS, at least, there are some differences between client and server sessions so add a client/server type to SocketSession to determine how the session was initiated.
Aside from reducing code duplication, the main advantage is that the test server will now timeout rather than hanging indefinitely when less input that expected is received.
Previously an error was only thrown when errno was set but in practice this is usually not the case. This may have something to do with getting errno late but attempts to get it earlier have not been successful. It appears that errno usually gets cleared and spot research seems to indicate that other users have similar issues.
An error at this point indicates unexpected EOF so it seems better to just throw an error all the time and be consistent.
To test this properly our test server needs to call SSL_shutdown() except when the client expects this error.
This abstraction allows the session code to be shared between the TLS client and (upcoming) server code.
Session management is no longer implemented in TlsClient so the HttpClient was updated to free and create sessions as needed. No test changes were required for HttpClient so the functionality should be unchanged.
Mechanical changes to the TLS tests were required to use TlsSession where appropriate rather than TlsClient. There should be no change in functionality other than how sessions are managed, i.e. using tlsClientOpen()/tlsSessionFree() rather than just tlsClientOpen().
The errorInternalThrowSys*() functions were marked as returning during coverage testing even when they had no possibility to return, i.e. the error parameter was set to constant true. This meant the compiler would treat the functions as returning even when they would not.
Instead create completely separate functions for coverage to use for THROW_ON_SYS_ERROR*() that can return and leave the regular functions marked __noreturn__.
This abstraction allows the session code to be shared between the socket client and (upcoming) server code. There should no difference in how the code works -- only the organization has changed. Note that no changes to the tests were required.
This same abstraction will be required for TlsClient but that will be done in a separate commit because it requires test changes.
These forks were done in a custom way (not sure why) and lack the capability of the standard macros for the parent to wait for child exit.
This mean that the server would continue to run after the tests were complete and that multiple servers could run at once. This caused subtle timing and connection issues that required larger timeouts to resolve.
Don't change the timeouts here since they need to be adjusted in future commits anyway.
It is pretty much impossible for a static IP to not resolve to an address but in theory the error could catch other conditions so it seems best to keep it.
This limitation forced extra logic in cases where zero wait times were needed.
Remove the limitation and the extra logic in cases where zero wait times are possible.
Help identify whether errors are happening in the forked server or the main test by showing the line number where the server was forked off in the stack trace.
If these are not reset then an error not wrapped in a TEST_ERROR*() macro may show the line number of the previous error in a stack trace, which is confusing.
It is better for the line number to be unreported than wrong.
The default process id was previously always 0 but there are cases where it is useful to be able to set the default.
Currently the only use case is for testing but the upcoming server code will also make use of it.
The storage driver requires two list functions to be implemented, list and infoList. But the former is a subset of the latter so implementing both in every driver is wasteful. The reason both exist is that in Posix it is cheaper to get a list of names than it is to stat files to get size, time, etc. In S3 these operations are equivalent.
Introduce storageInfoLevelType to determine the amount of information required by the caller. That way Posix can work efficiently and all drivers can return only the data required which saves some bandwidth. The storageList() and storageInfoList() functions remain in the storage interface since they are useful -- the only change is simplifying the drivers with no external impact.
Note that since list() accepted an expression infoList() must now do so. Checking the expression is optional for the driver but can be used to limit results or save IO costs.
Similarly, exists() and pathExists() are just specialized forms of info() so adapt them to call info() instead.
When DEBUG_COVERAGE was defined errno was being used instead of the value being passed. This apparently worked by happenstance in the single existing usage but it won't work in general.
It's better to start out with plural forms rather than flip back and forth as functions are added and subtracted. So, use "Constructors" instead of "Constructor".
Use "Getters/Setters" rather than "Getters" or "Setters" to avoid similar churn.
This has been the policy for some time but due to migration pressure only new functions and refactors have been following this rule. Now it seems sensible to make a clean sweep and move all the comments that have not been moved already (i.e. most of them).
Only obvious typos and gross inaccuracies in the comments have been fixed. For this most part this was a copy and paste operation.
Useless comments, e.g. "New object", were not copied. Even so, there are surely many deficient comments left.
Some rearranging was done where needed and functions were placed in the proper sections, e.g. "Constructors", "Functions", etc.
A few function prototypes were found that not longer had an implementation. These were removed, but there may be more.
The coding document has been updated to reflect this policy, which is not new but has never been documented.
These functions accepted const Buffer objects and returned non-const pointers which is definitely not a good idea. Add bufPtrConst() to handle cases where only a const return value is needed and update call sites.
Use UNCONSTIFY() in cases where library code out of our control requires a non-const pointer. This includes the already-documented exception in command/backup/pageChecksum and input buffers in the gzCompress and gzDecompress filters.
Allows casting const-ness away from an expression, but doesn't allow changing the type. Enforcement of the latter currently only works for gcc-like compilers.
Note that it is not safe to cast const-ness away if the result will ever be modified (it would be undefined behavior). Doing so can cause compiler mis-optimizations or runtime crashes (by modifying read-only memory). It is only safe to use when the result will not be modified, but API design or language restrictions prevent you from declaring that (e.g. because a function returns both const and non-const variables).
Note that this only works in function scope, not for global variables (it would be nice, but not trivial, to improve that).
UNCONSTIFY() requires static assert which is a feature in its own right.
These were once used heavily in the Perl code but now only a few of them are used in the documentation.
Remove the unused exports as there is generally no need create them for new commands/options.
PostgreSQL enables this option when available which seems like a good idea since we also do not share connections between processes.
Note that as in PostgreSQL there is no way to disable this option.
PostgreSQL enables this option when available which seems like a good idea since we also buffer transmissions.
Note that as in PostgreSQL there is no way to disable this option.
This is really a socket option so the new name is clearer.
Since common/io/socket/tcp will contains a mix of options it makes sense to rename it to socket and cascade name changes as needed.
Prior to 2.25 the individual TCP keep-alive options were not being configured due to a missing header. In 2.25 they were being configured incorrectly due to a disconnect between the timeout specified in ms and what was expected by the TCP options, i.e. seconds.
Instead make the TCP keep-alive options directly configurable, with correct units and better testing. Keep-alive is enabled by default (though it can be defaulted to the system setting instead) and the rest of the options are not set by default. This is in line with what PostgreSQL does, though PostgreSQL does not allow keep-alive to be defaulted.
Also move configuration of TCP options before connect() as PostgreSQL does.
This functionality was embedded into TlsClient but that was starting to get unwieldy.
Add SocketClient to contain all socket-related client functionality.
Documentation builds and tests have only a few packages in common so rearrange packages to save some time and clarify dependencies.
Remove the libperl-dev package which became obsolete when the LibC module was removed in 79cfd3ae.
Add a few comments for good measure.
The primary purpose of this test (currently) is to measure the performance of storageRemoteInfoList(), which is critical for building a manifest when the PostgreSQL host is remote.
The starting baseline of 1 million files is perhaps a bit aggressive but it seems very likely to blow up if there are performance regressions.
Recent performance improvements allow increasing the baseline of this test.
In general it is best if the baseline is large enough to cause the test to blow up if there are performance regressions.
Features:
* Add lz4 compression support. Note that setting compress-type=lz4 will make new backups and archive incompatible (unrestorable) with prior versions of pgBackRest. (Reviewed by Cynthia Shang.)
* Add --dry-run option to the expire command. Use dry-run to see which backups/archive would be removed by the expire command without actually removing anything. (Contributed by Cynthia Shang, Luca Ferrari.)
Improvements:
* Improve performance of remote manifest build. (Suggested by Jens Wilke.)
* Fix detection of keepalive options on Linux. (Contributed by Marc Cousin.)
* Add configure host detection to set standards flags correctly. (Contributed by Marc Cousin.)
* Remove compress/compress-level options from commands where unused. These commands (e.g. restore, archive-get) never used the compress options but allowed them to be passed on the command line. Now they will error when these options are passed on the command line. If these errors occur then remove the unused options. (Reviewed by Cynthia Shang.)
* Limit backup file copy size to size reported at backup start. If a file grows during the backup it will be reconstructed by WAL replay during recovery so there is no need to copy the additional data. (Reviewed by Cynthia Shang.)
If the tests are running quickly then the time target might end up the same as the end time of the prior full backup. That means restore auto-select will not pick it as a candidate and restore the last backup instead causing the restore compare to fail.
So, sleep one second.
Decisions about when to optimize or enable debug code were spread out in too many places making it hard to keep them consistent.
Centralize the logic as much as possible to make it easier to maintain.
Decisions about when to optimize or enable debug code were spread out in too many places making it hard to keep them consistent.
Centralize the logic as much as possible to make it easier to maintain.
Most strings do not contain escape/d characters, so optimize the path where the string contains few or no escape/d characters.
This means far fewer calls to strCatChr() in favor of strCatZN(), which is much more efficient.
Append N characters from a zero-terminated string.
Note that the string does not actually need to be zero-terminated as long as N is <= the end of the string being concatenated.
This construct is logically equivalent but it seems to make the compiler more likely to inline the function, probably because the compiled code is slightly smaller.
The major bottleneck was finding the memory allocation to be resized since it required a sequential search through a list.
Instead, put the allocation header at the beginning of the allocation and return an offset to the user for their buffer. This allows us to use pointer arithmetic to get back to the allocation header quickly when resizing. A side effect is to make memFree() faster as well. The downside is we won't detect garbage pointers passed to memResize()/memFree(), which is also true for MemContext pointers.
The performance benefits can be pretty large in certain cases, in particular when loading and saving manifests. The following are the before and after performance tests on a 900K file manifest.
Before:
run 003 - manifestNewLoad()/manifestSave()
000.000s l0125 - generate manifest
183.411s l0236 - 101.2MB manifest generated with 900000 files
183.411s l0239 - load manifest
403.816s l0243 - completed in 220405ms
403.816s l0245 - check file total
403.816s l0248 - save manifest
670.217s l0253 - completed in 266401ms
670.217s l0256 - find all files
671.263s l0266 - completed in 1046ms
After:
run 003 - manifestNewLoad()/manifestSave()
000.000s l0125 - generate manifest
007.730s l0236 - 101.2MB manifest generated with 900000 files
007.730s l0239 - load manifest
033.431s l0243 - completed in 25701ms
033.431s l0245 - check file total
033.431s l0248 - save manifest
057.755s l0253 - completed in 24324ms
057.755s l0256 - find all files
058.689s l0266 - completed in 934ms
Other storage*InfoList() functions do this but it was missed here.
memResize()/memFree() operations become more expensive as the mem context grows larger so freeing it periodically saves processing time.
* Fix a few issues with file names being truncated introduced in 787d3fd6.
* Use function line info from the lcov file to calculate which lines to show for uncovered functions. This is more accurate than what we were doing before and function comment headers are now excluded which reduces clutter in the report.
The prior macros had grown over time to be pretty significant pieces of code that required a lot of compile time, though runtime was efficient.
Move most of the macro code into functions to reduce compile time, perhaps at a slight expense to runtime. The overall performance benefit is 10-15% so this seems like a good tradeoff.
Add TEST_RESULT_UINT_INT() to safely compare uint to int with range checking.
Upcoming changes to the TEST_RESULT_* macros are more type safe and identified that the wrong macros were being used to test results in many cases.
Commit these changes separately to verify that they work with the current macro versions.
Note that no core bugs were exposed by these changes.
TRY...CATCH blocks are fairly expensive and when all the TEST_RESULT*() macros succeed they are not needed.
Instead just record info at the start of the result test so a detailed exception can be thrown in test.c in the rare case where an exception occurs.
This is helpful for test macros that know the line number.
The line number can now be non-zero below the top of the stack without WITH_BACKTRACE so instead ignore the line number for output when it is zero.
This error type distinguishes test errors with detailed debug info from normal errors which may need, for example, a stack trace to be added for context.
This was passing since we don't test WITH_BACKTRACE in CI because it is used only for test builds.
Ideally we would test this but it doesn't seem worth the trouble at the moment.
If the work or result directories already contain data then the docs might be generated slightly differently. Doing a clean ensures they will always produce the same output (provided the code does not change).
David Steele