Re: Proposal: knowing detail of config files via SQL

On Wed, Mar 11, 2015 at 11:46 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> On Tue, Mar 10, 2015 at 12:08 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> Sawada,
>>
>> * Sawada Masahiko (sawada.mshk@gmail.com) wrote:
>>> Thank you for your review!
>>> Attached file is the latest version (without document patch. I making it now.)
>>> As per discussion, there is no change regarding of super user permission.
>>
>> Ok.  Here's another review.
>>
>
> Thank you for your review!
>
>>> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
>>> index 5e69e2b..94ee229 100644
>>> --- a/src/backend/catalog/system_views.sql
>>> +++ b/src/backend/catalog/system_views.sql
>>> @@ -414,6 +414,11 @@ CREATE RULE pg_settings_n AS
>>>
>>>  GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;
>>>
>>> +CREATE VIEW pg_file_settings AS
>>> +   SELECT * FROM pg_show_all_file_settings() AS A;
>>> +
>>> +REVOKE ALL on pg_file_settings FROM public;
>>> +
>>
>> This suffers from a lack of pg_dump support, presuming that the
>> superuser is entitled to change the permissions on this function.  As it
>> turns out though, you're in luck in that regard since I'm working on
>> fixing that for a mostly unrelated patch.  Any feedback you have on what
>> I'm doing to pg_dump here:
>>
>> http://www.postgresql.org/message-id/20150307213935.GO29780@tamriel.snowman.net
>>
>> Would certainly be appreciated.
>>
>
> Thank you for the info.
> I will read the discussion and send the feedbacks.
>
>>> diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
>> [...]
>>> +      * Calculate size of guc_array and allocate it. From the secound time to allcate memory,
>>> +      * we should free old allocated memory.
>>> +      */
>>> +     guc_array_size = num_guc_file_variables * sizeof(struct ConfigFileVariable);
>>> +     if (!guc_file_variables)
>>> +     {
>>> +             /* For the first call */
>>> +             guc_file_variables = (ConfigFileVariable *) guc_malloc(FATAL, guc_array_size);
>>> +     }
>>> +     else
>>> +     {
>>> +             guc_array = guc_file_variables;
>>> +             for (item = head; item; item = item->next, guc_array++)
>>> +             {
>>> +                     free(guc_array->name);
>>> +                     free(guc_array->value);
>>> +                     free(guc_array->filename);
>>
>> It's a minor nit-pick, as the below loop should handle anything we care
>> about, but I'd go ahead and reset guc_array->sourceline to be -1 or
>> something too, just to show that everything's being handled here with
>> regard to cleanup.  Or perhaps just add a comment saying that the
>> sourceline for all currently valid entries will be updated.
>
> Fixed.
>
>>
>>> +             guc_file_variables = (ConfigFileVariable *) guc_realloc(FATAL, guc_file_variables, guc_array_size);
>>> +     }
>>
>> Nasby made a comment, I believe, that we might actually be able to use
>> memory contexts here, which would certainly be much nicer as then you'd
>> be able to just throw away the whole context and create a new one.  Have
>> you looked at that approach at all?  Offhand, I'm not sure if it'd work
>> or not (I have my doubts..) but it seems worthwhile to consider.
>>
>
> I successfully used palloc() and pfree() when allocate and free
> guc_file_variable,
> but these variable seems to be freed already when I get value of
> pg_file_settings view via SQL.
>
>> Otherwise, it seems like this would address my previous concerns that
>> this would end up leaking memory, and even if it's a bit slower than one
>> might hope, it's not an operation we should be doing very frequently.
>>
>>> --- a/src/backend/utils/misc/guc.c
>>> +++ b/src/backend/utils/misc/guc.c
>> [...]
>>>  /*
>>> + * Return the total number of GUC File variables
>>> + */
>>> +int
>>> +GetNumConfigFileOptions(void)
>>> +{
>>> +     return num_guc_file_variables;
>>> +}
>>
>> What's the point of this function..?  I'm not convinced it's the best
>> idea, but it certainly looks like the function and the variable are only
>> used from this file anyway?
>
> It's unnecessary.
> Fixed.
>
>>> +     if (call_cntr < max_calls)
>>> +     {
>>> +             char       *values[NUM_PG_FILE_SETTINGS_ATTS];
>>> +             HeapTuple       tuple;
>>> +             Datum           result;
>>> +             ConfigFileVariable conf;
>>> +             char            buffer[256];
>>
>> Isn't that buffer a bit.. excessive in size?
>
> Fixed.
>
>>
>>> diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
>>> index d3100d1..ebb96cc 100644
>>> --- a/src/include/utils/guc.h
>>> +++ b/src/include/utils/guc.h
>>> @@ -133,6 +133,14 @@ typedef struct ConfigVariable
>>>       struct ConfigVariable *next;
>>>  } ConfigVariable;
>>>
>>> +typedef struct ConfigFileVariable
>>> +{
>>> +     char    *name;
>>> +     char    *value;
>>> +     char    *filename;
>>> +     int             sourceline;
>>> +} ConfigFileVariable;
>>> +
>> [...]
>>> +extern int   GetNumConfigFileOptions(void);
>>
>> These aren't actually used anywhere else, are they?  Not sure that
>> there's any need to expose them beyond guc.c when that's the only place
>> they're used.
>>
>
> Fixed.
>
>> This will also need a
>> REVOKE EXECUTE on pg_show_all_file_settings() FROM public;
>
> Added.
>

I added documentation changes to patch is attached.
Also I tried to use memory context for allocation of guc_file_variable
in TopMemoryContext,
but it was failed access after received SIGHUP.

Please review.

Regards,

-------
Sawada Masahiko