| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: gen_guc_tables improvements |
| Date: | 2026-03-16 03:12:54 |
| Message-ID: | 41FFF107-16B6-4B5A-BFDF-88AEF4B776F4@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Mar 15, 2026, at 20:18, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> Hello
>
> While reviewing a patch, I noticed a typo in guc_params.dat. The code
> compiled and seemingly worked, and I was very surprised that the
> generator script didn't catch the mistake.
>
> I looked into it, and I found several missing checks in
> gen_guc_tables. I attached fixes for 4 that I think would definitely
> improve the script (for now as separate patches, so it is easy to
> select only some of them):
>
> * 0001 fixes the issue that started this, it validates the allowed
> field names, preventing typos in their names
> * 0002 goes a step further and validates that fields specific to some
> types can only appear for those types
> * 0003 just improves the error reported by duplicate names, previously
> this was confusing (it referred to incorrect ordering)
> * 0004 adds basic checks about allowed characters in GUC names
>
> I was also thinking about adding validations for the enum/define
> values (config group, flags, guc context), but that requires a
> somewhat fragile extraction code, and I decided to leave that out for
> now.
>
> What do you think about these changes?
> <0001-gen_guc_tables-reject-unrecognized-field-names-in-gu.patch><0002-gen_guc_tables-reject-type-inappropriate-fields-in-g.patch><0003-gen_guc_tables-report-duplicate-entry-names-distinct.patch><0004-gen_guc_tables-validate-GUC-name-format.patch>
I reviewed the patch and played a little bit. Overall looks good to me.
My only comment is on 0004:
```
+ unless ($entry->{name} =~ /^[a-zA-Z][a-zA-Z0-9_]*$/)
+ {
+ die sprintf(
+ qq{%s:%d: error: entry name "%s" is not a valid GUC name (must start with a letter, contain only letters, digits, and underscores)\n},
+ $input_fname, $entry->{line_number}, $entry->{name});
+ }
+
```
I think dot is allowed in a GUC name. Looking at the C code:
```
/*
* Decide whether a proposed custom variable name is allowed.
*
* It must be two or more identifiers separated by dots, where the rules
* for what is an identifier agree with scan.l. (If you change this rule,
* adjust the errdetail in assignable_custom_variable_name().)
*/
static bool
valid_custom_variable_name(const char *name)
```
A GUC name can be dot separated multiple identifiers, the rule this patch used applies to identifiers.
Thought current guc_parameters.dat doesn’t have any GUC whose name contains a dot, but dot should be allowed. For example, I tried on master branch with adding “.test” to an existing GUC name:
```
{ name => 'debug_print_raw_parse.test', type => 'bool', context => 'PGC_USERSET', group => 'LOGGING_WHAT',
short_desc => 'Logs each query\'s raw parse tree.',
variable => 'Debug_print_raw_parse',
boot_val => 'false',
},
```
And it works:
```
evantest=# set debug_print_raw_parse.test = 1;
SET
```
BTW, I have similar patch [1] that verify duplicate GUC enum values.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-03-16 03:14:57 | Re: Use SMgrRelation instead of SMgrRelationData * in pgaio_io_set_target_smgr() |
| Previous Message | Xuneng Zhou | 2026-03-16 03:05:42 | Re: Streamify more code paths |