Re: [PATCH] Proof of concept for GUC improvements
On 2022-03-22 13:15:34 -0500, David Christensen wrote: > > On Mar 21, 2022, at 7:53 PM, Tom Lane wrote: > > If we'd done it like this from the beginning, it'd have been > > great, but retrofitting it now is a lot less appealing. > > Yeah, agreed on this. As far as I’m concerned we can reject. Updated CF entry to say so...
Re: [PATCH] Proof of concept for GUC improvements
> On Mar 21, 2022, at 7:53 PM, Tom Lane wrote: > > Andres Freund writes: >> My impression is that there's not a lot of enthusiasm for the concept? If >> that's true we maybe ought to mark the CF entry as rejected? > > Yeah, I'm kind of leaning that way too. I don't see how we can > incorporate the symbolic values into any existing display paths > without breaking applications that expect the old output. > That being the case, it seems like we'd have "two ways to do it" > indefinitely, which would add enough confusion that I'm not > sure there's a net gain. In particular, I foresee novice questions > along the lines of "I set foo to disabled, why is it showing > as zero?" Yeah, my main motivation here was about trying to have less special values in the config files, but I guess it would effectively be making everything effectively an enum and still relies on knowing just what the specific magic values are, no not really a net gain in this department. For the record, I thought this would have a fairly low chance of getting in, was mainly curious what level of effort it would take to get something like this going and get some feedback on the actual idea. > If we'd done it like this from the beginning, it'd have been > great, but retrofitting it now is a lot less appealing. Yeah, agreed on this. As far as I’m concerned we can reject. Thanks, David
Re: [PATCH] Proof of concept for GUC improvements
Andres Freund writes: > My impression is that there's not a lot of enthusiasm for the concept? If > that's true we maybe ought to mark the CF entry as rejected? Yeah, I'm kind of leaning that way too. I don't see how we can incorporate the symbolic values into any existing display paths without breaking applications that expect the old output. That being the case, it seems like we'd have "two ways to do it" indefinitely, which would add enough confusion that I'm not sure there's a net gain. In particular, I foresee novice questions along the lines of "I set foo to disabled, why is it showing as zero?". If we'd done it like this from the beginning, it'd have been great, but retrofitting it now is a lot less appealing. regards, tom lane
Re: [PATCH] Proof of concept for GUC improvements
Hi, On 2022-01-12 12:57:02 -0600, David Christensen wrote: > > Hi, > > > > According to the cfbot, the patch doesn't apply anymore and needs a > > rebase: http://cfbot.cputube.org/patch_36_3290.log > > V4 rebased attached. Doesn't apply anymore, again: http://cfbot.cputube.org/patch_37_3290.log My impression is that there's not a lot of enthusiasm for the concept? If that's true we maybe ought to mark the CF entry as rejected? Greetings, Andres Freund
Re: [PATCH] Proof of concept for GUC improvements
> Hi, > > According to the cfbot, the patch doesn't apply anymore and needs a > rebase: http://cfbot.cputube.org/patch_36_3290.log V4 rebased attached. special-guc-values-v4.patch Description: Binary data
Re: [PATCH] Proof of concept for GUC improvements
Hi, On Thu, Nov 4, 2021 at 5:50 AM David Christensen wrote: > > Hi, enclosed is a v3 [...] According to the cfbot, the patch doesn't apply anymore and needs a rebase: http://cfbot.cputube.org/patch_36_3290.log > 43 out of 133 hunks FAILED -- saving rejects to file > src/backend/utils/misc/guc.c.rej I'm switching the patch to Waiting on Author.
Re: [PATCH] Proof of concept for GUC improvements
> On Nov 3, 2021, at 5:35 AM, Daniel Gustafsson wrote: > > >> >>> On 15 Oct 2021, at 23:54, Cary Huang wrote: >> >> I scanned through the GUC list and found that the following parameters can >> potentially be categorized in the "special_disabled0" group, just for your >> reference. > > > This patch no longer applies, can you please submit a rebased version? Also, > do you have any thoughts on Cary's suggestion in the above review? Hi, enclosed is a v3, which includes the rebase as well as the additional int GUCs mentioned in Cary’s review. I can add support for Reals (required for the JIT and several other ones), but wanted to get feedback/buy-in on the overall idea first. Best, David special-guc-values-v3.patch Description: Binary data
Re: [PATCH] Proof of concept for GUC improvements
> On 15 Oct 2021, at 23:54, Cary Huang wrote: > I scanned through the GUC list and found that the following parameters can > potentially be categorized in the "special_disabled0" group, just for your > reference. This patch no longer applies, can you please submit a rebased version? Also, do you have any thoughts on Cary's suggestion in the above review? -- Daniel Gustafsson https://vmware.com/
Re: [PATCH] Proof of concept for GUC improvements
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:not tested Hi I quite like the feature this patch provides, it makes special int values more meaningful to user. I like that you added a configuration to allow the original int value to be outputted or the special value, this adds some flexibility in case returning special value breaks some old applications. I scanned through the GUC list and found that the following parameters can potentially be categorized in the "special_disabled0" group, just for your reference. max_prepared_transactions vacuum_cost_delay effective_io_concurrency max_parallel_workers max_parallel_maintenance_workers max_parallel_workers_per_gather max_logical_replication_workers max_sync_workers_per_subscription jit_above_cost jit_inline_above_cost jit_optimize_above_cost log_rotation_age log_rotation_size log_transaction_sample_rate Cary Huang - HighGo Software Canada www.highgo.ca
Re: [PATCH] Proof of concept for GUC improvements
Updated version attached with comment fixes and updated for new GUC. special-guc-values-v2.patch Description: Binary data
Re: [PATCH] Proof of concept for GUC improvements
It looks like this patch rotted a little and needs to be rebased. Please see http://cfbot.cputube.org/ The new status of this patch is: Waiting on Author
Re: [PATCH] Proof of concept for GUC improvements
On Fri, Aug 27, 2021 at 1:19 AM Michael Paquier wrote: > > On Thu, Aug 19, 2021 at 03:58:57PM -0700, David G. Johnston wrote: > > I'm at -0.5 as to whether such a patch would actually be an improvement or > > whether the added possibilities would just be confusing and, because it is > > all optional, indefinitely so. > > FWIW, I find this proposition of introducing a set of optional > synonyms to map with some special-case values we have in the > configurations a bit confusing, as that's basically introducing > enum-like options into GUCs that already have a type assigned. It does use enum-like mappings, but that is just because I needed to tie together name + value and just reused the already similar data structure. That could be changed if the code itself is less understandable based on the struct names. > The patch, with its set of options like special_disabled0, > special_disabled_all is not really easy to parse either so that's just > a recipe to make the set of synonyms to grow on an GUC-basis. Yes, when I started out on this, I expected maybe 2-3 different interpretations at most, with more common overlap. I am not tied to making *every* GUC support this; maybe we support the special_disabled or special_disabled0 with differing names. > What I am wondering, though, is if there are cases in the existing > GUCs, with their existing types, where the situation of a default or > disabled value could be improved, though, to make the overall picture > more consistent. I think this would be possible, although the benefit of what I've written is that it doesn't change the interpretation of the value anywhere else, just in GUC parsing (and optionally GUC display). The parsing was where I felt this improved understanding, I'm less tied to outputting in the "canonical" way.
Re: [PATCH] Proof of concept for GUC improvements
On Thu, Aug 19, 2021 at 03:58:57PM -0700, David G. Johnston wrote: > I'm at -0.5 as to whether such a patch would actually be an improvement or > whether the added possibilities would just be confusing and, because it is > all optional, indefinitely so. FWIW, I find this proposition of introducing a set of optional synonyms to map with some special-case values we have in the configurations a bit confusing, as that's basically introducing enum-like options into GUCs that already have a type assigned. The patch, with its set of options like special_disabled0, special_disabled_all is not really easy to parse either so that's just a recipe to make the set of synonyms to grow on an GUC-basis. What I am wondering, though, is if there are cases in the existing GUCs, with their existing types, where the situation of a default or disabled value could be improved, though, to make the overall picture more consistent. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Proof of concept for GUC improvements
On Thu, Aug 19, 2021 at 3:44 PM David Christensen < david.christen...@crunchydata.com> wrote: > Functionality-wise, any thoughts on the overall approach or the specific > patch? > If this information was exposed only by an addition to pg_settings, and thus not changeable via a GUC or affecting SHOW, I think it would be more likely to get accepted. Being more liberal in the accepting of these values as input seems less problematic so that aspect could just stay. But the display changes should be done with new outputs, not repurposing existing ones. I'm at -0.5 as to whether such a patch would actually be an improvement or whether the added possibilities would just be confusing and, because it is all optional, indefinitely so. David J.
Re: [PATCH] Proof of concept for GUC improvements
> Hi, > For parse_special_int(): > > + * true. If it's not found, return false and retval is set to 0. > ... > + /* don't touch the return value in other case */ > + return false; > > It seems the two comments are not consistent with each other (retval is not > set in case no entry is found). > > For special_int_to_value(): > > + * true. If it's not found, return false and retval is set to 0. > > First, there is no assignment to retval at the end of the method. Second, > retval points to string, so it shouldn't be set to 0. > > Cheers Thanks, I actually noticed on a re-read that the comments didn't match, but they'll be fixed in the next version. (Will wait to collect additional feedback.) Functionality-wise, any thoughts on the overall approach or the specific patch? Thanks, David
Re: [PATCH] Proof of concept for GUC improvements
On Thu, Aug 19, 2021 at 3:17 PM David Christensen < david.christen...@crunchydata.com> wrote: > -hackers, > > Enclosed, find a POC patch that implements "special values" for int GUCs. > We have quite a few GUCs > with values that have special meaning atop other settings. I have > attempted to identify these and > make it so you can specify a symbol name for these values instead of just > relying on the magic > number instead. > > For instance, many GUCs have -1 for "disabled", so I've just made it so > you can > specify something like: > > SET log_min_duration_statement = disabled; > > And the raw value will be set to -1 in this case. For the purposes of > testing, I have also added a > new GUC "output_special_values" to affect whether `SHOW` or anything that > relies on _ShowOption() > can show with the special value instead of just the raw magic value, > allowing tools to consume the > original raw value, or provide the output to the user in the nicer format. > > This has only been done for ints, and the passthru I did was very quick, > so I have probably missed > some options that didn't explicitly have their interpretations in the file > and/or I didn't know > about it already. I do not think there are these sorts of values in other > non-int GUCs, but there > might be, so a similar approach could be taken to expand things to other > config types in the future. > > Let me know your thoughts; I personally find this to be useful, and would > be a nicer way for some > configs to be displayed in the postgresql.conf file. > > Best, > > David > > > -- > Hi, For parse_special_int(): + * true. If it's not found, return false and retval is set to 0. ... + /* don't touch the return value in other case */ + return false; It seems the two comments are not consistent with each other (retval is not set in case no entry is found). For special_int_to_value(): + * true. If it's not found, return false and retval is set to 0. First, there is no assignment to retval at the end of the method. Second, retval points to string, so it shouldn't be set to 0. Cheers
Re: [PATCH] Proof of concept for GUC improvements
On 8/20/21 12:09 AM, David Christensen wrote: > -hackers, > > Enclosed, find a POC patch that implements "special values" for int GUCs. We > have quite a few GUCs > with values that have special meaning atop other settings. I have attempted > to identify these and > make it so you can specify a symbol name for these values instead of just > relying on the magic > number instead. > > For instance, many GUCs have -1 for "disabled", so I've just made it so you > can > specify something like: > > SET log_min_duration_statement = disabled; > > And the raw value will be set to -1 in this case. For the purposes of > testing, I have also added a > new GUC "output_special_values" to affect whether `SHOW` or anything that > relies on _ShowOption() > can show with the special value instead of just the raw magic value, allowing > tools to consume the > original raw value, or provide the output to the user in the nicer format. > > This has only been done for ints, and the passthru I did was very quick, so I > have probably missed > some options that didn't explicitly have their interpretations in the file > and/or I didn't know > about it already. I do not think there are these sorts of values in other > non-int GUCs, but there > might be, so a similar approach could be taken to expand things to other > config types in the future. > > Let me know your thoughts; I personally find this to be useful, and would be > a nicer way for some > configs to be displayed in the postgresql.conf file. As discussed on IRC, I am in favor of this improvement. (I have not yet looked at the patch.) -- Vik Fearing
