Re: Proposal: knowing detail of config files via SQL

Sawada,

* Sawada Masahiko (sawada(dot)mshk(at)gmail(dot)com) wrote:
> Attached patch is latest version including following changes.
> - This view is available to super use only
> - Add sourceline coulmn

Alright, first off, to Josh's point- I'm definitely interested in a
capability to show where the heck a given config value is coming from.
I'd be even happier if there was a way to *force* where config values
have to come from, but that ship sailed a year ago or so. Having this
be for the files, specifically, seems perfectly reasonable to me. I
could see having another function which will report where a currently
set GUC variable came from (eg: ALTER ROLE or ALTER DATABASE), but
having a function for "which config file is this GUC set in" seems
perfectly reasonable to me.

To that point, here's a review of this patch.

> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
> index 6df6bf2..f628cb0 100644
> --- a/src/backend/catalog/system_views.sql
> +++ b/src/backend/catalog/system_views.sql
> @@ -412,6 +412,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;

While this generally "works", the usual expectation is that functions
that should be superuser-only have a check in the function rather than
depending on the execute privilege. I'm certainly happy to debate the
merits of that approach, but for the purposes of this patch, I'd suggest
you stick an if (!superuser()) ereport("must be superuser") into the
function itself and not work about setting the correct permissions on
it.

> + ConfigFileVariable *guc;

As this ends up being an array, I'd call it "guc_array" or something
along those lines. GUC in PG-land has a pretty specific single-entity
meaning.

> @@ -344,6 +346,21 @@ ProcessConfigFile(GucContext context)
> PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
> }
>
> + guc_file_variables = (ConfigFileVariable *)
> + guc_malloc(FATAL, num_guc_file_variables * sizeof(struct ConfigFileVariable));

Uh, perhaps I missed it, but what happens on a reload? Aren't we going
to realloc this every time? Seems like we should be doing a
guc_malloc() the first time through but doing guc_realloc() after, or
we'll leak memory on every reload.

> + /*
> + * Apply guc config parameters to guc_file_variable
> + */
> + guc = guc_file_variables;
> + for (item = head; item; item = item->next, guc++)
> + {
> + guc->name = guc_strdup(FATAL, item->name);
> + guc->value = guc_strdup(FATAL, item->value);
> + guc->filename = guc_strdup(FATAL, item->filename);
> + guc->sourceline = item->sourceline;
> + }

Uh, ditto and double-down here. I don't see a great solution other than
looping through the previous array and free'ing each of these, since we
can't depend on the memory context machinery being up and ready at this
point, as I recall.

> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 931993c..c69ce66 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -3561,9 +3561,17 @@ static const char *const map_old_guc_names[] = {
> */
> static struct config_generic **guc_variables;
>
> +/*
> + * lookup of variables for pg_file_settings view.
> + */
> +static struct ConfigFileVariable *guc_file_variables;
> +
> /* Current number of variables contained in the vector */
> static int num_guc_variables;
>
> +/* Number of file variables */
> +static int num_guc_file_variables;
> +

I'd put these together, and add a comment explaining that
guc_file_variables is an array of length num_guc_file_variables..

> /*
> + * Return the total number of GUC File variables
> + */
> +int
> +GetNumConfigFileOptions(void)
> +{
> + return num_guc_file_variables;
> +}

I don't see the point of this function..

> +Datum
> +show_all_file_settings(PG_FUNCTION_ARGS)
> +{
> + FuncCallContext *funcctx;
> + TupleDesc tupdesc;
> + int call_cntr;
> + int max_calls;
> + AttInMetadata *attinmeta;
> + MemoryContext oldcontext;
> +
> + if (SRF_IS_FIRSTCALL())
> + {
> + funcctx = SRF_FIRSTCALL_INIT();
> +
> + oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
> +
> + /*
> + * need a tuple descriptor representing NUM_PG_SETTINGS_ATTS columns
> + * of the appropriate types
> + */
> +
> + tupdesc = CreateTemplateTupleDesc(NUM_PG_FILE_SETTINGS_ATTS, false);
> + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
> + TEXTOID, -1, 0);
> + TupleDescInitEntry(tupdesc, (AttrNumber) 2, "setting",
> + TEXTOID, -1, 0);
> + TupleDescInitEntry(tupdesc, (AttrNumber) 3, "sourcefile",
> + TEXTOID, -1, 0);
> + TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sourceline",
> + INT4OID, -1, 0);

As the sourcefile and sourceline were discussed as being the 'key' for
this, I expected them to be the first columns.. Any reason to not do
that? It's certainly look cleaner, at least to me, that way.

> + if (call_cntr < max_calls)
> + {
> + char *values[NUM_PG_FILE_SETTINGS_ATTS];
> + HeapTuple tuple;
> + Datum result;
> + ConfigFileVariable conf;
> + char buffer[256];
> +
> + conf = guc_file_variables[call_cntr];

I'm a bit nervous that a sighup in the middle could screw this up. Have
you considered that? At a minimum, I'd suggest a check along the lines
of:

if (call_cntr > num_guc_file_variables)
SRF_RETURNDONE();

To try and avoid going past the end of the array.

> + values[0] = conf.name;
> + values[1] = conf.value;
> + values[2] = conf.filename;
> + snprintf(buffer, sizeof(buffer), "%d", conf.sourceline);

Seems like we might be able to use pg_ltoa() there..

> + if (call_cntr >= max_calls)
> + SRF_RETURN_DONE(funcctx);

Isn't this redundant? We wouldn't be here if this was true.

Just a quick review.

Thanks!

Stephen