> We could extend pg_show_all_settings() with a boolean parameter to > enforce listing all the parameters, even the ones that are marked as > NOSHOW, but this does not count on GetConfigOptionValues() that could > force a parameter to become noshow on a superuser-only GUC depending > on the role that's running the function. At the end, we'd better rely > on a separate superuser-only function to do this job, aka option 1.
I had started a separate thread [1] to refactor the code around GetConfigOptionValues() and the patch is already committed. Now it makes our job simpler to extend pg_show_all_settings() with a boolean parameter to enforce listing all the parameters, even the ones that are marked as NOSHOW. I have attached the patch for the same. Kindly look into it and share your thoughts. [1]: https://www.postgresql.org/message-id/flat/CALj2ACXZMOGEtjk%2Beh0Zeiz%3D46ETVkr0koYAjWt8SoJUJJUe9g%40mail.gmail.com#04705e421e0dc63b1f0c862ae4929e6f Thanks & Regards, Nitin Jadhav On Wed, Jan 18, 2023 at 12:31 PM Nitin Jadhav <nitinjadhavpostg...@gmail.com> wrote: > > > We would miss the names of the parameters that are marked as NO_SHOW, > > missing from pg_settings, making debugging harder. > > > > This would make the test more costly by forcing one SQL for each > > GUC.. > > I agree. > > > > We could extend pg_show_all_settings() with a boolean parameter to > > enforce listing all the parameters, even the ones that are marked as > > NOSHOW, but this does not count on GetConfigOptionValues() that could > > force a parameter to become noshow on a superuser-only GUC depending > > on the role that's running the function. At the end, we'd better rely > > on a separate superuser-only function to do this job, aka option 1. > > I did not get it completely. To understand it better, I just gave a > thought of adding a boolean parameter to pg_show_all_settings(). Then > we should modify GetConfigOptionValues() like below [1]. When we call > pg_show_all_settings(false), it behaves like existing behaviour (with > super user and without super user). When we call > pg_show_all_settings(true) with super user privileges, it returns all > parameters including GUC_NO_SHOW_ALL as well as GUC_SUPER_USER_ONLY. > If we call pg_show_all_settings(true) without super user privileges, > then it returns all parameters except GUC_NO_SHOW_ALL and > GUC_SUPER_USER_ONLY. Can't we do it this way? Please share your > thoughts. > > > > How much do we need to care with the duplication this would involve > > with show_all_settings(), actually? If you don't use the SRF macros, > > the code would just be a couple of lines with InitMaterializedSRF() > > doing a loop on GetConfigOptionValues(). Even if that means listing > > twice the parameters in pg_proc.dat, the chances of adding new > > parameters in pg_settings is rather low so that would be a one-time > > change? > > How about just fetching the parameter name instead of fetching all its > details. This will meet our objective as well as it controls the code > duplication. > > [1]: > static void > GetConfigOptionValues(struct config_generic *conf, const char **values, > bool *noshow, bool is_show_all) > { > char buffer[256]; > > if (noshow) > { > if (((conf->flags & GUC_NO_SHOW_ALL) && !is_show_all) || > ((conf->flags & GUC_NO_SHOW_ALL) && > !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) || > ((conf->flags & GUC_SUPERUSER_ONLY) && > !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) > *noshow = true; > else > *noshow = false; > } > - > - > - > } > > On Mon, Jan 16, 2023 at 7:58 AM Michael Paquier <mich...@paquier.xyz> wrote: > > > > On Sat, Jan 14, 2023 at 07:10:55PM +0530, Nitin Jadhav wrote: > > > Option-1 is, expose a function like pg_settings_get_no_show_all() > > > which just returns the parameters which are just listed as > > > GUC_NO_SHOW_ALL (Not in combination with NOT_IN_SAMPLE). We can then > > > use this function in the test file and verify whether there are config > > > entries for these. > > > > > > Option-2 is, if exposing new function and that too to expose > > > parameters which are listed as GUC_NO_SHOW_ALL is not recommended, > > > then how about exposing a function like pg_settings_get_count() which > > > returns the count of all parameters including GUC_NO_SHOW_ALL. We can > > > then use this number to verify whether these many are present in the > > > sample config file. But we cannot show the name of the parameters if > > > it is not matching. We can just display an error saying "Parameter > > > with GUC_NO_SHOW_ALL is missing from postgresql.conf.sample". > > > > We would miss the names of the parameters that are marked as NO_SHOW, > > missing from pg_settings, making debugging harder. > > > > > Option-3 is, if exposing both of the above functions is not > > > recommended, then we can use the existing function > > > pg_settings_get_flags() for each of the parameters while reading the > > > sample config file in 003_check_guc.pl. This validates the > > > GUC_NO_SHOW_ALL parameter if that is present in the sample config > > > file. It does not validate if it is present in guc.c and missing in > > > the sample config file. > > > > This would make the test more costly by forcing one SQL for each > > GUC.. > > > > > Option-4 is, how about manually adding the parameter name to > > > 'all_params_array' in 003_check_guc.pl whenever we add such GUCs. > > > > > > I am not able to choose any of the above options as each has some > > > disadvantages but if no other options exist, then I would like to go > > > with option-3 as it validates more than the one currently doing. > > > Please share if any other better ideas. > > > > We could extend pg_show_all_settings() with a boolean parameter to > > enforce listing all the parameters, even the ones that are marked as > > NOSHOW, but this does not count on GetConfigOptionValues() that could > > force a parameter to become noshow on a superuser-only GUC depending > > on the role that's running the function. At the end, we'd better rely > > on a separate superuser-only function to do this job, aka option 1. > > > > How much do we need to care with the duplication this would involve > > with show_all_settings(), actually? If you don't use the SRF macros, > > the code would just be a couple of lines with InitMaterializedSRF() > > doing a loop on GetConfigOptionValues(). Even if that means listing > > twice the parameters in pg_proc.dat, the chances of adding new > > parameters in pg_settings is rather low so that would be a one-time > > change? > > -- > > Michael
v1-0001-Fix-GUC_NO_SHOW_ALL-test-scenario-in-003_check_guc.p.patch
Description: Binary data
