Hello!
I did a quick review of this when I evaluated it for use in the HBA
discussion. These are mostly minor comments, but please check and
consider if these would be good improvements.
+ for (elt = optenum->members; elt->string_val; elt++)
+ {
+ if (strcmp(value, elt->string_val) == 0)
+ {
Shouldn't this, and many others checks use pg_strcasecmp instead? This
seems to be fine when using the proper interfaces, but other codepaths
(e.g. direct catalog manipulation) could still add incorrect entries.
Loading from the catalog ignores errors, but it would be even better
if it could properly match entries.
in optionParseRawValues:
+ int i;
+
+ is_set = palloc0(sizeof(bool) * spec_set->num);
+ foreach(cell, raw_values)
+ {
Shouldn't this function pfree is_set at the end, especially if we
think about this as a generic options library used in many places in
the future?
+ if (namspace)
+ {
+ spec_set->namspace = palloc(strlen(namspace) + 1);
+ strcpy(spec_set->namspace, namspace);
+ }
I guess it's namspace to avoid the C++ namespace keyword? Its used
consistently, but it looks too much of a typo to me, maybe something
like "nspace" would be better which makes clearer that it's
intentional?
+ if (!parsed)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for enum option \"%s\": %s",
+ option->gen->name, value),
+ optenum->detailmsg ?
+ errdetail_internal("%s", _(optenum->detailmsg)) : 0));
It seems invalid enum options always result in an error, even with
validation turned off. Is this intentional?
+-- Can reset option that is not allowed, but for some reason is already set
+UPDATE pg_class
+ SET reloptions = '{fillfactor=13,autovacuum_enabled=false,illegal_option=4}'
+ WHERE oid = 'reloptions_test'::regclass;
+ALTER TABLE reloptions_test RESET (illegal_option);
+SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
+ reloptions
+------------------------------------------
+ {fillfactor=13,autovacuum_enabled=false}
+(1 row)
+
I think it would be useful to include more tests with invalid values, such as:
* resetting invalid values one by one works even if there are multiple
invalid values
* editing normal values is possible even if the table has invalid values
Also it seems like this test I copied here is repeated twice - the
second is at the toast tests, but it doesn't do anything toast
specific.
