Re: Improve GetConfigOptionValues function

> Yes, the existing caller isn't using the fetched values when noshow is
> set to true. However, I think returning from GetConfigOptionValues()
> when noshow is set to true without fetching values limits the use of
> the function. What if someother caller wants to use the function to
> get the values with noshow passed in and use the values when noshow is
> set to true?

I agree that it limits the use of the function but I feel it is good
to focus on the existing use cases and modify the functions
accordingly. In future, if such a use case occurs, then the author
should take care of modifying the required functions. The idea
suggested by Tom to refactor the function looks good as it aligns with
current use cases and it can be used in future cases as you were
suggesting.

> Also, do we gain anything with the patch? I mean, can
> show_all_settings()/pg_settings/pg_show_all_settings() get faster, say
> with a non-superuser without pg_read_all_settings predefined role or
> with a superuser? I see there're about 6 GUC_NO_SHOW_ALL GUCs and 20
> GUC_SUPERUSER_ONLY GUCs, I'm not sure if it leads to some benefit with
> the patch.

As the number of such parameters (GUC_NO_SHOW_ALL and
GUC_SUPERUSER_ONLY) are less, we may not see improvements in
performance. We can treat it as a kind of refactoring.

Thanks & Regards,
Nitin Jadhav

On Wed, Jan 18, 2023 at 1:47 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Jan 18, 2023 at 1:24 PM Nitin Jadhav
> <nitinjadhavpostgres(at)gmail(dot)com> wrote:
> >
> > Attaching the patch.
> >
> > On Wed, Jan 18, 2023 at 1:21 PM Nitin Jadhav
> > <nitinjadhavpostgres(at)gmail(dot)com> wrote:
> > >
> > > Hi,
> > >
> > > GetConfigOptionValues function extracts the config parameters for the
> > > given variable irrespective of whether it results in noshow or not.
> > > But the parent function show_all_settings ignores the values parameter
> > > if it results in noshow. It's unnecessary to fetch all the values
> > > during noshow. So a return statement in GetConfigOptionValues() when
> > > noshow is set to true is needed. Attached the patch for the same.
> > > Please share your thoughts.
>
> Yes, the existing caller isn't using the fetched values when noshow is
> set to true. However, I think returning from GetConfigOptionValues()
> when noshow is set to true without fetching values limits the use of
> the function. What if someother caller wants to use the function to
> get the values with noshow passed in and use the values when noshow is
> set to true?
>
> Also, do we gain anything with the patch? I mean, can
> show_all_settings()/pg_settings/pg_show_all_settings() get faster, say
> with a non-superuser without pg_read_all_settings predefined role or
> with a superuser? I see there're about 6 GUC_NO_SHOW_ALL GUCs and 20
> GUC_SUPERUSER_ONLY GUCs, I'm not sure if it leads to some benefit with
> the patch.
>
> Having said above, I don't mind keeping the things the way they're right now.
>
> --
> Bharath Rupireddy
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com