On Tue, Sep 10, 2024 at 05:11:13PM +1000, Peter Smith wrote: > I have rebased the two remaining patches. See v12 attached.
I've looked over the patch set again, and applied 0002. 0001 could be more ambitious and more consistent, like: - The GUC name coming from the table's record is only used for PGC_ENUM, let's use conf->gen.name across the board so as this counts for custom GUCs. - Once we do the first bullet point, parse_and_validate_value() can be simplified as it does not need its "name" argument anymore. - Once the second bullet point is simplified, going one way up reveals more in set_config_with_handle(). I was wondering about pg_parameter_aclcheck() for a bit until I've noticed convert_GUC_name_for_parameter_acl() that applies a conversion with the contents of the catalogs when checking for a parameter ACL, similar to guc_name_compare(). One limitation is in AlterSystemSetConfigFile() where the parameter name comes from the command and not a GUC record as the ACL check happens before the GUC table lookup, but we could live with that. At the end, I get the attached revised 0001. WDYT? -- Michael
From 3b3f4a241a2d127b421a968f5fb19d555e55b91c Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 7 Oct 2024 13:17:01 +0900
Subject: [PATCH v13] Apply GUC name from central table in more places of guc.c
---
src/backend/utils/misc/guc.c | 65 +++++++++++++++----------------
src/test/regress/expected/guc.out | 4 ++
src/test/regress/sql/guc.sql | 3 ++
3 files changed, 39 insertions(+), 33 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 13527fc258..507a5d329a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3115,7 +3115,6 @@ config_enum_get_options(struct config_enum *record, const char *prefix,
* and also calls any check hook the parameter may have.
*
* record: GUC variable's info record
- * name: variable name (should match the record of course)
* value: proposed value, as a string
* source: identifies source of value (check hooks may need this)
* elevel: level to log any error reports at
@@ -3127,7 +3126,7 @@ config_enum_get_options(struct config_enum *record, const char *prefix,
*/
static bool
parse_and_validate_value(struct config_generic *record,
- const char *name, const char *value,
+ const char *value,
GucSource source, int elevel,
union config_var_val *newval, void **newextra)
{
@@ -3142,7 +3141,7 @@ parse_and_validate_value(struct config_generic *record,
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("parameter \"%s\" requires a Boolean value",
- name)));
+ conf->gen.name)));
return false;
}
@@ -3162,7 +3161,7 @@ parse_and_validate_value(struct config_generic *record,
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
- name, value),
+ conf->gen.name, value),
hintmsg ? errhint("%s", _(hintmsg)) : 0));
return false;
}
@@ -3181,7 +3180,7 @@ parse_and_validate_value(struct config_generic *record,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%d%s%s is outside the valid range for parameter \"%s\" (%d%s%s .. %d%s%s)",
newval->intval, unitspace, unit,
- name,
+ conf->gen.name,
conf->min, unitspace, unit,
conf->max, unitspace, unit)));
return false;
@@ -3203,7 +3202,7 @@ parse_and_validate_value(struct config_generic *record,
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
- name, value),
+ conf->gen.name, value),
hintmsg ? errhint("%s", _(hintmsg)) : 0));
return false;
}
@@ -3222,7 +3221,7 @@ parse_and_validate_value(struct config_generic *record,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%g%s%s is outside the valid range for parameter \"%s\" (%g%s%s .. %g%s%s)",
newval->realval, unitspace, unit,
- name,
+ conf->gen.name,
conf->min, unitspace, unit,
conf->max, unitspace, unit)));
return false;
@@ -3278,7 +3277,7 @@ parse_and_validate_value(struct config_generic *record,
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
- name, value),
+ conf->gen.name, value),
hintmsg ? errhint("%s", _(hintmsg)) : 0));
if (hintmsg)
@@ -3460,7 +3459,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
errmsg("parameter \"%s\" cannot be set during a parallel operation",
- name)));
+ record->name)));
return 0;
}
@@ -3476,7 +3475,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed",
- name)));
+ record->name)));
return 0;
}
break;
@@ -3499,7 +3498,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
- name)));
+ record->name)));
return 0;
}
break;
@@ -3509,7 +3508,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed now",
- name)));
+ record->name)));
return 0;
}
@@ -3529,14 +3528,14 @@ set_config_with_handle(const char *name, config_handle *handle,
*/
AclResult aclresult;
- aclresult = pg_parameter_aclcheck(name, srole, ACL_SET);
+ aclresult = pg_parameter_aclcheck(record->name, srole, ACL_SET);
if (aclresult != ACLCHECK_OK)
{
/* No granted privilege */
ereport(elevel,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to set parameter \"%s\"",
- name)));
+ record->name)));
return 0;
}
}
@@ -3578,7 +3577,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be set after connection start",
- name)));
+ record->name)));
return 0;
}
break;
@@ -3591,14 +3590,14 @@ set_config_with_handle(const char *name, config_handle *handle,
*/
AclResult aclresult;
- aclresult = pg_parameter_aclcheck(name, srole, ACL_SET);
+ aclresult = pg_parameter_aclcheck(record->name, srole, ACL_SET);
if (aclresult != ACLCHECK_OK)
{
/* No granted privilege */
ereport(elevel,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to set parameter \"%s\"",
- name)));
+ record->name)));
return 0;
}
}
@@ -3637,7 +3636,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("cannot set parameter \"%s\" within security-definer function",
- name)));
+ record->name)));
return 0;
}
if (InSecurityRestrictedOperation())
@@ -3645,7 +3644,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("cannot set parameter \"%s\" within security-restricted operation",
- name)));
+ record->name)));
return 0;
}
}
@@ -3657,7 +3656,7 @@ set_config_with_handle(const char *name, config_handle *handle,
{
ereport(elevel,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("parameter \"%s\" cannot be reset", name)));
+ errmsg("parameter \"%s\" cannot be reset", record->name)));
return 0;
}
if (action == GUC_ACTION_SAVE)
@@ -3665,7 +3664,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("parameter \"%s\" cannot be set locally in functions",
- name)));
+ record->name)));
return 0;
}
}
@@ -3691,7 +3690,7 @@ set_config_with_handle(const char *name, config_handle *handle,
if (changeVal && !makeDefault)
{
elog(DEBUG3, "\"%s\": setting ignored because previous source is higher priority",
- name);
+ record->name);
return -1;
}
changeVal = false;
@@ -3710,7 +3709,7 @@ set_config_with_handle(const char *name, config_handle *handle,
if (value)
{
- if (!parse_and_validate_value(record, name, value,
+ if (!parse_and_validate_value(record, value,
source, elevel,
&newval_union, &newextra))
return 0;
@@ -3743,7 +3742,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
- name)));
+ conf->gen.name)));
return 0;
}
record->status &= ~GUC_PENDING_RESTART;
@@ -3808,7 +3807,7 @@ set_config_with_handle(const char *name, config_handle *handle,
if (value)
{
- if (!parse_and_validate_value(record, name, value,
+ if (!parse_and_validate_value(record, value,
source, elevel,
&newval_union, &newextra))
return 0;
@@ -3841,7 +3840,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
- name)));
+ conf->gen.name)));
return 0;
}
record->status &= ~GUC_PENDING_RESTART;
@@ -3906,7 +3905,7 @@ set_config_with_handle(const char *name, config_handle *handle,
if (value)
{
- if (!parse_and_validate_value(record, name, value,
+ if (!parse_and_validate_value(record, value,
source, elevel,
&newval_union, &newextra))
return 0;
@@ -3939,7 +3938,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
- name)));
+ conf->gen.name)));
return 0;
}
record->status &= ~GUC_PENDING_RESTART;
@@ -4004,7 +4003,7 @@ set_config_with_handle(const char *name, config_handle *handle,
if (value)
{
- if (!parse_and_validate_value(record, name, value,
+ if (!parse_and_validate_value(record, value,
source, elevel,
&newval_union, &newextra))
return 0;
@@ -4063,7 +4062,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
- name)));
+ conf->gen.name)));
return 0;
}
record->status &= ~GUC_PENDING_RESTART;
@@ -4133,7 +4132,7 @@ set_config_with_handle(const char *name, config_handle *handle,
if (value)
{
- if (!parse_and_validate_value(record, name, value,
+ if (!parse_and_validate_value(record, value,
source, elevel,
&newval_union, &newextra))
return 0;
@@ -4166,7 +4165,7 @@ set_config_with_handle(const char *name, config_handle *handle,
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
- name)));
+ conf->gen.name)));
return 0;
}
record->status &= ~GUC_PENDING_RESTART;
@@ -4660,7 +4659,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
union config_var_val newval;
void *newextra = NULL;
- if (!parse_and_validate_value(record, name, value,
+ if (!parse_and_validate_value(record, value,
PGC_S_FILE, ERROR,
&newval, &newextra))
ereport(ERROR,
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 14edb42704..7f9e29c765 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -6,6 +6,10 @@ SHOW datestyle;
Postgres, MDY
(1 row)
+-- Check output style of CamelCase enum options
+SET intervalstyle to 'asd';
+ERROR: invalid value for parameter "IntervalStyle": "asd"
+HINT: Available values: postgres, postgres_verbose, sql_standard, iso_8601.
-- SET to some nondefault value
SET vacuum_cost_delay TO 40;
SET datestyle = 'ISO, YMD';
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 2be7ab2940..f65f84a263 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -2,6 +2,9 @@
-- we can't rely on any specific default value of vacuum_cost_delay
SHOW datestyle;
+-- Check output style of CamelCase enum options
+SET intervalstyle to 'asd';
+
-- SET to some nondefault value
SET vacuum_cost_delay TO 40;
SET datestyle = 'ISO, YMD';
--
2.45.2
signature.asc
Description: PGP signature
