Thread: GUC tables - use designated initializers
Hi hackers, Enums index a number of the GUC tables. This all relies on the elements being carefully arranged to be in the same order as those enums. There are comments to say what enum index belongs to each table element. But why not use designated initializers to enforce what the comments are hoping for? ~~ PSA a patch for the same. Doing this also exposed a minor typo in the comments. "ERROR_HANDLING" -> "ERROR_HANDLING_OPTIONS" Furthermore, with this change, now the GUC table elements are able to be rearranged into any different order - eg alphabetical - if that would be useful (my patch does not do this). ~~ In passing, I also made a 0002 patch to remove some inconsistent whitespace noticed in those config tables. Thoughts? ------ Kind Regards, Peter Smith. Fujitsu Australia.
Attachment
On Tue, Sep 27, 2022 at 09:27:48AM +1000, Peter Smith wrote: > But why not use designated initializers to enforce what the comments > are hoping for? This is a C99 thing as far as I understand, adding one safety net. Why not for these cases.. > Doing this also exposed a minor typo in the comments. > "ERROR_HANDLING" -> "ERROR_HANDLING_OPTIONS" Right. -- Michael
Attachment
Peter Smith <smithpb2250@gmail.com> writes:
> Enums index a number of the GUC tables. This all relies on the
> elements being carefully arranged to be in the same order as those
> enums. There are comments to say what enum index belongs to each table
> element.
> But why not use designated initializers to enforce what the comments
> are hoping for?
Interesting proposal, but it's not clear to me that this solution makes
the code more bulletproof rather than less so. Yeah, you removed the
order dependency, but the other concern here is that the array gets
updated at all when adding a new enum entry. This method seems like
it'd help disguise such an oversight. In particular, the adjacent
StaticAssertDecls about the array lengths are testing something different
than they used to, and I fear they lost some of their power. Can we
improve those checks so they'd catch a missing entry again?
> Furthermore, with this change, now the GUC table elements are able to
> be rearranged into any different order - eg alphabetical - if that
> would be useful (my patch does not do this).
If anything, that's an anti-feature IMV. I quite dislike code where
the same set of items are dealt with in randomly different orders
in different places.
regards, tom lane
On Wed, Sep 28, 2022 at 2:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > Enums index a number of the GUC tables. This all relies on the > > elements being carefully arranged to be in the same order as those > > enums. There are comments to say what enum index belongs to each table > > element. > > But why not use designated initializers to enforce what the comments > > are hoping for? > > Interesting proposal, but it's not clear to me that this solution makes > the code more bulletproof rather than less so. Yeah, you removed the > order dependency, but the other concern here is that the array gets > updated at all when adding a new enum entry. This method seems like > it'd help disguise such an oversight. In particular, the adjacent > StaticAssertDecls about the array lengths are testing something different > than they used to, and I fear they lost some of their power. Thanks for the feedback! The current code StaticAssertDecl asserts that the array length is the same as the number of enums by using hardwired knowledge of what enum is the "last" one (e.g. DEVELOPER_OPTIONS in the example below). StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 2), "array length mismatch"); Hmmm. I think maybe I found the example to justify your fear. It's a bit subtle and AFAIK the HEAD code would not suffer this problem --- imagine if the developer adds the new enum before the "last" one (e.g. ADD_NEW_BEFORE_LAST comes before DEVELOPER_OPTIONS) and at the same time they *forgot* to update the table elements, then that designated index [DEVELOPER_OPTIONS] will still ensure the table becomes the correct increased length (so the StaticAssertDecl will be ok) except now there will be an undetected "hole" in the table at the forgotten [ADD_NEW_BEFORE_LAST] index. > Can we > improve those checks so they'd catch a missing entry again? Thinking... ------ Kind Regards, Peter Smith. Fujitsu Australia
On Wed, Sep 28, 2022 at 12:04 PM Peter Smith <smithpb2250@gmail.com> wrote: > > On Wed, Sep 28, 2022 at 2:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Peter Smith <smithpb2250@gmail.com> writes: > > > Enums index a number of the GUC tables. This all relies on the > > > elements being carefully arranged to be in the same order as those > > > enums. There are comments to say what enum index belongs to each table > > > element. > > > But why not use designated initializers to enforce what the comments > > > are hoping for? > > > > Interesting proposal, but it's not clear to me that this solution makes > > the code more bulletproof rather than less so. Yeah, you removed the > > order dependency, but the other concern here is that the array gets > > updated at all when adding a new enum entry. This method seems like > > it'd help disguise such an oversight. In particular, the adjacent > > StaticAssertDecls about the array lengths are testing something different > > than they used to, and I fear they lost some of their power. > > Thanks for the feedback! > > The current code StaticAssertDecl asserts that the array length is the > same as the number of enums by using hardwired knowledge of what enum > is the "last" one (e.g. DEVELOPER_OPTIONS in the example below). > > StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 2), > "array length mismatch"); > > Hmmm. I think maybe I found the example to justify your fear. It's a > bit subtle and AFAIK the HEAD code would not suffer this problem --- > imagine if the developer adds the new enum before the "last" one (e.g. > ADD_NEW_BEFORE_LAST comes before DEVELOPER_OPTIONS) and at the same > time they *forgot* to update the table elements, then that designated > index [DEVELOPER_OPTIONS] will still ensure the table becomes the > correct increased length (so the StaticAssertDecl will be ok) except > now there will be an undetected "hole" in the table at the forgotten > [ADD_NEW_BEFORE_LAST] index. > > > Can we > > improve those checks so they'd catch a missing entry again? > > Thinking... > Although adding designated initializers did fix some behaviour of the current code (e.g. which assumed array element order, but cannot enforce it), it also introduces some new unwanted quirks (e.g. accidentally omitting table elements can now be undetectable). I can't see any good coming from exchanging one kind of problem for a new kind of problem, so I am abandoning the idea of using designated initializers in these GUC tables. ~ The v2 patches are updated as follows: 0001 - Now this patch only fixes a comment that had a wrong enum name. 0002 - Removes unnecessary whitespace (same as v1-0002) ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Tue, Oct 04, 2022 at 04:20:36PM +1100, Peter Smith wrote: > The v2 patches are updated as follows: > > 0001 - Now this patch only fixes a comment that had a wrong enum name. This was wrong, so fixed. > 0002 - Removes unnecessary whitespace (same as v1-0002) This one does not seem worth doing, though.. -- Michael
Attachment
On Tue, Oct 4, 2022 at 5:48 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Oct 04, 2022 at 04:20:36PM +1100, Peter Smith wrote: > > The v2 patches are updated as follows: > > > > 0001 - Now this patch only fixes a comment that had a wrong enum name. > > This was wrong, so fixed. Thanks for pushing! > > > 0002 - Removes unnecessary whitespace (same as v1-0002) > > This one does not seem worth doing, though.. Yeah, fair enough. I didn't really expect much support for that one, but I thought I'd post it anyway when I saw it removed 250 lines from the already long source file. ------ Kind Regards, Peter Smith. Fujitsu Australia