Re: Proposal: knowing detail of config files via SQL

From: Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Johnston <david(dot)g(dot)johnston(at)gmail(dot)com>, David Fetter <david(at)fetter(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Proposal: knowing detail of config files via SQL
Date: 2015-04-04 13:21:42
Message-ID: CAD21AoCV9RxSiQVWDoofC3i0W8EJh_RW5BT+_8yPo+-FoFg+Uw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Mar 11, 2015 at 11:46 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> 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.
>

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

Attachment Content-Type Size
000_pg_file_settings_view_v5.patch application/octet-stream 9.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Haribabu Kommi 2015-04-04 13:29:03 Re: Providing catalog view to pg_hba.conf file - Patch submission
Previous Message Amit Kapila 2015-04-04 12:57:12 Re: TABLESAMPLE patch