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:

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>

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():

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.

--

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

-- 
- David Steele
da...@pgmasters.net


Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to