Horton

Thread

  1. [PATCH] Proof of concept for GUC improvements

    David Christensen <david.christensen@crunchydata.com> — 2021-08-19T22:09:25Z

    -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

  2. Re: [PATCH] Proof of concept for GUC improvements

    Vik Fearing <vik@postgresfriends.org> — 2021-08-19T22:30:31Z

    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

  3. Re: [PATCH] Proof of concept for GUC improvements

    David Christensen <david.christensen@crunchydata.com> — 2021-08-19T22:43:55Z

    > 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

  4. Re: [PATCH] Proof of concept for GUC improvements

    Zhihong Yu <zyu@yugabyte.com> — 2021-08-19T22:47:33Z

    On Thu, Aug 19, 2021 at 3:17 PM David Christensen <
david.christensen@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

  5. Re: [PATCH] Proof of concept for GUC improvements

    David G. Johnston <david.g.johnston@gmail.com> — 2021-08-19T22:58:57Z

    On Thu, Aug 19, 2021 at 3:44 PM David Christensen <
david.christensen@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.

  6. Re: [PATCH] Proof of concept for GUC improvements

    Michael Paquier <michael@paquier.xyz> — 2021-08-27T06:19:04Z

    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

  7. Re: [PATCH] Proof of concept for GUC improvements

    David Christensen <david.christensen@crunchydata.com> — 2021-08-27T18:22:24Z

    On Fri, Aug 27, 2021 at 1:19 AM Michael Paquier <michael@paquier.xyz> 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.

  8. Re: [PATCH] Proof of concept for GUC improvements

    Aleksander Alekseev <afiskon@gmail.com> — 2021-09-09T09:48:37Z

    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

  9. Re: [PATCH] Proof of concept for GUC improvements

    David Christensen <david.christensen@crunchydata.com> — 2021-09-15T14:54:44Z

    Updated version attached with comment fixes and updated for new GUC.

  10. Re: [PATCH] Proof of concept for GUC improvements

    Cary Huang <cary.huang@highgo.ca> — 2021-10-15T21:54:21Z

    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

  11. Re: [PATCH] Proof of concept for GUC improvements

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-03T10:35:31Z

    > On 15 Oct 2021, at 23:54, Cary Huang <cary.huang@highgo.ca> 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/

  12. Re: [PATCH] Proof of concept for GUC improvements

    David Christensen <david@pgguru.net> — 2021-11-03T21:50:20Z

    > On Nov 3, 2021, at 5:35 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> 
>> 
>>> On 15 Oct 2021, at 23:54, Cary Huang <cary.huang@highgo.ca> 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

  13. Re: [PATCH] Proof of concept for GUC improvements

    Julien Rouhaud <rjuju123@gmail.com> — 2022-01-12T06:55:16Z

    Hi,

On Thu, Nov 4, 2021 at 5:50 AM David Christensen <david@pgguru.net> 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.

  14. Re: [PATCH] Proof of concept for GUC improvements

    David Christensen <david@pgguru.net> — 2022-01-12T18:57:02Z

    > 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.

  15. Re: [PATCH] Proof of concept for GUC improvements

    Andres Freund <andres@anarazel.de> — 2022-03-22T00:37:00Z

    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

  16. Re: [PATCH] Proof of concept for GUC improvements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T00:53:44Z

    Andres Freund <andres@anarazel.de> 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

  17. Re: [PATCH] Proof of concept for GUC improvements

    David Christensen <david@pgguru.net> — 2022-03-22T18:15:34Z

    > On Mar 21, 2022, at 7:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Andres Freund <andres@anarazel.de> 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

  18. Re: [PATCH] Proof of concept for GUC improvements

    Andres Freund <andres@anarazel.de> — 2022-03-22T19:04:06Z

    On 2022-03-22 13:15:34 -0500, David Christensen wrote:
> > On Mar 21, 2022, at 7:53 PM, Tom Lane <tgl@sss.pgh.pa.us> 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...