Thread: Improve documentation regarding custom settings, placeholders, and the administrative functions
Improve documentation regarding custom settings, placeholders, and the administrative functions
From
"David G. Johnston"
Date:
Hey!
Motivated by recent user complaints regarding our documentation of behavior in this area I propose the following to shore things up a bit.
There may be other places that need this coverage but it seems the most likely place our users are going to experience this behavior is in using set_config and current_setting.
Mostly I'm pointing out the fact that one can never take the null value to be the actual value of a setting. In terms of current_setting this then establishes the fact that the null value it may return is an error-handling alternative only and not something to be relied upon as being an actual value of the setting.
The change to custom options and set_config point expands on the "placeholder" concept already Introduced and just documents that set_config can and will create such placeholders implicitly.
Also, in realizing that the null value is not technically a valid input for new_value in set_config, a paragraph is added to explain how the null value gets interpreted.
I sorta get not wanting to encourage the use of custom options but saying "have no function" just doesn't make sense. The certainly exist, and as data they don't have any function on their own anyway. The surrounding discussion regarding extensions gets the same point across sufficiently without stating that an external loadable module is required when it is not.
David J.
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 934ef5e469..4478d0aa91 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -23,7 +23,7 @@
<para>
All parameter names are case-insensitive. Every parameter takes a
- value of one of five types: boolean, string, integer, floating point,
+ non-null value of one of five types: boolean, string, integer, floating point,
or enumerated (enum). The type determines the syntax for setting the
parameter:
</para>
@@ -11350,14 +11350,20 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
<para>
Because custom options may need to be set in processes that have not
loaded the relevant extension module, <productname>PostgreSQL</productname>
- will accept a setting for any two-part parameter name. Such variables
- are treated as placeholders and have no function until the module that
- defines them is loaded. When an extension module is loaded, it will add
+ will accept a setting for any two-part parameter name.
+ When an extension module is loaded, it will add
its variable definitions and convert any placeholder values according to
those definitions. If there are any unrecognized placeholders
that begin with its extension name, warnings are issued and those
placeholders are removed.
</para>
+
+ <para>
+ If a placeholder is created in a session it will exist for the
+ lifetime of the session unless removed by an extension.
+ Placeholders have a string data type with a reset value of the empty string.
+ </para>
+
</sect1>
<sect1 id="runtime-config-developer">
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index ad663c94d7..605bf533ee 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28157,7 +28157,7 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
<returnvalue>text</returnvalue>
</para>
<para>
- Returns the current value of the
+ Returns the current non-null value of the
setting <parameter>setting_name</parameter>. If there is no such
setting, <function>current_setting</function> throws an error
unless <parameter>missing_ok</parameter> is supplied and
@@ -28191,6 +28191,17 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
use <literal>false</literal> instead. This function corresponds to
the SQL command <xref linkend="sql-set"/>.
</para>
+ <para>
+ <function>set_config</function> accepts the NULL value for
+ <parameter>new_value</parameter>, but as settings cannot be null this input
+ is interpreted as a request to set the setting to its default value.
+ </para>
+ <para>
+ If <parameter>setting_name</parameter> does not already exist
+ <function>set_config</function> throws an error unless the identifier is a valid
+ <link linkend="runtime-config-custom">custom option</link> name, in which it
+ creates a placeholder with the empty string as its old value.
+ </para>
<para>
<literal>set_config('log_statement_stats', 'off', false)</literal>
<returnvalue>off</returnvalue>
index 934ef5e469..4478d0aa91 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -23,7 +23,7 @@
<para>
All parameter names are case-insensitive. Every parameter takes a
- value of one of five types: boolean, string, integer, floating point,
+ non-null value of one of five types: boolean, string, integer, floating point,
or enumerated (enum). The type determines the syntax for setting the
parameter:
</para>
@@ -11350,14 +11350,20 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
<para>
Because custom options may need to be set in processes that have not
loaded the relevant extension module, <productname>PostgreSQL</productname>
- will accept a setting for any two-part parameter name. Such variables
- are treated as placeholders and have no function until the module that
- defines them is loaded. When an extension module is loaded, it will add
+ will accept a setting for any two-part parameter name.
+ When an extension module is loaded, it will add
its variable definitions and convert any placeholder values according to
those definitions. If there are any unrecognized placeholders
that begin with its extension name, warnings are issued and those
placeholders are removed.
</para>
+
+ <para>
+ If a placeholder is created in a session it will exist for the
+ lifetime of the session unless removed by an extension.
+ Placeholders have a string data type with a reset value of the empty string.
+ </para>
+
</sect1>
<sect1 id="runtime-config-developer">
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index ad663c94d7..605bf533ee 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28157,7 +28157,7 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
<returnvalue>text</returnvalue>
</para>
<para>
- Returns the current value of the
+ Returns the current non-null value of the
setting <parameter>setting_name</parameter>. If there is no such
setting, <function>current_setting</function> throws an error
unless <parameter>missing_ok</parameter> is supplied and
@@ -28191,6 +28191,17 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
use <literal>false</literal> instead. This function corresponds to
the SQL command <xref linkend="sql-set"/>.
</para>
+ <para>
+ <function>set_config</function> accepts the NULL value for
+ <parameter>new_value</parameter>, but as settings cannot be null this input
+ is interpreted as a request to set the setting to its default value.
+ </para>
+ <para>
+ If <parameter>setting_name</parameter> does not already exist
+ <function>set_config</function> throws an error unless the identifier is a valid
+ <link linkend="runtime-config-custom">custom option</link> name, in which it
+ creates a placeholder with the empty string as its old value.
+ </para>
<para>
<literal>set_config('log_statement_stats', 'off', false)</literal>
<returnvalue>off</returnvalue>
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
"David G. Johnston"
Date:
Thoughts? Anyone?
On Sat, Oct 19, 2024 at 1:11 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
Hey!Motivated by recent user complaints regarding our documentation of behavior in this area I propose the following to shore things up a bit.There may be other places that need this coverage but it seems the most likely place our users are going to experience this behavior is in using set_config and current_setting.Mostly I'm pointing out the fact that one can never take the null value to be the actual value of a setting. In terms of current_setting this then establishes the fact that the null value it may return is an error-handling alternative only and not something to be relied upon as being an actual value of the setting.The change to custom options and set_config point expands on the "placeholder" concept already Introduced and just documents that set_config can and will create such placeholders implicitly.Also, in realizing that the null value is not technically a valid input for new_value in set_config, a paragraph is added to explain how the null value gets interpreted.I sorta get not wanting to encourage the use of custom options but saying "have no function" just doesn't make sense. The certainly exist, and as data they don't have any function on their own anyway. The surrounding discussion regarding extensions gets the same point across sufficiently without stating that an external loadable module is required when it is not.David J.diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 934ef5e469..4478d0aa91 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -23,7 +23,7 @@
<para>
All parameter names are case-insensitive. Every parameter takes a
- value of one of five types: boolean, string, integer, floating point,
+ non-null value of one of five types: boolean, string, integer, floating point,
or enumerated (enum). The type determines the syntax for setting the
parameter:
</para>
@@ -11350,14 +11350,20 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
<para>
Because custom options may need to be set in processes that have not
loaded the relevant extension module, <productname>PostgreSQL</productname>
- will accept a setting for any two-part parameter name. Such variables
- are treated as placeholders and have no function until the module that
- defines them is loaded. When an extension module is loaded, it will add
+ will accept a setting for any two-part parameter name.
+ When an extension module is loaded, it will add
its variable definitions and convert any placeholder values according to
those definitions. If there are any unrecognized placeholders
that begin with its extension name, warnings are issued and those
placeholders are removed.
</para>
+
+ <para>
+ If a placeholder is created in a session it will exist for the
+ lifetime of the session unless removed by an extension.
+ Placeholders have a string data type with a reset value of the empty string.
+ </para>
+
</sect1>
<sect1 id="runtime-config-developer">
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index ad663c94d7..605bf533ee 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28157,7 +28157,7 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
<returnvalue>text</returnvalue>
</para>
<para>
- Returns the current value of the
+ Returns the current non-null value of the
setting <parameter>setting_name</parameter>. If there is no such
setting, <function>current_setting</function> throws an error
unless <parameter>missing_ok</parameter> is supplied and
@@ -28191,6 +28191,17 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
use <literal>false</literal> instead. This function corresponds to
the SQL command <xref linkend="sql-set"/>.
</para>
+ <para>
+ <function>set_config</function> accepts the NULL value for
+ <parameter>new_value</parameter>, but as settings cannot be null this input
+ is interpreted as a request to set the setting to its default value.
+ </para>
+ <para>
+ If <parameter>setting_name</parameter> does not already exist
+ <function>set_config</function> throws an error unless the identifier is a valid
+ <link linkend="runtime-config-custom">custom option</link> name, in which it
+ creates a placeholder with the empty string as its old value.
+ </para>
<para>
<literal>set_config('log_statement_stats', 'off', false)</literal>
<returnvalue>off</returnvalue>
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
Zhang Mingli
Date:
On Oct 20, 2024 at 04:12 +0800, David G. Johnston <david.g.johnston@gmail.com>, wrote:
Mostly I'm pointing out the fact that one can never take the null value to be the actual value of a setting. In terms of current_setting this then establishes the fact that the null value it may return is an error-handling alternative only and not something to be relied upon as being an actual value of the setting.
- Returns the current value of the+ Returns the current non-null value of the setting <parameter>setting_name</parameter>. If there is no such setting, <function>current_setting</function> throws an error unless <parameter>missing_ok</parameter> is supplied and
Hi,
current_setting() could return NULL when missing_ok is passed, so is it right to say: Returns the current non-null value of the setting <parameter>setting_name</parameter>?
As the doc is for function of current_setting(), and it could return NULL actually.
gpadmin=# \pset null NULL
Null display is "NULL".
gpadmin=# select current_setting('pg_xmen', true);
current_setting
-----------------
NULL
(1 row)
--
Zhang Mingli
HashData
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
"David G. Johnston"
Date:
On Wed, Feb 5, 2025 at 7:36 PM Zhang Mingli <zmlpostgres@gmail.com> wrote:
On Oct 20, 2024 at 04:12 +0800, David G. Johnston <david.g.johnston@gmail.com>, wrote:
Mostly I'm pointing out the fact that one can never take the null value to be the actual value of a setting. In terms of current_setting this then establishes the fact that the null value it may return is an error-handling alternative only and not something to be relied upon as being an actual value of the setting.- Returns the current value of the+ Returns the current non-null value of the setting <parameter>setting_name</parameter>. If there is no such setting, <function>current_setting</function> throws an error unless <parameter>missing_ok</parameter> is supplied and
Hi,
current_setting() could return NULL when missing_ok is passed, so is it right to say: Returns the current non-null value of the setting <parameter>setting_name</parameter>?
As the doc is for function of current_setting(), and it could return NULL actually.
Yes, and when it does it is doing so in lieu of an error, and thus it is not returning the value of the setting. It does not mean the value taken on by the named parameter is NULL, which is not possible. i.e., SHOW will never produce NULL.
David J.
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
Zhang Mingli
Date:
On Feb 6, 2025 at 10:49 +0800, David G. Johnston <david.g.johnston@gmail.com>, wrote:
Yes, and when it does it is doing so in lieu of an error, and thus it is not returning the value of the setting. It does not mean the value taken on by the named parameter is NULL, which is not possible. i.e., SHOW will never produce NULL.
Hi,
OK, LGTM.
--
Zhang Mingli
HashData
OK, LGTM.
--
Zhang Mingli
HashData
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
"David G. Johnston"
Date:
On Thu, Feb 6, 2025 at 8:04 AM Zhang Mingli <zmlpostgres@gmail.com> wrote:
On Feb 6, 2025 at 10:49 +0800, David G. Johnston <david.g.johnston@gmail.com>, wrote:
Yes, and when it does it is doing so in lieu of an error, and thus it is not returning the value of the setting. It does not mean the value taken on by the named parameter is NULL, which is not possible. i.e., SHOW will never produce NULL.Hi,
OK, LGTM.
Thank you for the review. If you would like to add yourself as the reviewer and mark this ready to commit here is the commitfest entry.
David J.
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
Zhang Mingli
Date:
On Feb 28, 2025 at 02:27 +0800, David G. Johnston <david.g.johnston@gmail.com>, wrote:
On Thu, Feb 6, 2025 at 8:04 AM Zhang Mingli <zmlpostgres@gmail.com> wrote:On Feb 6, 2025 at 10:49 +0800, David G. Johnston <david.g.johnston@gmail.com>, wrote:Hi,
Yes, and when it does it is doing so in lieu of an error, and thus it is not returning the value of the setting. It does not mean the value taken on by the named parameter is NULL, which is not possible. i.e., SHOW will never produce NULL.
OK, LGTM.
Thank you for the review. If you would like to add yourself as the reviewer and mark this ready to commit here is the commitfest entry.
https://commitfest.postgresql.org/patch/5548/
David J.
Hi, Done.
--
Zhang Mingli
HashData
--
Zhang Mingli
HashData
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
Heikki Linnakangas
Date:
On 19/10/2024 23:11, David G. Johnston wrote: > diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml > index 934ef5e469..4478d0aa91 100644 > --- a/doc/src/sgml/config.sgml > +++ b/doc/src/sgml/config.sgml > @@ -23,7 +23,7 @@ > > <para> > All parameter names are case-insensitive. Every parameter takes a > - value of one of five types: boolean, string, integer, floating point, > + non-null value of one of five types: boolean, string, integer, > floating point, > or enumerated (enum). The type determines the syntax for setting the > parameter: > </para> Feels a bit obtuse to be honest. Who would've thought that they can be NULL? They're not regular SQL datatypes, after all. > @@ -11350,14 +11350,20 @@ dynamic_library_path = 'C:\tools\postgresql;H: > \my_project\lib;$libdir' > <para> > Because custom options may need to be set in processes that have not > loaded the relevant extension module, <productname>PostgreSQL</ > productname> > - will accept a setting for any two-part parameter name. Such variables > - are treated as placeholders and have no function until the module that > - defines them is loaded. When an extension module is loaded, it > will add > + will accept a setting for any two-part parameter name. > + When an extension module is loaded, it will add > its variable definitions and convert any placeholder values > according to > those definitions. If there are any unrecognized placeholders > that begin with its extension name, warnings are issued and those > placeholders are removed. > </para> -1, this completely removes the explanation of what a placeholder variable is, but the term placeholder is still used in the text that follows. > + > + <para> > + If a placeholder is created in a session it will exist for the > + lifetime of the session unless removed by an extension. > + Placeholders have a string data type with a reset value of the > empty string. > + </para> Placeholders can be created in postgresql.conf too, so this emphasis on "created in a session" feels a bit off. > diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml > index ad663c94d7..605bf533ee 100644 > --- a/doc/src/sgml/func.sgml > +++ b/doc/src/sgml/func.sgml > @@ -28157,7 +28157,7 @@ acl | {postgres=arwdDxtm/postgres,foo=r/ > postgres} > <returnvalue>text</returnvalue> > </para> > <para> > - Returns the current value of the > + Returns the current non-null value of the > setting <parameter>setting_name</parameter>. If there is no such > setting, <function>current_setting</function> throws an error > unless <parameter>missing_ok</parameter> is supplied and Feels a bit cavalier to mention the "non-null" like this. I understand that you wanted to make it clear that when current_setting() returns NULL, it means that the setting did not exist and you used missing_ok=true. But to deduce that, you still need to know that the value cannot be NULL. Otherwise you're left wondering what the function would return for NULL values, since this only describes what it returns for non-null values. > @@ -28191,6 +28191,17 @@ acl | {postgres=arwdDxtm/postgres,foo=r/ > postgres} > use <literal>false</literal> instead. This function corresponds to > the SQL command <xref linkend="sql-set"/>. > </para> > + <para> > + <function>set_config</function> accepts the NULL value for > + <parameter>new_value</parameter>, but as settings cannot be > null this input > + is interpreted as a request to set the setting to its default > value. > + </para> +1 on this part. Committed this paragraph so that we can make some incremental progress. > + <para> > + If <parameter>setting_name</parameter> does not already exist > + <function>set_config</function> throws an error unless the > identifier is a valid > + <link linkend="runtime-config-custom">custom option</link> > name, in which it > + creates a placeholder with the empty string as its old value. > + </para> > <para> > <literal>set_config('log_statement_stats', 'off', false)</literal> > <returnvalue>off</returnvalue> I was sold on this at first, but then I realized that the SET documentation doesn't mention placeholder variables either. I think it would be better to document them for SET, there's more room for discussion there than in this table. The set_config() documentation already mentions that it corresponds to the SET command. -- Heikki Linnakangas Neon (https://neon.tech)
Re: Improve documentation regarding custom settings, placeholders, and the administrative functions
From
"David G. Johnston"
Date:
On Fri, Apr 4, 2025 at 5:19 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 19/10/2024 23:11, David G. Johnston wrote:
> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index 934ef5e469..4478d0aa91 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -23,7 +23,7 @@
>
> <para>
> All parameter names are case-insensitive. Every parameter takes a
> - value of one of five types: boolean, string, integer, floating point,
> + non-null value of one of five types: boolean, string, integer,
> floating point,
> or enumerated (enum). The type determines the syntax for setting the
> parameter:
> </para>
Feels a bit obtuse to be honest. Who would've thought that they can be
NULL? They're not regular SQL datatypes, after all.
People aren't thinking about nullability at all when they read this, whether the differences between these and regular SQL data types even dawns on them. But my belief in adding that minor point is that at some point in the future when someone goes to write:
coalesce(current_setting('prefix.name', true), 'default-value')
they may have read, and stuck in the recesses of their mind, the "non-null value" bit and go "why am I using coalesce if the value cannot be null?".
> @@ -11350,14 +11350,20 @@ dynamic_library_path = 'C:\tools\postgresql;H:
> \my_project\lib;$libdir'
> <para>
> Because custom options may need to be set in processes that have not
> loaded the relevant extension module, <productname>PostgreSQL</
> productname>
> - will accept a setting for any two-part parameter name. Such variables
> - are treated as placeholders and have no function until the module that
> - defines them is loaded. When an extension module is loaded, it
> will add
> + will accept a setting for any two-part parameter name.
> + When an extension module is loaded, it will add
> its variable definitions and convert any placeholder values
> according to
> those definitions. If there are any unrecognized placeholders
> that begin with its extension name, warnings are issued and those
> placeholders are removed.
> </para>
-1, this completely removes the explanation of what a placeholder
variable is, but the term placeholder is still used in the text that
follows.
I accidentally removed the labelling of the set of settings meeting "two-part parameter name" as "placeholders". There isn't an explanation of what they are beyond that. There is an explanation of how they are treated, which I've left unmodified, aside from the counter-productive commentary about "have no function until the module that defines them is loaded" since they are enabling functionality every day by people without the use of extensions.
The fix is to just add back the label:
... will accept a setting for any two-part parameter name, called placeholders.
When an extension module is loaded, it will add ...
> +
> + <para>
> + If a placeholder is created in a session it will exist for the
> + lifetime of the session unless removed by an extension.
> + Placeholders have a string data type with a reset value of the
> empty string.
> + </para>
Placeholders can be created in postgresql.conf too, so this emphasis on
"created in a session" feels a bit off.
If someone claims the following is a bug:
login
current_setting('ctx.name'); // error
set_config('ctx.name', 'whatever');
reset
current_setting('ctx.name'); // empty string
// ^ bug - reset should make that an error again
I will point them right to this section and paragraph. Hopefully they just read it first; and can translate it to SQL. The pending "NULL Overview" documentation fills in that final gap in explanation.
People aren't sticking their custom context settings into postgresql.conf; or if they are, they aren't reporting the aforementioned bug.
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index ad663c94d7..605bf533ee 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -28157,7 +28157,7 @@ acl | {postgres=arwdDxtm/postgres,foo=r/
> postgres}
> <returnvalue>text</returnvalue>
> </para>
> <para>
> - Returns the current value of the
> + Returns the current non-null value of the
> setting <parameter>setting_name</parameter>. If there is no such
> setting, <function>current_setting</function> throws an error
> unless <parameter>missing_ok</parameter> is supplied and
Feels a bit cavalier to mention the "non-null" like this. I understand
that you wanted to make it clear that when current_setting() returns
NULL, it means that the setting did not exist and you used
missing_ok=true. But to deduce that, you still need to know that the
value cannot be NULL. Otherwise you're left wondering what the function
would return for NULL values, since this only describes what it returns
for non-null values.
I had this concern too, then added the obtuse "non-null" wording to config.sgml to address that in the definitional location.
To keep it closer to the problematic area, we can say:
Returns the current value (which is unable to be the null value) of the
I like this better but would still leave the non-null modifier in config.sgml as part of the definition of a setting.
> @@ -28191,6 +28191,17 @@ acl | {postgres=arwdDxtm/postgres,foo=r/
> postgres}
> use <literal>false</literal> instead. This function corresponds to
> the SQL command <xref linkend="sql-set"/>.
> </para>
> + <para>
> + <function>set_config</function> accepts the NULL value for
> + <parameter>new_value</parameter>, but as settings cannot be
> null this input
> + is interpreted as a request to set the setting to its default
> value.
> + </para>
+1 on this part. Committed this paragraph so that we can make some
incremental progress.
Thank you.
> + <para>
> + If <parameter>setting_name</parameter> does not already exist
> + <function>set_config</function> throws an error unless the
> identifier is a valid
> + <link linkend="runtime-config-custom">custom option</link>
> name, in which it
> + creates a placeholder with the empty string as its old value.
> + </para>
> <para>
> <literal>set_config('log_statement_stats', 'off', false)</literal>
> <returnvalue>off</returnvalue>
I was sold on this at first, but then I realized that the SET
documentation doesn't mention placeholder variables either. I think it
would be better to document them for SET, there's more room for
discussion there than in this table. The set_config() documentation
already mentions that it corresponds to the SET command.
Good catch. I will do that.
My next patch is going to include my take on the disputed items above unless you change your mind and commit them before I get around to it. As shown, you or someone else can just omit them again if my reasoning isn't sufficiently convincing.
David J.