On Sun, Mar 15, 2026 at 10:27 PM Michael Paquier <[email protected]> wrote:
> I'd say no on this one, which would mean going back-and-forth with the > input. If these fail due to a compilation failure, it's not that bad > either, it would be easy to pin-point that the failures are related to > the values of the GUC, not the overall shape of the data in the input > file. Saying that, it would depend on how much complexity this adds > and what kind of validation we'd get out of it. If the additions to > the script are simple like the ones you are proposing here, that would > be probably OK if these improvements catch a bunch of ground-breaking > inconsistencies. If the whole script would need to be refactored, > probably not. We can have two kinds of errors: 1. A typo in an enum name - this will result in a compilation failure most likely, and the compiler will provide nice error messages with the fix suggestion (e.g. GUC_NOT_IN_SAMPLE -> GUC_NOT_NI_SAMPLE, or PGC_SUSET -> PGC_SUSTE) 2. a mismatched enum because of a copy paste mistake (group says PGC_SUSET instead of WAL_ARCHIVING, or boot_val says WAL_ARCHIVING instead of ARCHIVE_MODE_OFF). The examples I provided there aren't likely copy-paste errors, that one is when you copy-paste an existing enum guc and forget to change its boot value. (2) currently compiles without warnings in both cases, but it is also usually easier to spot during review. The patch itself would be well contained: one or two utility functions (GUC_NOT_IN_SAMPLE and other flags are defines, not enums), and then calling them at a few places. So it's not a significant refactoring. But I'm also not a fan of parsing C source code in perl, that's what I meant by fragile. It seems to work, but I wouldn't guarantee that it can properly parse all edge cases. Thinking about this more, these checks would better be implemented as a custom clang-tidy check for example, which I already started to look into for another issue. On Mon, Mar 16, 2026 at 3:13 AM Chao Li <[email protected]> wrote: > I think dot is allowed in a GUC name. Looking at the C code: Yes, but by convention dot is only used by extensions to mark the prefix/namespace of the extension (<extension_name>.<guc_name>). None of the core server GUCs use it. The point of this script isn't to ensure that the generated C code will compile, but to prevent hidden errors and ensure we follow existing proper coding conventions. > BTW, I have similar patch [1] that verify duplicate GUC enum values. I forgotten about that, thanks for the reminder! I didn't like the runtime check approach of it, and I think there are more exceptions than the whitelisted items in the patch, but adding the ability to mark a guc value list unique could be another good candidate for a custom clang-tidy check. (assuming people like my clang-tidy idea, which I didn't even start a thread about yet)
