On Mon, Oct 7, 2024 at 3:22 PM Michael Paquier <mich...@paquier.xyz> wrote: > > 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?
Hi Michael. Thanks for pushing my other patch. As for what is left, I had made just the minimal patch to fix the known "IntervalStyle" problem, but that assumed prior knowledge of what were the only problem names in the first place. Your way is better -- to always use the record name where possible. One thing I thought remains not so good is how set_config_with_handle(name...) function does record=find_option(name,...) but then all subsequent names are supposed to be coming from the record->name. It feels confusing to have that parameter 'name' still in scope after the find(), e.g., when you should not be using that anymore. So, I tried to separate this logic into 2 functions -- set_config_with_handle() and set_config_with_handle_guts(). There are no name overlaps now, but I wonder if the fix was overkill. See v14. ====== Kind Regards, Peter Smith. Fujitsu Australia
v14-0001-Apply-GUC-name-from-central-table-in-more-places.patch
Description: Binary data
