db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-09-16 09:06:18 +02:00
Code Issues Releases Activity

Fix defaults in command-line help.

Browse Source 
This is another attempt to fix broken defaults in the command-line help. PR #2395 tried to do this by removing current and default values (at least from the list) but this removed much of the value of command-line help being context sensitive. Improvements were made to current values in b3ca2e34 but defaults were left as-is, i.e. broken in some cases.

This new approach handles defaults much the same way as current values. If there are multiple defaults for an option then it is displayed in the option list as <multi> and enumerated in the specific option help. For example, if repo1-type=s3 and repo2-type=azure then the defaults for repo1-storage-upload-chunk-size and repo2-storage-upload-chunk-size will differ.

However, getting the above right means we can no longer display defaults for options that are not valid in the current context. For instance, if repo1-type=posix then none of the repo1-s3-* options are valid and therefore their defaults are not displayed in help. In this case they don't have defaults and if you tried to set one of these options you would get an error.
This commit is contained in:
David Steele
2025-05-12 13:33:42 -04:00
committed by GitHub
parent 7b1b1f5322
commit 8737cad566
7 changed files with 193 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
doc/xml/release/2025/2.56.0.xml
View File

@@ -1,2 +1,17 @@
<release date="XXXX-XX-XX" version="2.56.0dev" title="UNDER DEVELOPMENT">
<release-core-list>
<release-improvement-list>
<release-item>
<github-pull-request id="2625"/>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="david.christensen"/>
<release-item-reviewer id="chris.bandy"/>
</release-item-contributor-list>
<p>Fix defaults in command-line help.</p>
</release-item>
</release-improvement-list>
</release-core-list>
</release>

64
src/command/help/help.c
View File

@@ -261,6 +261,45 @@ helpRenderValue(const ConfigOption optionId)
FUNCTION_TEST_RETURN(STRING, result);
}
/***********************************************************************************************************************************
Helper function for helpRender() to output defaults
***********************************************************************************************************************************/
static String *
helpRenderDefault(const ConfigOption optionId)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(ENUM, optionId);
FUNCTION_TEST_END();
String *result = strDup(cfgOptionIdxDefaultValue(optionId, 0));
if (cfgOptionGroup(optionId))
{
MEM_CONTEXT_TEMP_BEGIN()
{
for (unsigned int optionIdx = 1; optionIdx < cfgOptionGroupIdxTotal(cfgOptionGroupId(optionId)); optionIdx++)
{
const String *const defaultValue = cfgOptionIdxDefaultValue(optionId, optionIdx);
if (!strEq(result, defaultValue))
{
MEM_CONTEXT_PRIOR_BEGIN()
{
strFree(result);
result = strNewZ("<multi>");
}
MEM_CONTEXT_PRIOR_END();
break;
}
}
}
MEM_CONTEXT_TEMP_END();
}
FUNCTION_TEST_RETURN(STRING, result);
}
/***********************************************************************************************************************************
Determine if the first character of a summary should be lower-case
***********************************************************************************************************************************/
@@ -551,7 +590,7 @@ helpRender(const Buffer *const helpData)
String *const summary = helpRenderSummary(optionData[optionId].summary);
// Output current and default values if they exist
const String *const defaultValue = cfgOptionDefault(optionId);
const String *const defaultValue = helpRenderDefault(optionId);
const String *const value = helpRenderValue(optionId);
if (value != NULL || defaultValue != NULL)
@@ -619,7 +658,7 @@ helpRender(const Buffer *const helpData)
CONSOLE_WIDTH)));
// Output current and default values if they exist
const String *const defaultValue = cfgOptionDefault(option.id);
const String *const defaultValue = helpRenderDefault(option.id);
const String *const value = helpRenderValue(option.id);
if (value != NULL || defaultValue != NULL)
@@ -651,7 +690,26 @@ helpRender(const Buffer *const helpData)
}
if (defaultValue != NULL)
strCatFmt(result, "default: %s\n", strZ(defaultValue));
{
strCatZ(result, "default:");
if (!strEqZ(defaultValue, "<multi>"))
strCatFmt(result, " %s\n", strZ(defaultValue));
else
{
const unsigned int groupId = cfgOptionGroupId(option.id);
strCatChr(result, '\n');
for (unsigned int optionIdx = 0; optionIdx < cfgOptionGroupIdxTotal(groupId); optionIdx++)
{
const String *const defaultValue = cfgOptionIdxDefaultValue(option.id, optionIdx);
if (defaultValue != NULL)
strCatFmt(result, " %s: %s\n", cfgOptionGroupName(groupId, optionIdx), strZ(defaultValue));
}
}
}
}
// Output alternate name (call it deprecated so the user will know not to use it)

28
src/config/config.c
View File

@@ -410,21 +410,20 @@ cfgOptionIdxTotal(const ConfigOption optionId)
/**********************************************************************************************************************************/
FN_EXTERN const String *
cfgOptionDefault(const ConfigOption optionId)
cfgOptionIdxDefaultValue(const ConfigOption optionId, const unsigned int optionIdx)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(ENUM, optionId);
FUNCTION_TEST_PARAM(UINT, optionIdx);
FUNCTION_TEST_END();
ASSERT(cfgInited());
ASSERT(optionId < CFG_OPTION_TOTAL);
ASSERT_DECLARE(const bool group = configLocal->option[optionId].group);
ASSERT_DECLARE(const unsigned int indexTotal = configLocal->optionGroup[configLocal->option[optionId].groupId].indexTotal);
ASSERT((!group && optionIdx == 0) || (group && optionIdx < indexTotal));
ConfigOptionData *const option = &configLocal->option[optionId];
if (option->defaultValue == NULL)
option->defaultValue = cfgParseOptionDefault(cfgCommand(), optionId, cfgBin());
FUNCTION_TEST_RETURN_CONST(STRING, option->defaultValue);
FUNCTION_TEST_RETURN_CONST(STRING, configLocal->option[optionId].index[optionIdx].defaultValue);
}
/**********************************************************************************************************************************/
@@ -927,6 +926,21 @@ cfgOptionIdxSet(
optionValueType->string = NULL;
}
// Set the default when specified
if (source == cfgSourceDefault)
{
if (value != NULL)
{
MEM_CONTEXT_BEGIN(configLocal->memContext)
{
optionValue->defaultValue = cfgOptionDisplayVar(value, cfgParseOptionType(optionId));
}
MEM_CONTEXT_END();
}
else
optionValue->defaultValue = NULL;
}
// Clear the display value, which will be generated when needed
optionValue->display = NULL;

4
src/config/config.intern.h
View File

@@ -46,6 +46,7 @@ typedef struct ConfigOptionValue
bool reset; // Is the option reset?
unsigned int source; // Where the option came from, i.e. ConfigSource enum
const String *display; // Current display value, if any. Used for messages, etc.
const String *defaultValue; // Default value
ConfigOptionValueType value; // Option value
} ConfigOptionValue;
@@ -56,7 +57,6 @@ typedef struct ConfigOptionData
bool group; // In a group?
unsigned int groupId; // Id if in a group
ConfigOptionDataType dataType; // Underlying data type
const String *defaultValue; // Default value
ConfigOptionValue *index; // List of indexed values (only 1 unless the option is indexed)
const String **indexName; // Index names (e.g. repo1-path, repo2-path)
} ConfigOptionData;
@@ -110,7 +110,7 @@ FN_EXTERN unsigned int cfgOptionGroupId(ConfigOption optionId);
Option Functions
***********************************************************************************************************************************/
// Option default - should only be called by the help command
FN_EXTERN const String *cfgOptionDefault(ConfigOption optionId);
FN_EXTERN const String *cfgOptionIdxDefaultValue(ConfigOption optionId, unsigned int optionIdx);
// Format a variant for display using the supplied option type. cfgOptionDisplay()/cfgOptionIdxDisplay() should be used whenever
// possible, but sometimes the variant needs to be manipulated before being formatted.

20
src/config/parse.c
View File

@@ -2672,23 +2672,29 @@ cfgParse(const Storage *const storage, const unsigned int argListSize, const cha
{
configOptionValue->source = parseOptionValue->source;
}
// Else try to set a default
else
if ((!configOptionValue->set && !parseOptionValue->negate) || config->help)
{
// If the option has a default
if (cfgParseOptionalRule(&optionalRules, parseRuleOptionalTypeDefault, config->command, optionId))
{
configOptionValue->set = true;
configOptionValue->value = optionalRules.defaultValue;
if (!configOptionValue->set)
{
configOptionValue->set = true;
configOptionValue->value = optionalRules.defaultValue;
configOptionValue->display = optionalRules.defaultRaw;
}
configOptionValue->defaultValue = optionalRules.defaultRaw;
}
// Else error if option is required and help was not requested
else
else if (!config->help)
{
const bool required =
cfgParseOptionalRule(&optionalRules, parseRuleOptionalTypeRequired, config->command, optionId) ?
optionalRules.required : ruleOption->required;
if (required && !config->help)
if (required)
{
THROW_FMT(
OptionRequiredError, "%s command requires option: %s%s",
@@ -2707,6 +2713,8 @@ cfgParse(const Storage *const storage, const unsigned int argListSize, const cha
{
configOptionValue->set = true;
configOptionValue->value.boolean = dependResult.defaultValue;
configOptionValue->defaultValue = optionalRules.defaultRaw;
configOptionValue->display = optionalRules.defaultRaw;
}
pckReadFree(optionalRules.pack);

81
test/src/module/command/helpTest.c
View File

@@ -242,9 +242,9 @@ testRun(void)
" specified directory\n"
" --target recovery target\n"
" --target-action action to take when recovery target is\n"
" reached [default=pause]\n"
" reached\n"
" --target-exclusive stop just before the recovery target is\n"
" reached [default=n]\n"
" reached\n"
" --target-timeline recover along a timeline\n"
" --type recovery type [default=default]\n"
"\n"
@@ -298,19 +298,17 @@ testRun(void)
" --repo-azure-account azure repository account\n"
" --repo-azure-container azure repository container\n"
" --repo-azure-endpoint azure repository endpoint\n"
" [default=blob.core.windows.net]\n"
" --repo-azure-key azure repository key\n"
" --repo-azure-key-type azure repository key type [default=shared]\n"
" --repo-azure-uri-style azure URI Style [default=host]\n"
" --repo-azure-key-type azure repository key type\n"
" --repo-azure-uri-style azure URI Style\n"
" --repo-cipher-pass repository cipher passphrase\n"
" [current=<redacted>]\n"
" --repo-cipher-type cipher used to encrypt the repository\n"
" [current=<multi>, default=none]\n"
" --repo-gcs-bucket GCS repository bucket\n"
" --repo-gcs-endpoint GCS repository endpoint\n"
" [default=storage.googleapis.com]\n"
" --repo-gcs-key GCS repository key\n"
" --repo-gcs-key-type GCS repository key type [default=service]\n"
" --repo-gcs-key-type GCS repository key type\n"
" --repo-gcs-user-project GCS project ID\n"
" --repo-host repository host when operating remotely\n"
" [current=<multi>]\n"
@@ -339,19 +337,19 @@ testRun(void)
" --repo-s3-endpoint S3 repository endpoint\n"
" --repo-s3-key S3 repository access key\n"
" --repo-s3-key-secret S3 repository secret access key\n"
" --repo-s3-key-type S3 repository key type [default=shared]\n"
" --repo-s3-key-type S3 repository key type\n"
" --repo-s3-kms-key-id S3 repository KMS key\n"
" --repo-s3-region S3 repository region\n"
" --repo-s3-requester-pays S3 repository requester pays [default=n]\n"
" --repo-s3-requester-pays S3 repository requester pays\n"
" --repo-s3-role S3 repository role\n"
" --repo-s3-sse-customer-key S3 repository SSE customer key\n"
" --repo-s3-token S3 repository security token\n"
" --repo-s3-uri-style S3 URI Style [default=host]\n"
" --repo-s3-uri-style S3 URI Style\n"
" --repo-sftp-host SFTP repository host\n"
" --repo-sftp-host-fingerprint SFTP repository host fingerprint\n"
" --repo-sftp-host-key-check-type SFTP host key check type [default=strict]\n"
" --repo-sftp-host-key-check-type SFTP host key check type\n"
" --repo-sftp-host-key-hash-type SFTP repository host key hash type\n"
" --repo-sftp-host-port SFTP repository host port [default=22]\n"
" --repo-sftp-host-port SFTP repository host port\n"
" --repo-sftp-host-user SFTP repository host user\n"
" --repo-sftp-known-host SFTP known hosts file\n"
" --repo-sftp-private-key-file SFTP private key file\n"
@@ -360,11 +358,11 @@ testRun(void)
" --repo-storage-ca-file repository storage CA file\n"
" --repo-storage-ca-path repository storage CA path\n"
" --repo-storage-host repository storage host\n"
" --repo-storage-port repository storage port [default=443]\n"
" --repo-storage-port repository storage port\n"
" --repo-storage-tag repository storage tag(s)\n"
" --repo-storage-upload-chunk-size repository storage upload chunk size\n"
" [default=4194304]\n"
" --repo-storage-verify-tls repository storage certificate verify\n"
" [default=y]\n"
" --repo-target-time target time for repository\n"
" --repo-type type of storage used for the repository\n"
" [default=posix]\n"
@@ -639,6 +637,61 @@ testRun(void)
strLstAddZ(argList, "repo-host");
TEST_RESULT_VOID(testCfgLoad(argList), "help for restore command, repo-host option");
TEST_RESULT_STR_Z(helpRender(helpData), optionHelp, "check text");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("multiple default values");
#define HELP_OPTION_CHUNK \
"%s - 'restore' command - 'repo-storage-upload-chunk-size' option help\n" \
"\n" \
"Repository storage upload chunk size.\n" \
"\n" \
"Object stores such as S3 allow files to be uploaded in chunks when the file is\n" \
"too large to be stored in memory. Even if the file can be stored in memory, it\n" \
"is more memory efficient to limit the amount of memory used for uploads.\n" \
"\n" \
"A larger chunk size will generally lead to better performance because it will\n" \
"minimize upload requests and allow more files to be uploaded in a single\n" \
"request rather than in chunks. The disadvantage is that memory usage will be\n" \
"higher and because the chunk buffer must be allocated per process, larger\n" \
"process-max values will lead to more memory being consumed overall.\n" \
"\n" \
"Default chunk sizes by repo type:\n" \
"\n" \
"* azure - 4MiB\n" \
"* gcs - 4MiB\n" \
"* s3 - 5MiB\n" \
"\n" \
"Note that valid chunk sizes vary by storage type and by platform. For example,\n" \
"AWS S3 has a minimum chunk size of 5MiB but S3 clones may accept lower values.\n" \
"Terminology for chunk size varies by storage type, so when searching min/max\n" \
"values use \"part size\" for AWS S3, \"chunk size\" for GCS, and \"block size\" for\n" \
"Azure. No attempt is made to validate configured chunk sizes so selecting an\n" \
"invalid value will lead to errors from the storage service or undefined\n" \
"behavior.\n"
optionHelp = zNewFmt(
HELP_OPTION_CHUNK
"\n"
"default:\n"
" repo1: 5242880\n"
" repo2: 4194304\n",
helpVersion);
argList = strLstNew();
strLstAddZ(argList, "/path/to/pgbackrest");
hrnCfgArgKeyRawZ(argList, cfgOptRepoType, 1, "s3");
hrnCfgArgKeyRawZ(argList, cfgOptRepoType, 2, "azure");
hrnCfgArgKeyRawZ(argList, cfgOptRepoType, 3, "gcs");
strLstAddZ(argList, "help");
strLstAddZ(argList, "restore");
strLstAddZ(argList, "repo-storage-upload-chunk-size");
TEST_RESULT_VOID(testCfgLoad(argList), "help for restore command, repo-storage-upload-chunk-size option");
// There is currently no way for some defaults to be NULL and others not but it could happen in the future
cfgOptionIdxSet(cfgOptRepoStorageUploadChunkSize, 2, cfgSourceDefault, NULL);
TEST_RESULT_STR_Z(helpRender(helpData), optionHelp, "check text");
}
// *****************************************************************************************************************************

21
test/src/module/config/parseTest.c
View File

@@ -1945,7 +1945,7 @@ testRun(void)
TEST_RESULT_INT(cfgOptionInt64(cfgOptBufferSize), 65536, "buffer-size is set");
TEST_RESULT_INT(cfgOptionSource(cfgOptBufferSize), cfgSourceConfig, "backup-standby is source config");
TEST_RESULT_UINT(cfgOptionUInt64(cfgOptDbTimeout), 1800000, "db-timeout is set");
TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptDbTimeout), "1800", "db-timeout display");
TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptDbTimeout), "30m", "db-timeout display");
TEST_RESULT_UINT(cfgOptionUInt64(cfgOptProtocolTimeout), 3600000, "protocol-timeout is set");
TEST_RESULT_UINT(cfgOptionIdxUInt(cfgOptPgPort, 1), 5432, "pg2-port is set");
TEST_RESULT_UINT(cfgOptionIdxUInt64(cfgOptPgPort, 1), 5432, "pg2-port is set");
@@ -1957,13 +1957,18 @@ testRun(void)
TEST_RESULT_BOOL(cfgParseOptionRequired(cfgCmdBackup, cfgOptPgHost), false, "pg-host is not required for backup");
TEST_RESULT_BOOL(cfgParseOptionRequired(cfgCmdInfo, cfgOptStanza), false, "stanza is not required for info");
TEST_RESULT_STR_Z(cfgOptionDefault(cfgOptBackupStandby), "n", "backup-standby default is false");
TEST_RESULT_STR_Z(cfgOptionDefault(cfgOptBackupStandby), "n", "backup-standby default is false (again)");
TEST_RESULT_PTR(cfgOptionDefault(cfgOptPgHost), NULL, "pg-host default is NULL");
TEST_RESULT_STR_Z(cfgOptionDefault(cfgOptLogLevelConsole), "warn", "log-level-console default is warn");
TEST_RESULT_STR_Z(cfgOptionDefault(cfgOptPgPort), "5432", "pg-port default is 5432");
TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptPgPort), "5432", "pg-port display is 5432");
TEST_RESULT_STR_Z(cfgOptionDefault(cfgOptDbTimeout), "30m", "db-timeout default is 30m");
TEST_RESULT_STR_Z(cfgOptionIdxDefaultValue(cfgOptBackupStandby, 0), "n", "backup-standby default is false");
TEST_RESULT_STR_Z(cfgOptionIdxDefaultValue(cfgOptBackupStandby, 0), "n", "backup-standby default is false (again)");
TEST_RESULT_PTR(cfgOptionIdxDefaultValue(cfgOptPgHost, 0), NULL, "pg-host default is NULL");
TEST_RESULT_STR_Z(cfgOptionIdxDefaultValue(cfgOptLogLevelConsole, 0), "warn", "log-level-console default is warn");
TEST_RESULT_STR_Z(cfgOptionIdxDefaultValue(cfgOptPgPort, 0), "5432", "pg-port default is 5432");
TEST_RESULT_STR_Z(cfgOptionIdxDisplay(cfgOptPgPort, 0), "5432", "pg-port display is 5432");
TEST_RESULT_VOID(cfgOptionSet(cfgOptDbTimeout, cfgSourceDefault, VARINT64(30000)), "set db-timeout default");
TEST_RESULT_STR_Z(cfgOptionIdxDefaultValue(cfgOptDbTimeout, 0), "30", "db-timeout default is 30m");
TEST_RESULT_STR_Z(cfgParseOptionDefault(cfgCmdBackup, cfgOptPgHost, STRDEF("pgbackrest")), NULL, "default not found");
TEST_RESULT_STR_Z(
cfgParseOptionDefault(cfgCmdBackup, cfgOptRepoPath, STRDEF("pgbackrest")), "/var/lib/pgbackrest", "default found");
TEST_RESULT_VOID(
cfgOptionIdxSet(cfgOptPgSocketPath, 1, cfgSourceDefault, VARSTRDEF("/default")), "set pg-socket-path default");