Re: Proposal: knowing detail of config files via SQL

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.

Regards,

-------
Sawada Masahiko