[patch] [doc] Clarify that signal functions have no feedback

Started by David G. Johnstonover 5 years ago6 messages
#1David G. Johnston
david.g.johnston@gmail.com
1 attachment(s)

Hackers,

Over in Bug# 16652 [1]/messages/by-id/16652-58dd6028047058a6@postgresql.org Christoph failed to recognize the fact that signal
sending functions are inherently one-way just as signals are. It seems
worth heading off this situation in the future by making it clear how
signals behave and, in the specific case of pg_reload_conf, that the
important feedback one would hope to get out of a success/failure response
from the function call must instead be found in other locations.

Please see the attached patch, included inline as well.

David J.

[1]: /messages/by-id/16652-58dd6028047058a6@postgresql.org
/messages/by-id/16652-58dd6028047058a6@postgresql.org

commit 6f0ba7c8fd131c906669882e4402930e548e4522
Author: David G. Johnston <david.g.johnston@gmail.com>
Date: Mon Oct 12 22:35:38 2020 +0000

Clarify that signal functions have no feedback

Bug# 16652 complains that the definition of success for pg_reload_conf
doesn't include the outcome of actually reloading the configuration
files. While this is a fairly easy gap to cross given knowledge of
signals, being more explicit here doesn't hurt.

Additionally, because of the special nature of pg_reload_conf, add
links to the various locations where information related to the
success or failure of a reload can be found. Lacking an existing
holistic location in the documentation to point the reader just
list the three resources explicitly.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7cff980dd..75ff8acc93 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23927,7 +23927,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
    <para>
     The functions shown in <xref
-    linkend="functions-admin-signal-table"/> send control signals to
+    linkend="functions-admin-signal-table"/> send uni-directional
+    control signals to
     other server processes.  Use of these functions is restricted to
     superusers by default but access may be granted to others using
     <command>GRANT</command>, with noted exceptions.
@@ -23935,7 +23936,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
    <para>
     Each of these functions returns <literal>true</literal> if
-    successful and <literal>false</literal> otherwise.
+    the signal was successfully sent and <literal>false</literal>
+    if the sending of the signal failed.
    </para>
    <table id="functions-admin-signal-table">
@@ -23983,7 +23985,14 @@ SELECT collation for ('foo' COLLATE "de_DE");
         server to reload their configuration files.  (This is initiated by
         sending a <systemitem>SIGHUP</systemitem> signal to the postmaster
         process, which in turn sends <systemitem>SIGHUP</systemitem> to
each
-        of its children.)
+        of its children.)  Inspection of the
+        <link linkend="runtime-config-logging">log file</link>,
+        <link linkend="view-pg-file-settings">pg_file_settings view</link>,
+        and the
+        <link linkend="view-pg-settings">pg_settings view</link>,
+        is recommended before and/or after executing
+        this function to detect whether there are any issues in the
configuration
+        files preventing some of all of their setting changes from taking
effect.
        </para></entry>
       </row>

Attachments:

v1-doc-pg-reload-conf-signaling.patchapplication/octet-stream; name=v1-doc-pg-reload-conf-signaling.patchDownload
commit 6f0ba7c8fd131c906669882e4402930e548e4522
Author: David G. Johnston <david.g.johnston@gmail.com>
Date:   Mon Oct 12 22:35:38 2020 +0000

    Clarify that signal functions have no feedback
    
    Bug# 16652 complains that the definition of success for pg_reload_conf
    doesn't include the outcome of actually reloading the configuration
    files.  While this is a fairly easy gap to cross given knowledge of
    signals, being more explicit here doesn't hurt.
    
    Additionally, because of the special nature of pg_reload_conf, add
    links to the various locations where information related to the
    success or failure of a reload can be found.  Lacking an existing
    holistic location in the documentation to point the reader just
    list the three resources explicitly.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7cff980dd..75ff8acc93 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23927,7 +23927,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
 
    <para>
     The functions shown in <xref
-    linkend="functions-admin-signal-table"/> send control signals to
+    linkend="functions-admin-signal-table"/> send uni-directional
+    control signals to
     other server processes.  Use of these functions is restricted to
     superusers by default but access may be granted to others using
     <command>GRANT</command>, with noted exceptions.
@@ -23935,7 +23936,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
 
    <para>
     Each of these functions returns <literal>true</literal> if
-    successful and <literal>false</literal> otherwise.
+    the signal was successfully sent and <literal>false</literal>
+    if the sending of the signal failed.
    </para>
 
    <table id="functions-admin-signal-table">
@@ -23983,7 +23985,14 @@ SELECT collation for ('foo' COLLATE "de_DE");
         server to reload their configuration files.  (This is initiated by
         sending a <systemitem>SIGHUP</systemitem> signal to the postmaster
         process, which in turn sends <systemitem>SIGHUP</systemitem> to each
-        of its children.)
+        of its children.)  Inspection of the
+        <link linkend="runtime-config-logging">log file</link>, 
+        <link linkend="view-pg-file-settings">pg_file_settings view</link>,
+        and the
+        <link linkend="view-pg-settings">pg_settings view</link>,
+        is recommended before and/or after executing
+        this function to detect whether there are any issues in the configuration
+        files preventing some of all of their setting changes from taking effect.
        </para></entry>
       </row>
 
#2Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: David G. Johnston (#1)
Re: [patch] [doc] Clarify that signal functions have no feedback

On 2020-10-13 00:43, David G. Johnston wrote:

Over in Bug# 16652 [1] Christoph failed to recognize the fact that
signal sending functions are inherently one-way just as signals are.  It
seems worth heading off this situation in the future by making it clear
how signals behave and, in the specific case of pg_reload_conf, that the
important feedback one would hope to get out of a success/failure
response from the function call must instead be found in other locations.

I agree that the documentation could be improved here. But I don't see
how the added advice actually helps in practice. How can you detect
reload errors by inspecting pg_settings etc.?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#3David G. Johnston
david.g.johnston@gmail.com
In reply to: Peter Eisentraut (#2)
1 attachment(s)
Re: [patch] [doc] Clarify that signal functions have no feedback

On Tue, Oct 27, 2020 at 1:19 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

On 2020-10-13 00:43, David G. Johnston wrote:

Over in Bug# 16652 [1] Christoph failed to recognize the fact that
signal sending functions are inherently one-way just as signals are. It
seems worth heading off this situation in the future by making it clear
how signals behave and, in the specific case of pg_reload_conf, that the
important feedback one would hope to get out of a success/failure
response from the function call must instead be found in other locations.

I agree that the documentation could be improved here. But I don't see
how the added advice actually helps in practice. How can you detect
reload errors by inspecting pg_settings etc.?

I decided I was trying to be too thorough here by including stuff other
than the file related view added mainly for this purpose (of which I missed
including the one pertinent to the bug report - pg_hba_file_rules).

Attached is a version 2 patch listing only pg_hba_file_rules and
pg_file_settings as the "before reload" places (as they do show current
file contents) to validate that the server understands the newly changed
contents of the pg_hba.conf file and the configuration settings.

David J.

Attachments:

v2-doc-pg-reload-conf-signaling.patchapplication/octet-stream; name=v2-doc-pg-reload-conf-signaling.patchDownload
commit f77b42a4d01ba59017e01d30c145eb4a78f83565
Author: David G. Johnston <david.g.johnston@gmail.com>
Date:   Mon Nov 2 15:55:39 2020 +0000

    Clarify the signal functions have no feedback
    
    Bug# 16652 complains that the definition of success for pg_reload_conf
    doesn't include the outcome of actually reloading the configuration
    files.  While this is a fairly easy gap to cross given knowledge of
    signals, being more explicit here doesn't hurt.
    
    Additionally, because of the special nature of pg_reload_conf, add
    links to the various locations where information related to the
    success or failure of a reload can be found.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index bf6004f321..43bc2cf086 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23892,7 +23892,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
 
    <para>
     The functions shown in <xref
-    linkend="functions-admin-signal-table"/> send control signals to
+    linkend="functions-admin-signal-table"/> send uni-directional
+    control signals to
     other server processes.  Use of these functions is restricted to
     superusers by default but access may be granted to others using
     <command>GRANT</command>, with noted exceptions.
@@ -23900,7 +23901,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
 
    <para>
     Each of these functions returns <literal>true</literal> if
-    successful and <literal>false</literal> otherwise.
+    the signal was successfully sent and <literal>false</literal>
+    if the sending of the signal failed.
    </para>
 
    <table id="functions-admin-signal-table">
@@ -23948,7 +23950,11 @@ SELECT collation for ('foo' COLLATE "de_DE");
         server to reload their configuration files.  (This is initiated by
         sending a <systemitem>SIGHUP</systemitem> signal to the postmaster
         process, which in turn sends <systemitem>SIGHUP</systemitem> to each
-        of its children.)
+        of its children.)  Inspection of the relevant
+        <link linkend="view-pg-file-settings">pg_file_settings</link>
+        or
+        <link linkend="view-pg-hba-file-rules">pg_hba_file_rules</link> views
+        is recommended after making changes but before signaling the server.
        </para></entry>
       </row>
 
#4Heikki Linnakangas
hlinnaka@iki.fi
In reply to: David G. Johnston (#3)
Re: [patch] [doc] Clarify that signal functions have no feedback

On 02/11/2020 18:02, David G. Johnston wrote:

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index bf6004f321..43bc2cf086 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23892,7 +23892,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
<para>
The functions shown in <xref
-    linkend="functions-admin-signal-table"/> send control signals to
+    linkend="functions-admin-signal-table"/> send uni-directional
+    control signals to
other server processes.  Use of these functions is restricted to
superusers by default but access may be granted to others using
<command>GRANT</command>, with noted exceptions.

The "uni-directional" sounds a bit redundant, "send" implies that it's
uni-directional I think.

@@ -23900,7 +23901,8 @@ SELECT collation for ('foo' COLLATE "de_DE");

<para>
Each of these functions returns <literal>true</literal> if
-    successful and <literal>false</literal> otherwise.
+    the signal was successfully sent and <literal>false</literal>
+    if the sending of the signal failed.
</para>

This is a good clarification.

<table id="functions-admin-signal-table">
@@ -23948,7 +23950,11 @@ SELECT collation for ('foo' COLLATE "de_DE");
server to reload their configuration files.  (This is initiated by
sending a <systemitem>SIGHUP</systemitem> signal to the postmaster
process, which in turn sends <systemitem>SIGHUP</systemitem> to each
-        of its children.)
+        of its children.)  Inspection of the relevant
+        <link linkend="view-pg-file-settings">pg_file_settings</link>
+        or
+        <link linkend="view-pg-hba-file-rules">pg_hba_file_rules</link> views
+        is recommended after making changes but before signaling the server.
</para></entry>
</row>

I don't understand this recommendation. What is the user supposed to
look for in those views? And why before signaling the server?

[me reads what those views do]. Oh, I see, the idea is that you can use
those views to check the configuration for errors, before applying the
changes. How about this:

You can use the <link
linkend="view-pg-file-settings">pg_file_settings</link> and <link
linkend="view-pg-hba-file-rules">pg_hba_file_rules</link> views to check
the configuration files for possible errors, before reloading.

- Heikki

#5David G. Johnston
david.g.johnston@gmail.com
In reply to: Heikki Linnakangas (#4)
1 attachment(s)
Re: [patch] [doc] Clarify that signal functions have no feedback

On Tue, Nov 17, 2020 at 6:13 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 02/11/2020 18:02, David G. Johnston wrote:

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index bf6004f321..43bc2cf086 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23892,7 +23892,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
<para>
The functions shown in <xref
-    linkend="functions-admin-signal-table"/> send control signals to
+    linkend="functions-admin-signal-table"/> send uni-directional
+    control signals to
other server processes.  Use of these functions is restricted to
superusers by default but access may be granted to others using
<command>GRANT</command>, with noted exceptions.

The "uni-directional" sounds a bit redundant, "send" implies that it's
uni-directional I think.

Agreed, the other two changes sufficiently address the original complaint.

You can use the <link

linkend="view-pg-file-settings">pg_file_settings</link> and <link
linkend="view-pg-hba-file-rules">pg_hba_file_rules</link> views to check
the configuration files for possible errors, before reloading.

I agree with adding "why" you want to check those links, and added a bit
about why doing so before reloading works, but the phrasing using "You"
seems out-of-place in this region of the documentation.

v3 attached

David J.

Attachments:

v3-doc-pg-reload-conf-signaling.patchapplication/octet-stream; name=v3-doc-pg-reload-conf-signaling.patchDownload
commit aee7ed858a4dbf6fde324dc66b15a862113800b8
Author: David G. Johnston <david.g.johnston@gmail.com>
Date:   Mon Nov 2 15:55:39 2020 +0000

    Clarify the signal functions have no feedback
    
    Bug# 16652 complains that the definition of success for pg_reload_conf
    doesn't include the outcome of actually reloading the configuration
    files.  While this is a fairly easy gap to cross given knowledge of
    signals, being more explicit here doesn't hurt.
    
    Additionally, because of the special nature of pg_reload_conf, add
    links to the various locations where information related to the
    success or failure of a reload can be found.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index bf6004f321..2b3aed12dc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23900,7 +23900,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
 
    <para>
     Each of these functions returns <literal>true</literal> if
-    successful and <literal>false</literal> otherwise.
+    the signal was successfully sent and <literal>false</literal>
+    if the sending of the signal failed.
    </para>
 
    <table id="functions-admin-signal-table">
@@ -23948,7 +23949,12 @@ SELECT collation for ('foo' COLLATE "de_DE");
         server to reload their configuration files.  (This is initiated by
         sending a <systemitem>SIGHUP</systemitem> signal to the postmaster
         process, which in turn sends <systemitem>SIGHUP</systemitem> to each
-        of its children.)
+        of its children.)  To check the configuration files for errors, inspect
+        the <link linkend="view-pg-file-settings">pg_file_settings</link>
+        and
+        <link linkend="view-pg-hba-file-rules">pg_hba_file_rules</link> views.
+        This can be done before reloading as those views display live file
+        contents.
        </para></entry>
       </row>
 
#6Heikki Linnakangas
hlinnaka@iki.fi
In reply to: David G. Johnston (#5)
Re: [patch] [doc] Clarify that signal functions have no feedback

On 17/11/2020 21:50, David G. Johnston wrote:

On Tue, Nov 17, 2020 at 6:13 AM Heikki Linnakangas <hlinnaka@iki.fi
<mailto:hlinnaka@iki.fi>> wrote:
You can use the <link
linkend="view-pg-file-settings">pg_file_settings</link> and <link
linkend="view-pg-hba-file-rules">pg_hba_file_rules</link> views to
check
the configuration files for possible errors, before reloading.

I agree with adding "why" you want to check those links, and added a bit
about why doing so before reloading works, but the phrasing using "You"
seems out-of-place in this region of the documentation.

There are plenty of "you"s in the docs. Matter of taste, for sure, but
I'd prefer more active voice. Me being stubborn, I pushed this using my
wording. :-)

Thanks!

- Heikki