Re: Proposal: knowing detail of config files via SQL

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.

> 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.

> 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.

> + 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.

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?

> + 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?

> 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.

Thanks!

Stephen