On Sat, Apr 25, 2015 at 3:40 AM, David Steele <da...@pgmasters.net> wrote: > On 4/4/15 9:21 AM, Sawada Masahiko wrote: >> 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. > Below is my review of the v5 patch:
Thank you for your review comment!
The latest patch is attached.
> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> + <sect1 id="view-pg-file-settings">
> + <title><structname>pg_file_settings</structname></title>
> +
> + <indexterm zone="view-pg-file-settings">
> + <primary>pg_file_settings</primary>
> + </indexterm>
> +
> + <para>
> + The view <structname>pg_file_settings</structname> provides confirm
> parameters via SQL,
> + which is written in configuration file. This view can be update by
> reloading configration file.
> + </para>
>
> Perhaps something like:
>
> <para>
> The view <structname>pg_file_settings</structname> provides access to
> run-time parameters
> that are defined in configuration files via SQL. In contrast to
> <structname>pg_settings</structname> a row is provided for each
> occurrence
> of the parameter in a configuration file. This is helpful for
> discovering why one value
> may have been used in preference to another when the parameters were
> loaded.
> </para>
>
> + <table>
> + <title><structname>pg_file_settings</> Columns</title>
> +
> + <tgroup cols="3">
> + <thead>
> + <row>
> + <entry>Name</entry>
> + <entry>Type</entry>
> + <entry>Description</entry>
> + </row>
> + </thead>
> + <tbody>
> + <row>
> + <entry><structfield>sourcefile</structfield></entry>
> + <entry><structfield>text</structfield></entry>
> + <entry>A path of configration file</entry>
>
> From pg_settings:
>
> <entry>Configuration file the current value was set in (null for
> values set from sources other than configuration files, or when
> examined by a non-superuser);
> helpful when using <literal>include</> directives in configuration
> files</entry>
>
> + </row>
> + <row>
> + <entry><structfield>sourceline</structfield></entry>
> + <entry><structfield>integer</structfield></entry>
> + <entry>The line number in configuration file</entry>
>
> From pg_settings:
>
> <entry>Line number within the configuration file the current value was
> set at (null for values set from sources other than configuration
> files,
> or when examined by a non-superuser)
> </entry>
>
>
> + </row>
> + <row>
> + <entry><structfield>name</structfield></entry>
> + <entry><structfield>text</structfield></entry>
> + <entry>Parameter name in configuration file</entry>
>
> From pg_settings:
>
> <entry>Run-time configuration parameter name</entry>
>
> + </row>
> + <row>
> + <entry><structfield>setting</structfield></entry>
> + <entry><structfield>text</structfield></entry>
> + <entry>Value of the parameter in configuration file</entry>
> + </row>
> + </tbody>
> + </tgroup>
> + </table>
> +
> +</sect1>
The documentation is updated based on your suggestion.
> diff --git a/src/backend/utils/misc/guc-file.l
> b/src/backend/utils/misc/guc-file.l
> @@ -342,6 +345,42 @@ ProcessConfigFile(GucContext context)
> + 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);
>
> There's an issue freeing these when calling pg_reload_conf():
Fix.
> postgres=# alter system set max_connections = 100;
> ALTER SYSTEM
> postgres=# select * from pg_reload_conf();
> LOG: received SIGHUP, reloading configuration files
> pg_reload_conf
> ----------------
> t
> (1 row)
>
> postgres=# postgres(25078,0x7fff747b2300) malloc: *** error for object
> 0x424d380044: pointer being freed was not allocated
> *** set a breakpoint in malloc_error_break to debug
>
> Of course a config reload can't change max_connections, but I wouldn't
> expect it to crash, either.
>
> + guc_array->sourceline = -1;
>
> I can't see the need for this since it is reassigned further down.
Fix.
> --
>
> Up-thread David J had recommended an ordering column (or seqno) that
> would provide the order in which the settings were loaded. I think this
> would give valuable information that can't be gleaned from the line
> numbers alone.
>
> Personally I think it would be fine to start from 1 and increment for
> each setting found, rather than rank within a setting. If the user
> wants per setting ranking that's what SQL is for. So the output would
> look something like:
>
> sourcefile | sourceline | seqno | name | setting
> --------------------------+------------+-------+-----------------+-----------
> /db/postgresql.conf | 64 | 1 | max_connections | 100
> /db/postgresql.conf | 116 | 2 | shared_buffers | 128MB
> /db/postgresql.conf | 446 | 3 | log_timezone |
> US/Eastern
> /db/postgresql.auto.conf | 3 | 4 | max_connections | 200
>
Yep, I agree with you.
Added seqno column into pg_file_settings view.
Regards,
-------
Sawada Masahiko
000_pg_file_settings_view_v6.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
