Re: Proposal: knowing detail of config files via SQL
От | Sawada Masahiko |
---|---|
Тема | Re: Proposal: knowing detail of config files via SQL |
Дата | |
Msg-id | CAD21AoCV9RxSiQVWDoofC3i0W8EJh_RW5BT+_8yPo+-FoFg+Uw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Proposal: knowing detail of config files via SQL (Sawada Masahiko <sawada.mshk@gmail.com>) |
Ответы |
Re: Proposal: knowing detail of config files via SQL
(David Steele <david@pgmasters.net>)
|
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Haribabu KommiДата:
Сообщение: Re: Providing catalog view to pg_hba.conf file - Patch submission