On Sat, Feb 28, 2015 at 2:27 PM, Stephen Frost <sfr...@snowman.net> wrote:
> Sawada,
>
> * Sawada Masahiko (sawada.m...@gmail.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.
>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. -- Regards, ------- Sawada Masahiko
000_pg_file_settings_view_v3.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
