Re: Proposal: knowing detail of config files via SQL

On Tue, Mar 10, 2015 at 12:08 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Sawada,
>
> * Sawada Masahiko (sawada(dot)mshk(at)gmail(dot)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