Thread: doc review for v13

I reviewed docs for v13, like:
git log --cherry-pick origin/master...origin/REL_12_STABLE -p doc

I did something similar for v12 [0].  I've included portions of that here which
still seem lacking 12 months later (but I'm not intending to continue defending
each individual patch hunk).

I previously mailed separately about a few individual patches, some of which
have separate, ongoing discussion and aren't included here (incr sort, parallel
vacuum).

Justin

[0] https://www.postgresql.org/message-id/flat/20190709161256.GH22387%40telsasoft.com#56889b868e5886e36b90e9f5a1165186

On Wed, Apr 08, 2020 at 11:56:53AM -0500, Justin Pryzby wrote:
> I previously mailed separately about a few individual patches, some of which
> have separate, ongoing discussion and aren't included here (incr sort, parallel
> vacuum).

I have gone through your changes, and committed what looked like the
most obvious mistakes in my view.  Please see below for more
comments.

 required pages to remove both downlink and rightlink are already  locked.  That
-evades potential right to left page locking order, which could deadlock with
+avoid potential right to left page locking order, which could deadlock with
Not sure which one is better, but the new change is grammatically
incorrect.

       <varname>auto_explain.log_settings</varname> controls whether information
-      about modified configuration options are printed when execution plan is logged.
-      Only options affecting query planning with value different from the built-in
+      about modified configuration options is printed when an execution plan is logged.
+      Only those options which affect query planning and whose value differs from its built-in
Depends on how you read the sentence, but here is seemt to me that
"statistics" is the correct subject, no?

-        replication is disabled.  Abrupt streaming client disconnection might
+        replication is disabled.  Abrupt disconnection of a streaming client might
Original looks correct to me here.

     <literal>_tz</literal> suffix. These functions have been implemented to
-    support comparison of date/time values that involves implicit
+    support comparison of date/time values that involve implicit
The subject is "comparison" here, no?

     may be included. It also stores the size, last modification time, and
-    an optional checksum for each file.
+    optionally a checksum for each file.
The original sounds fine to me as well.

Anything related to imath.c needs to be reported upstream, though I
recall reporting these two already.
--
Michael

On Fri, Apr 10, 2020 at 11:27:46AM +0900, Michael Paquier wrote:
> On Wed, Apr 08, 2020 at 11:56:53AM -0500, Justin Pryzby wrote:
> > I previously mailed separately about a few individual patches, some of which
> > have separate, ongoing discussion and aren't included here (incr sort, parallel
> > vacuum).
> 
> I have gone through your changes, and committed what looked like the
> most obvious mistakes in my view.  Please see below for more
> comments.

Thanks - rebased for cfbot and continued review.

>  required pages to remove both downlink and rightlink are already  locked.  That
> -evades potential right to left page locking order, which could deadlock with
> +avoid potential right to left page locking order, which could deadlock with
> Not sure which one is better, but the new change is grammatically
> incorrect.

Thanks for noticing

"Evades" usually means to act to avoid detection by the government.  Like tax
evasion.

>        <varname>auto_explain.log_settings</varname> controls whether information
> -      about modified configuration options are printed when execution plan is logged.
> -      Only options affecting query planning with value different from the built-in
> +      about modified configuration options is printed when an execution plan is logged.
> +      Only those options which affect query planning and whose value differs from its built-in
> Depends on how you read the sentence, but here is seemt to me that 
> "statistics" is the correct subject, no?

Statistics ?

>      <literal>_tz</literal> suffix. These functions have been implemented to
> -    support comparison of date/time values that involves implicit
> +    support comparison of date/time values that involve implicit
> The subject is "comparison" here, no?

You're right.

-- 
Justin

On Thu, Apr 09, 2020 at 10:01:51PM -0500, Justin Pryzby wrote:
> On Fri, Apr 10, 2020 at 11:27:46AM +0900, Michael Paquier wrote:
>>  required pages to remove both downlink and rightlink are already  locked.  That
>> -evades potential right to left page locking order, which could deadlock with
>> +avoid potential right to left page locking order, which could deadlock with
>> Not sure which one is better, but the new change is grammatically
>> incorrect.
>
> "Evades" usually means to act to avoid detection by the government.  Like tax
> evasion.

This change is from Alexander Korotkov as of 32ca32d, so I am adding
him in CC to get his opinion.

>>        <varname>auto_explain.log_settings</varname> controls whether information
>> -      about modified configuration options are printed when execution plan is logged.
>> -      Only options affecting query planning with value different from the built-in
>> +      about modified configuration options is printed when an execution plan is logged.
>> +      Only those options which affect query planning and whose value differs from its built-in
>> Depends on how you read the sentence, but here is seemt to me that
>> "statistics" is the correct subject, no?
>
> Statistics ?

Oops.  I may have messed up with a different part of the patch set.
Your suggestion is right as the subject is "information" here.
--
Michael

Added a few more.
And rebased on top of dbc60c5593f26dc777a3be032bff4fb4eab1ddd1

-- 
Justin

On Sun, Apr 12, 2020 at 04:35:45PM -0500, Justin Pryzby wrote:
> Added a few more.
> And rebased on top of dbc60c5593f26dc777a3be032bff4fb4eab1ddd1

Thanks for the patch set, I have applied the most obvious parts (more
or less 1/3) to reduce the load.  Here is a review of the rest.

> @@ -2829,7 +2829,6 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
>
>          ExplainPropertyList("Sort Methods Used", methodNames, es);
>
> -        if (groupInfo->maxMemorySpaceUsed > 0)
>          {
>              long        avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
>              const char *spaceTypeName;
> @@ -2846,7 +2845,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
>
>              ExplainCloseGroup("Sort Spaces", memoryName.data, true, es);
>          }
> -        if (groupInfo->maxDiskSpaceUsed > 0)
> +
>          {
>              long        avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
>              const char *spaceTypeName;

If this can be reworked, it seems to me that more cleanup could be
done.

> @@ -987,7 +987,7 @@ ExecInitIncrementalSort(IncrementalSort *node, EState *estate, int eflags)
>
>      /*
>       * Incremental sort can't be used with either EXEC_FLAG_REWIND,
> -     * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only one of many sort
> +     * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only ???? one of many sort
>       * batches in the current sort state.
>       */
>      Assert((eflags & (EXEC_FLAG_BACKWARD |

The following is inconsistent with this comment block, and I guess
that "????" should be "have":
    Assert((eflags & (EXEC_FLAG_BACKWARD |
                          EXEC_FLAG_MARK)) == 0);
This is only a doc-related change though, so I'll start a different
thread about that after looking more at it.

> @@ -1153,7 +1153,7 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
>      /*
>       * If we've set up either of the sort states yet, we need to reset them.
>       * We could end them and null out the pointers, but there's no reason to
> -     * repay the setup cost, and because guard setting up pivot comparator
> +     * repay the setup cost, and because ???? guard setting up pivot comparator
>       * state similarly, doing so might actually cause a leak.
>       */
>      if (node->fullsort_state != NULL)

I don't quite understand this comment either, but it seems to me that
the last part should be a fully-separate sentence, aka "This guards
against..".

> @@ -631,7 +631,7 @@ logicalrep_partition_open(LogicalRepRelMapEntry *root,
>      /*
>       * If the partition's attributes don't match the root relation's, we'll
>       * need to make a new attrmap which maps partition attribute numbers to
> -     * remoterel's, instead the original which maps root relation's attribute
> +     * remoterel's, instead of the original which maps root relation's attribute
>       * numbers to remoterel's.
>       *
>       * Note that 'map' which comes from the tuple routing data structure

Okay, this is not really clear to start with.  I think that I would
rewrite that completely as follows:
"If the partition's attributes do not match the root relation's
attributes, we cannot use the original attribute map which maps the
root relation's attributes with remoterel's attributes.  Instead,
build a new attribute map which maps the partition's attributes with
remoterel's attributes."

> +++ b/src/backend/storage/lmgr/proc.c
> @@ -1373,7 +1373,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
>              else
>                  LWLockRelease(ProcArrayLock);
>
> -            /* prevent signal from being resent more than once */
> +            /* prevent signal from being re-sent more than once */
>              allow_autovacuum_cancel = false;
>          }

Shouldn't that just be "sent more than two times"?


> @@ -1428,11 +1428,11 @@ tuplesort_updatemax(Tuplesortstate *state)
>      }
>
>      /*
> -     * Sort evicts data to the disk when it didn't manage to fit those data to
> -     * the main memory.  This is why we assume space used on the disk to be
> +     * Sort evicts data to the disk when it didn't manage to fit the data in
> +     * main memory.  This is why we assume space used on the disk to be

Both the original and the suggestion are wrong?  It seems to me that
it should be "this data" instead because it refers to the data evicted
in the first part of the sentence.

>       * more important for tracking resource usage than space used in memory.
> -     * Note that amount of space occupied by some tuple set on the disk might
> -     * be less than amount of space occupied by the same tuple set in the
> +     * Note that amount of space occupied by some tupleset on the disk might
> +     * be less than amount of space occupied by the same tupleset in the
>       * memory due to more compact representation.
>       */
>      if ((isSpaceDisk && !state->isMaxSpaceDisk) ||

Yep, right.

> +++ b/doc/src/sgml/logicaldecoding.sgml
> @@ -223,7 +223,7 @@ $ pg_recvlogical -d postgres --slot=test --drop-slot
>       A logical slot will emit each change just once in normal operation.
>       The current position of each slot is persisted only at checkpoint, so in
>       the case of a crash the slot may return to an earlier LSN, which will
> -     then cause recent changes to be resent when the server restarts.
> +     then cause recent changes to be re-sent when the server restarts.
>       Logical decoding clients are responsible for avoiding ill effects from
>       handling the same message more than once.  Clients may wish to record
>       the last LSN they saw when decoding and skip over any repeated data or

"sent again" instead of "resent" or "re-sent"?

> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -889,7 +889,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
>        from the parent table will be created in the partition, if they don't
>        already exist.
>        If any of the <literal>CHECK</literal> constraints of the table being
> -      attached is marked <literal>NO INHERIT</literal>, the command will fail;
> +      attached are marked <literal>NO INHERIT</literal>, the command will fail;
>        such constraints must be recreated without the
>        <literal>NO INHERIT</literal> clause.
>       </para>

It seems to me that both are actually correct here.

> +++ b/doc/src/sgml/ref/pg_basebackup.sgml
> @@ -604,7 +604,7 @@ PostgreSQL documentation
>          not contain any checksums. Otherwise, it will contain a checksum
>          of each file in the backup using the specified algorithm. In addition,
>          the manifest will always contain a <literal>SHA256</literal>
> -        checksum of its own contents. The <literal>SHA</literal> algorithms
> +        checksum of its own content. The <literal>SHA</literal> algorithms
>          are significantly more CPU-intensive than <literal>CRC32C</literal>,
>          so selecting one of them may increase the time required to complete
>          the backup.

And the original is correct here IMO.

> +++ b/doc/src/sgml/ref/psql-ref.sgml
> @@ -1244,7 +1244,7 @@ testdb=>
>          (see <xref linkend="catalog-pg-opclass"/>).
>          If <replaceable class="parameter">access-method-patttern</replaceable>
>          is specified, only operator classes associated with access methods whose
> -        names match pattern are listed.
> +        names match the pattern are listed.
>          If <replaceable class="parameter">input-type-pattern</replaceable>
>          is specified, only operator classes associated with input types whose
>          names match the pattern are listed.

Another error I see here is that pattern has three 't', while the
original parameter name is correct.

> +++ b/doc/src/sgml/runtime.sgml
> @@ -2643,7 +2643,7 @@ openssl x509 -req -in server.csr -text -days 365 \
>     <para>
>      The <productname>PostgreSQL</productname> server will listen for both
>      normal and <acronym>GSSAPI</acronym>-encrypted connections on the same TCP
> -    port, and will negotiate with any connecting client on whether to
> +    port, and will negotiate with any connecting client whether to
>      use <acronym>GSSAPI</acronym> for encryption (and for authentication).  By
>      default, this decision is up to the client (which means it can be
>      downgraded by an attacker); see <xref linkend="auth-pg-hba-conf"/> about

Actually correct?
--
Michael

On Tue, Apr 14, 2020 at 02:47:54PM +0900, Michael Paquier wrote:
> On Sun, Apr 12, 2020 at 04:35:45PM -0500, Justin Pryzby wrote:
> > Added a few more.
> > And rebased on top of dbc60c5593f26dc777a3be032bff4fb4eab1ddd1
> 
> Thanks for the patch set, I have applied the most obvious parts (more
> or less 1/3) to reduce the load.  Here is a review of the rest.

Thanks - attached are the remaining undisputed portions..

> > +++ b/doc/src/sgml/ref/alter_table.sgml
> > @@ -889,7 +889,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
> >        from the parent table will be created in the partition, if they don't
> >        already exist.
> >        If any of the <literal>CHECK</literal> constraints of the table being
> > -      attached is marked <literal>NO INHERIT</literal>, the command will fail;
> > +      attached are marked <literal>NO INHERIT</literal>, the command will fail;
> >        such constraints must be recreated without the
> >        <literal>NO INHERIT</literal> clause.
> >       </para>
>
> It seems to me that both are actually correct here.

I think my text is correct.  This would *also* be correct:

|       If any <literal>CHECK</literal> constraint on the table being
|       attached is marked <literal>NO INHERIT</literal>, the command will fail;

But not the hybrid: "If any OF THE .. is .."

-- 
Justin

On Sun, Apr 26, 2020 at 12:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Tue, Apr 14, 2020 at 02:47:54PM +0900, Michael Paquier wrote:
> > On Sun, Apr 12, 2020 at 04:35:45PM -0500, Justin Pryzby wrote:
> > > Added a few more.
> > > And rebased on top of dbc60c5593f26dc777a3be032bff4fb4eab1ddd1
> >
> > Thanks for the patch set, I have applied the most obvious parts (more
> > or less 1/3) to reduce the load.  Here is a review of the rest.
>
> Thanks - attached are the remaining undisputed portions..
>
> > > +++ b/doc/src/sgml/ref/alter_table.sgml
> > > @@ -889,7 +889,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
> > >        from the parent table will be created in the partition, if they don't
> > >        already exist.
> > >        If any of the <literal>CHECK</literal> constraints of the table being
> > > -      attached is marked <literal>NO INHERIT</literal>, the command will fail;
> > > +      attached are marked <literal>NO INHERIT</literal>, the command will fail;
> > >        such constraints must be recreated without the
> > >        <literal>NO INHERIT</literal> clause.
> > >       </para>
> >
> > It seems to me that both are actually correct here.
>
> I think my text is correct.  This would *also* be correct:
>
> |       If any <literal>CHECK</literal> constraint on the table being
> |       attached is marked <literal>NO INHERIT</literal>, the command will fail;
>
> But not the hybrid: "If any OF THE .. is .."

"any of the...are" sounds more natural to my ears, and some searching
yielded some grammar sites that agree (specifically that "any of" is
only used with singular verbs if the construction is uncountable or
negative).

However there are also multiple claims by grammarians that either
singular or plural verbs are acceptable with the "any of"
construction. So that's not all that helpful.

James

James Coleman <jtc331@gmail.com> writes:
> On Sun, Apr 26, 2020 at 12:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> I think my text is correct.  This would *also* be correct:
>> |       If any <literal>CHECK</literal> constraint on the table being
>> |       attached is marked <literal>NO INHERIT</literal>, the command will fail;
>> But not the hybrid: "If any OF THE .. is .."

> "any of the...are" sounds more natural to my ears,

Yeah, I think the same.  If you want to argue grammar, I'd point
out that the "any" could refer to several of the constraints,
making it correct to use the plural verb.  The alternative that
Justin mentions could be written as "If any one constraint is ...",
which is correct in that form; but the plural way seems less stilted.

            regards, tom lane

On Sun, Apr 26, 2020 at 08:59:05PM -0400, Tom Lane wrote:
> James Coleman <jtc331@gmail.com> writes:
>> On Sun, Apr 26, 2020 at 12:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>>> I think my text is correct.  This would *also* be correct:
>>> |       If any <literal>CHECK</literal> constraint on the table being
>>> |       attached is marked <literal>NO INHERIT</literal>, the command will fail;
>>> But not the hybrid: "If any OF THE .. is .."
>
>> "any of the...are" sounds more natural to my ears,
>
> Yeah, I think the same.  If you want to argue grammar, I'd point
> out that the "any" could refer to several of the constraints,
> making it correct to use the plural verb.  The alternative that
> Justin mentions could be written as "If any one constraint is ...",
> which is correct in that form; but the plural way seems less stilted.

Hm, okay.  There are still pieces in those patches about which I am
not sure, so I have let that aside for the time being.

Anyway, I have applied patch 12, and reported the typos from imath.c
directly to upstream:
https://github.com/creachadair/imath/issues/45
https://github.com/creachadair/imath/issues/46
--
Michael

On Mon, Apr 27, 2020 at 03:03:05PM +0900, Michael Paquier wrote:
> Hm, okay.  There are still pieces in those patches about which I am
> not sure, so I have let that aside for the time being.
> 
> Anyway, I have applied patch 12, and reported the typos from imath.c

Thank you.

I will leave this here in case someone else wants to make a pass or vet them.

diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml
index 192d6574c3..de2be61bff 100644
--- a/doc/src/sgml/auto-explain.sgml
+++ b/doc/src/sgml/auto-explain.sgml
@@ -200,9 +200,9 @@ LOAD 'auto_explain';
     <listitem>
      <para>
       <varname>auto_explain.log_settings</varname> controls whether information
-      about modified configuration options is printed when execution plan is logged.
-      Only options affecting query planning with value different from the built-in
-      default value are included in the output.  This parameter is off by default.
+      about modified configuration options is printed when an execution plan is logged.
+      Only those options which affect query planning and whose value differs from their
+      built-in default are included in the output.  This parameter is off by default.
       Only superusers can change this setting.
      </para>
     </listitem>
diff --git a/doc/src/sgml/btree.sgml b/doc/src/sgml/btree.sgml
index e9cab4a55d..ff1e49e509 100644
--- a/doc/src/sgml/btree.sgml
+++ b/doc/src/sgml/btree.sgml
@@ -609,7 +609,7 @@ equalimage(<replaceable>opcintype</replaceable> <type>oid</type>) returns bool
   </para>
   <para>
    Deduplication works by periodically merging groups of duplicate
-   tuples together, forming a single posting list tuple for each
+   tuples together, forming a single <firstterm>posting list</firstterm> tuple for each
    group.  The column key value(s) only appear once in this
    representation.  This is followed by a sorted array of
    <acronym>TID</acronym>s that point to rows in the table.  This
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a14df06292..87e0183a89 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3667,7 +3667,7 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
         servers or streaming base backup clients (i.e., the maximum number of
         simultaneously running WAL sender processes). The default is
         <literal>10</literal>.  The value <literal>0</literal> means
-        replication is disabled.  Abrupt streaming client disconnection might
+        replication is disabled.  Abrupt disconnection of a streaming client might
         leave an orphaned connection slot behind until a timeout is reached,
         so this parameter should be set slightly higher than the maximum
         number of expected clients so disconnected clients can immediately
@@ -3790,9 +3790,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
         slots</link> are allowed to retain in the <filename>pg_wal</filename>
         directory at checkpoint time.
         If <varname>max_slot_wal_keep_size</varname> is -1 (the default),
-        replication slots retain unlimited amount of WAL files.  If
-        restart_lsn of a replication slot gets behind more than that megabytes
-        from the current LSN, the standby using the slot may no longer be able
+        replication slots retain unlimited amount of WAL files.  Otherwise, if
+        restart_lsn of a replication slot falls behind the current LSN by more
+        than the specified size, the standby using the slot may no longer be able
         to continue replication due to removal of required WAL files. You
         can see the WAL availability of replication slots
         in <link linkend="view-pg-replication-slots">pg_replication_slots</link>.
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 2a478c4f73..9ec0d4c783 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3959,7 +3959,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
      Before running the <command>ATTACH PARTITION</command> command, it is
      recommended to create a <literal>CHECK</literal> constraint on the table to
      be attached matching the desired partition constraint. That way,
-     the system will be able to skip the scan to validate the implicit
+     the system will be able to skip the scan which is otherwise needed to validate the implicit
      partition constraint. Without the <literal>CHECK</literal> constraint,
      the table will be scanned to validate the partition constraint while
      holding an <literal>ACCESS EXCLUSIVE</literal> lock on that partition
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 75d2224a61..b9c789eb6f 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -925,11 +925,11 @@ postgresql:///mydb?host=localhost&port=5433
    </para>
 
    <para>
-    Connection <acronym>URI</acronym> needs to be encoded with 
+    A connection <acronym>URI</acronym> needs to be encoded with 
     <ulink url="https://tools.ietf.org/html/rfc3986#section-2.1">Percent-encoding</ulink> 
-    if it includes symbols with special meaning in any of its parts. 
-    Here is an example where equal sign (<literal>=</literal>) is replaced
-    with <literal>%3D</literal> and whitespace character with
+    if it includes symbols with special meanings in any of its parts. 
+    Here is an example where an equal sign (<literal>=</literal>) is replaced
+    with <literal>%3D</literal> and a whitespace character with
     <literal>%20</literal>:
 <programlisting>
 postgresql://user@localhost:5433/mydb?options=-c%20synchronous_commit%3Doff
@@ -1223,7 +1223,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
       <term><literal>connect_timeout</literal></term>
       <listitem>
       <para>
-       Maximum wait for connection, in seconds (write as a decimal integer,
+       Maximum time to wait while connecting, in seconds (write as a decimal integer,
        e.g. <literal>10</literal>).  Zero, negative, or not specified means
        wait indefinitely.  The minimum allowed timeout is 2 seconds, therefore
        a value of <literal>1</literal> is interpreted as <literal>2</literal>.
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index eba331a72b..faf6d56bed 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -404,7 +404,7 @@
     <para>
      Replication is only supported by tables, including partitioned tables.
      Attempts to replicate other types of relations such as views, materialized
-     views, or foreign tables, will result in an error.
+     views, or foreign tables will result in an error.
     </para>
    </listitem>
 
diff --git a/doc/src/sgml/logicaldecoding.sgml b/doc/src/sgml/logicaldecoding.sgml
index bad3bfe620..e08ae9e2af 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -223,7 +223,7 @@ $ pg_recvlogical -d postgres --slot=test --drop-slot
      A logical slot will emit each change just once in normal operation.
      The current position of each slot is persisted only at checkpoint, so in
      the case of a crash the slot may return to an earlier LSN, which will
-     then cause recent changes to be resent when the server restarts.
+     then cause recent changes to be re-sent when the server restarts.
      Logical decoding clients are responsible for avoiding ill effects from
      handling the same message more than once.  Clients may wish to record
      the last LSN they saw when decoding and skip over any repeated data or
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 6562cc400b..3cabc24721 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1484,11 +1484,11 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
         </row>
         <row>
          <entry><literal>RecoveryConflictSnapshot</literal></entry>
-         <entry>Waiting for recovery conflict resolution on a vacuum cleanup.</entry>
+         <entry>Waiting for recovery conflict resolution during vacuum cleanup.</entry>
         </row>
         <row>
          <entry><literal>RecoveryConflictTablespace</literal></entry>
-         <entry>Waiting for recovery conflict resolution on dropping tablespace.</entry>
+         <entry>Waiting for recovery conflict resolution while dropping tablespace.</entry>
         </row>
         <row>
          <entry><literal>RecoveryPause</literal></entry>
@@ -1526,9 +1526,9 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
         <row>
          <entry><literal>RecoveryRetrieveRetryInterval</literal></entry>
          <entry>
-          Waiting when WAL data is not available from any kind of sources
-          (<filename>pg_wal</filename>, archive or stream) before trying
-          again to retrieve WAL data, at recovery.
+          Waiting in recovery when WAL data is not available from any source
+          (<filename>pg_wal</filename>, archive or stream) before re-trying
+          to retrieve WAL data.
          </entry>
         </row>
         <row>
@@ -4577,8 +4577,8 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
       <entry><literal>waiting for checkpoint to finish</literal></entry>
       <entry>
        The WAL sender process is currently performing
-       <function>pg_start_backup</function> to set up for
-       taking a base backup, and waiting for backup start
+       <function>pg_start_backup</function> to prepare to
+       take a base backup, and waiting for the start-of-backup
        checkpoint to finish.
       </entry>
      </row>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 20d1fe0ad8..06919bd87c 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2586,7 +2586,7 @@ The commands accepted in replication mode are:
           and sent along with the backup.  The manifest is a list of every
           file present in the backup with the exception of any WAL files that
           may be included. It also stores the size, last modification time, and
-          an optional checksum for each file.
+          optionally a checksum for each file.
           A value of <literal>force-encode</literal> forces all filenames
           to be hex-encoded; otherwise, this type of encoding is performed only
           for files whose names are non-UTF8 octet sequences.
@@ -2602,7 +2602,7 @@ The commands accepted in replication mode are:
         <term><literal>MANIFEST_CHECKSUMS</literal> <replaceable>checksum_algorithm</replaceable></term>
         <listitem>
          <para>
-          Specifies the algorithm that should be applied to each file included
+          Specifies the checksum algorithm that should be applied to each file included
           in the backup manifest. Currently, the available
           algorithms are <literal>NONE</literal>, <literal>CRC32C</literal>,
           <literal>SHA224</literal>, <literal>SHA256</literal>,
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 6563bd5ab2..39e9f9a7c7 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -671,7 +671,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
       When applied to a partitioned table, nothing is moved, but any
       partitions created afterwards with
       <command>CREATE TABLE PARTITION OF</command> will use that tablespace,
-      unless the <literal>TABLESPACE</literal> clause is used to override it.
+      unless overridden by its <literal>TABLESPACE</literal> clause.
      </para>
 
      <para>
@@ -891,7 +891,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
       from the parent table will be created in the partition, if they don't
       already exist.
       If any of the <literal>CHECK</literal> constraints of the table being
-      attached is marked <literal>NO INHERIT</literal>, the command will fail;
+      attached are marked <literal>NO INHERIT</literal>, the command will fail;
       such constraints must be recreated without the
       <literal>NO INHERIT</literal> clause.
      </para>
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 1a90c244fb..c18618d5c8 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -159,7 +159,7 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
          <para>
           It is safe to use <literal>off</literal> for logical replication:
           If the subscriber loses transactions because of missing
-          synchronization, the data will be resent from the publisher.
+          synchronization, the data will be re-sent from the publisher.
          </para>
 
          <para>
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 01ce44ee22..b742470f13 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -604,7 +604,7 @@ PostgreSQL documentation
         not contain any checksums. Otherwise, it will contain a checksum
         of each file in the backup using the specified algorithm. In addition,
         the manifest will always contain a <literal>SHA256</literal>
-        checksum of its own contents. The <literal>SHA</literal> algorithms
+        checksum of its own content. The <literal>SHA</literal> algorithms
         are significantly more CPU-intensive than <literal>CRC32C</literal>,
         so selecting one of them may increase the time required to complete
         the backup.
@@ -614,7 +614,7 @@ PostgreSQL documentation
         of each file for users who wish to verify that the backup has not been
         tampered with, while the CRC32C algorithm provides a checksum which is
         much faster to calculate and good at catching errors due to accidental
-        changes but is not resistant to targeted modifications.  Note that, to
+        changes but is not resistant to malicious modifications.  Note that, to
         be useful against an adversary who has access to the backup, the backup
         manifest would need to be stored securely elsewhere or otherwise
         verified not to have been modified since the backup was taken.
@@ -808,7 +808,7 @@ PostgreSQL documentation
   </para>
 
   <para>
-   Tablespaces will in plain format by default be backed up to the same path
+   In plain format, tablespaces will by default be backed up to the same path
    they have on the server, unless the
    option <literal>--tablespace-mapping</literal> is used.  Without
    this option, running a plain format base backup on the same host as the
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index a9bc397165..d58cd05f46 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -323,7 +323,7 @@ PostgreSQL documentation
       <listitem>
        <para>
         Run the dump in parallel by dumping <replaceable class="parameter">njobs</replaceable>
-        tables simultaneously. This option reduces the time of the dump but it also
+        tables simultaneously. This option reduces the duration of the dump but it also
         increases the load on the database server. You can only use this option with the
         directory output format because this is the only output format where multiple processes
         can write their data at the same time.
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 07c49e4719..acdefe58b8 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -215,7 +215,7 @@ PostgreSQL documentation
         <command>pg_rewind</command> to return without waiting, which is
         faster, but means that a subsequent operating system crash can leave
         the synchronized data directory corrupt.  Generally, this option is
-        useful for testing but should not be used when creating a production
+        useful for testing but should not be used on a production
         installation.
        </para>
       </listitem>
@@ -309,7 +309,7 @@ GRANT EXECUTE ON function pg_catalog.pg_read_binary_file(text, bigint, bigint, b
   <para>
    When executing <application>pg_rewind</application> using an online
    cluster as source which has been recently promoted, it is necessary
-   to execute a <command>CHECKPOINT</command> after promotion so as its
+   to execute a <command>CHECKPOINT</command> after promotion such that its
    control file reflects up-to-date timeline information, which is used by
    <application>pg_rewind</application> to check if the target cluster
    can be rewound using the designated source cluster.
diff --git a/doc/src/sgml/ref/pg_verifybackup.sgml b/doc/src/sgml/ref/pg_verifybackup.sgml
index 4f9759414f..9618275364 100644
--- a/doc/src/sgml/ref/pg_verifybackup.sgml
+++ b/doc/src/sgml/ref/pg_verifybackup.sgml
@@ -46,7 +46,7 @@ PostgreSQL documentation
    every check which will be performed by a running server when attempting
    to make use of the backup. Even if you use this tool, you should still
    perform test restores and verify that the resulting databases work as
-   expected and that they appear to contain the correct data. However,
+   expected and that they contain the correct data. However,
    <application>pg_verifybackup</application> can detect many problems
    that commonly occur due to storage problems or user error.
   </para>
@@ -84,7 +84,7 @@ PostgreSQL documentation
    for any files for which the computed checksum does not match the
    checksum stored in the manifest. This step is not performed for any files
    which produced errors in the previous step, since they are already known
-   to have problems. Also, files which were ignored in the previous step are
+   to have problems. Files which were ignored in the previous step are
    also ignored in this step.
   </para>
 
@@ -123,7 +123,7 @@ PostgreSQL documentation
   <title>Options</title>
 
    <para>
-    The following command-line options control the behavior.
+    The following command-line options control the behavior of this program.
 
     <variablelist>
      <varlistentry>
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c54a7c420d..bde5eca164 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -249,7 +249,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
   <para>
    Reindexing a single index or table requires being the owner of that
    index or table.  Reindexing a schema or database requires being the
-   owner of that schema or database.  Note that is therefore sometimes
+   owner of that schema or database.  Note specifically that it's
    possible for non-superusers to rebuild indexes of tables owned by
    other users.  However, as a special exception, when
    <command>REINDEX DATABASE</command>, <command>REINDEX SCHEMA</command>
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index f6c3d9538b..4388d1329c 100644
--- a/doc/src/sgml/ref/reindexdb.sgml
+++ b/doc/src/sgml/ref/reindexdb.sgml
@@ -173,8 +173,8 @@ PostgreSQL documentation
        <para>
         Execute the reindex commands in parallel by running
         <replaceable class="parameter">njobs</replaceable>
-        commands simultaneously.  This option reduces the time of the
-        processing but it also increases the load on the database server.
+        commands simultaneously.  This option reduces the processing time
+        but it also increases the load on the database server.
        </para>
        <para>
         <application>reindexdb</application> will open
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index fd1dc140ab..93a3eed813 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -155,8 +155,8 @@ PostgreSQL documentation
        <para>
         Execute the vacuum or analyze commands in parallel by running
         <replaceable class="parameter">njobs</replaceable>
-        commands simultaneously.  This option reduces the time of the
-        processing but it also increases the load on the database server.
+        commands simultaneously.  This option reduces the processing
+        duration but it also increases the load on the database server.
        </para>
        <para>
         <application>vacuumdb</application> will open
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index a34d31d297..3f90c15f3e 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2643,7 +2643,7 @@ openssl x509 -req -in server.csr -text -days 365 \
    <para>
     The <productname>PostgreSQL</productname> server will listen for both
     normal and <acronym>GSSAPI</acronym>-encrypted connections on the same TCP
-    port, and will negotiate with any connecting client on whether to
+    port, and will negotiate with any connecting client whether to
     use <acronym>GSSAPI</acronym> for encryption (and for authentication).  By
     default, this decision is up to the client (which means it can be
     downgraded by an attacker); see <xref linkend="auth-pg-hba-conf"/> about
diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index 283c3e0357..5a8dbcb4d3 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -373,7 +373,7 @@ ereport(ERROR,
      specify suppression of the <literal>CONTEXT:</literal> portion of a message in
      the postmaster log.  This should only be used for verbose debugging
      messages where the repeated inclusion of context would bloat the log
-     volume too much.
+     too much.
     </para>
    </listitem>
   </itemizedlist>
@@ -466,8 +466,8 @@ Hint:       the addendum
     enough for error messages.  Detail and hint messages can be relegated to a
     verbose mode, or perhaps a pop-up error-details window.  Also, details and
     hints would normally be suppressed from the server log to save
-    space. Reference to implementation details is best avoided since users
-    aren't expected to know the details.
+    space. References to implementation details are best avoided since users
+    aren't expected to know them.
    </para>
 
   </simplesect>
@@ -518,7 +518,7 @@ Hint:       the addendum
    <title>Use of Quotes</title>
 
    <para>
-    Use quotes always to delimit file names, user-supplied identifiers, and
+    Always use quotes to delimit file names, user-supplied identifiers, and
     other variables that might contain words.  Do not use them to mark up
     variables that will not contain words (for example, operator names).
    </para>
diff --git a/src/backend/access/gin/README b/src/backend/access/gin/README
index 125a82219b..41d4e1e8a0 100644
--- a/src/backend/access/gin/README
+++ b/src/backend/access/gin/README
@@ -413,7 +413,7 @@ leftmost leaf of the tree.
 Deletion algorithm keeps exclusive locks on left siblings of pages comprising
 currently investigated path.  Thus, if current page is to be removed, all
 required pages to remove both downlink and rightlink are already locked.  That
-evades potential right to left page locking order, which could deadlock with
+avoids potential right to left page locking order, which could deadlock with
 concurrent stepping right.
 
 A search concurrent to page deletion might already have read a pointer to the
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 7ae6131676..7cdf4b84e2 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2869,7 +2869,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
 }
 
 /*
- * If it's EXPLAIN ANALYZE, show tuplesort stats for a incremental sort node
+ * If it's EXPLAIN ANALYZE, show tuplesort stats for an incremental sort node
  */
 static void
 show_incremental_sort_info(IncrementalSortState *incrsortstate,
@@ -2917,7 +2917,7 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
             &incrsortstate->shared_info->sinfo[n];
 
             /*
-             * If a worker hasn't process any sort groups at all, then exclude
+             * If a worker hasn't processed any sort groups at all, then exclude
              * it from output since it either didn't launch or didn't
              * contribute anything meaningful.
              */
diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c
index 39ba11cdf7..da99453c91 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -987,7 +987,7 @@ ExecInitIncrementalSort(IncrementalSort *node, EState *estate, int eflags)
 
     /*
      * Incremental sort can't be used with either EXEC_FLAG_REWIND,
-     * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only one of many sort
+     * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only ???? one of many sort
      * batches in the current sort state.
      */
     Assert((eflags & (EXEC_FLAG_BACKWARD |
@@ -1153,8 +1153,10 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
     /*
      * If we've set up either of the sort states yet, we need to reset them.
      * We could end them and null out the pointers, but there's no reason to
-     * repay the setup cost, and because guard setting up pivot comparator
-     * state similarly, doing so might actually cause a leak.
+     * repay the setup cost, and because ExecIncrementalSort guards
+     * presorted column functions by checking to see if the full sort state
+     * has been initialized yet, setting the sort states to null here might
+     * actually cause a leak.
      */
     if (node->fullsort_state != NULL)
     {
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index fec39354c0..351b0950c0 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -631,7 +631,7 @@ logicalrep_partition_open(LogicalRepRelMapEntry *root,
     /*
      * If the partition's attributes don't match the root relation's, we'll
      * need to make a new attrmap which maps partition attribute numbers to
-     * remoterel's, instead the original which maps root relation's attribute
+     * remoterel's, instead of the original which maps root relation's attribute
      * numbers to remoterel's.
      *
      * Note that 'map' which comes from the tuple routing data structure
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 5aa19d3f78..22fe566f48 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1373,7 +1373,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
             else
                 LWLockRelease(ProcArrayLock);
 
-            /* prevent signal from being resent more than once */
+            /* prevent signal from being re-sent more than once */
             allow_autovacuum_cancel = false;
         }
 
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index bc063061cf..0faa66551f 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -35,7 +35,7 @@
  * executeItemOptUnwrapTarget() function have 'unwrap' argument, which indicates
  * whether unwrapping of array is needed.  When unwrap == true, each of array
  * members is passed to executeItemOptUnwrapTarget() again but with unwrap == false
- * in order to evade subsequent array unwrapping.
+ * in order to avoid subsequent array unwrapping.
  *
  * All boolean expressions (predicates) are evaluated by executeBoolItem()
  * function, which returns tri-state JsonPathBool.  When error is occurred
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index de38c6c7e0..c25a22f79b 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1428,11 +1428,11 @@ tuplesort_updatemax(Tuplesortstate *state)
     }
 
     /*
-     * Sort evicts data to the disk when it didn't manage to fit those data to
-     * the main memory.  This is why we assume space used on the disk to be
+     * Sort evicts data to the disk when it didn't fit data in
+     * main memory.  This is why we assume space used on the disk to be
      * more important for tracking resource usage than space used in memory.
-     * Note that amount of space occupied by some tuple set on the disk might
-     * be less than amount of space occupied by the same tuple set in the
+     * Note that the amount of space occupied by some tupleset on the disk might
+     * be less than amount of space occupied by the same tupleset in
      * memory due to more compact representation.
      */
     if ((isSpaceDisk && !state->isMaxSpaceDisk) ||
diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h
index f7af921f5a..88f4c9a53f 100644
--- a/src/include/lib/simplehash.h
+++ b/src/include/lib/simplehash.h
@@ -560,7 +560,7 @@ restart:
         uint32        curoptimal;
         SH_ELEMENT_TYPE *entry = &data[curelem];
 
-        /* any empty bucket can directly be used */
+        /* any empty bucket can be used directly */
         if (entry->status == SH_STATUS_EMPTY)
         {
             tb->members++;

-- 
Justin

Some new bits,
And some old ones.

-- 
Justin

On Thu, Jun 11, 2020 at 09:37:09PM -0500, Justin Pryzby wrote:
> Some new bits,
> And some old ones.

I was looking at this patch set, and 0005 has attracted my attention
here:

> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -4240,7 +4240,6 @@ AttrDefaultFetch(Relation relation)
>      HeapTuple    htup;
>      Datum        val;
>      bool        isnull;
> -    int            found;
>      int            i;

Since 16828d5, this variable is indeed unused.  Now, the same commit
has removed the following code:
-   if (found != ndef)
-       elog(WARNING, "%d attrdef record(s) missing for rel %s",
-            ndef - found, RelationGetRelationName(relation));

Should we actually keep this variable and have this sanity check in
place?  It seems to me that it would be good to have that, so as we
can make sure that the number of default attributes cached matches
with the number of defaults actually found when scanning each
attribute.  Adding in CC Andrew as the author of 16828d5 for more
input.
--
Michael

On Thu, Jun 11, 2020 at 09:37:09PM -0500, Justin Pryzby wrote:
> Some new bits,
> And some old ones.

I have merged 0003 and 0004 together and applied them.  0005 seems to
have a separate issue as mentioned upthread, and I have not really
looked at 0001 and 0002.  Thanks.
--
Michael

On Fri, Jun 12, 2020 at 09:13:02PM +0900, Michael Paquier wrote:
> I have merged 0003 and 0004 together and applied them.  0005 seems to
> have a separate issue as mentioned upthread, and I have not really
> looked at 0001 and 0002.  Thanks.

And committed 0001 and 0002 after some tiny adjustments as of
7a3543c.
--
Michael

I stand by these changes which I proposed handful of times since April, but not
yet included by Michael's previous commits.

-- 
Justin

On Tue, Aug 18, 2020 at 12:17:03PM -0500, Justin Pryzby wrote:
>         The WAL sender process is currently performing
> -       <function>pg_start_backup</function> to set up for
> -       taking a base backup, and waiting for backup start
> +       <function>pg_start_backup</function> to prepare to
> +       take a base backup, and waiting for the start-of-backup
>         checkpoint to finish.

Wouldn't it be more simple to use "to prepare for a base backup" here?

>          Run the dump in parallel by dumping <replaceable class="parameter">njobs</replaceable>
> -        tables simultaneously. This option reduces the time of the dump but it also
> +        tables simultaneously. This option may reduces the time needed to perform the dump but it also
>          increases the load on the database server. You can only use this option with the
> [...]
>          Execute the reindex commands in parallel by running
>          <replaceable class="parameter">njobs</replaceable>
> -        commands simultaneously.  This option reduces the time of the
> -        processing but it also increases the load on the database server.
> +        commands simultaneously.  This option may reduce the processing time
> +        but it also increases the load on the database server.
> [...]
>          Execute the vacuum or analyze commands in parallel by running
>          <replaceable class="parameter">njobs</replaceable>
> -        commands simultaneously.  This option reduces the time of the
> -        processing but it also increases the load on the database server.
> +        commands simultaneously.  This option may reduce the processing time
> +        but it also increases the load on the database server.
>         </para>
>         <para>
>          <application>vacuumdb</application> will open

The original versions are fine IMO.

>       Replication is only supported by tables, including partitioned tables.
>       Attempts to replicate other types of relations such as views, materialized
> -     views, or foreign tables, will result in an error.
> +     views, or foreign tables will result in an error.
>      </para>

I think that the original is fine.

>       * If the partition's attributes don't match the root relation's, we'll
>       * need to make a new attrmap which maps partition attribute numbers to
> -     * remoterel's, instead the original which maps root relation's attribute
> +     * remoterel's, instead of the original which maps root relation's attribute
>       * numbers to remoterel's.

Indeed.

>        from the parent table will be created in the partition, if they don't
>        already exist.
>        If any of the <literal>CHECK</literal> constraints of the table being
> -      attached is marked <literal>NO INHERIT</literal>, the command will fail;
> +      attached are marked <literal>NO INHERIT</literal>, the command will fail;
>        such constraints must be recreated without the
>        <literal>NO INHERIT</literal> clause.

Singular or plural depends on the context when if comes to any with a
countable word, and plural looks more natural to me here.  So, right.

>      enough for error messages.  Detail and hint messages can be relegated to a
>      verbose mode, or perhaps a pop-up error-details window.  Also, details and
>      hints would normally be suppressed from the server log to save
> -    space. Reference to implementation details is best avoided since users
> -    aren't expected to know the details.
> +    space. References to implementation details are best avoided since users
> +    aren't expected to know them.

Original is fine IMO (see 6335c80).

>          not contain any checksums. Otherwise, it will contain a checksum
>          of each file in the backup using the specified algorithm. In addition,
>          the manifest will always contain a <literal>SHA256</literal>
> -        checksum of its own contents. The <literal>SHA</literal> algorithms
> +        checksum of its own content. The <literal>SHA</literal> algorithms
>          are significantly more CPU-intensive than <literal>CRC32C</literal>,
>          so selecting one of them may increase the time required to complete
>          the backup.
> [...]
>     every check which will be performed by a running server when attempting
>     to make use of the backup. Even if you use this tool, you should still
>     perform test restores and verify that the resulting databases work as
> -   expected and that they appear to contain the correct data. However,
> +   expected and that they contain the correct data. However,
>     <application>pg_verifybackup</application> can detect many problems
>     that commonly occur due to storage problems or user error.
> [...]
> @@ -82,7 +82,7 @@ PostgreSQL documentation
>     for any files for which the computed checksum does not match the
>     checksum stored in the manifest. This step is not performed for any files
>     which produced errors in the previous step, since they are already known
> -   to have problems. Also, files which were ignored in the previous step are
> +   to have problems. Files which were ignored in the previous step are
>     also ignored in this step.

No sure this needs to change

>    </para>
>
> @@ -121,7 +121,7 @@ PostgreSQL documentation
>    <title>Options</title>
>
>     <para>
> -    The following command-line options control the behavior.
> +    The following command-line options control the behavior of this program.

"pg_verifybackup accepts the following command-line arguments:" is
more consistent with the style of all the other tools.  This needs to
be fixed.

>      The <productname>PostgreSQL</productname> server will listen for both
>      normal and <acronym>GSSAPI</acronym>-encrypted connections on the same TCP
> -    port, and will negotiate with any connecting client on whether to
> +    port, and will negotiate with any connecting client whether to
>      use <acronym>GSSAPI</acronym> for encryption (and for authentication).  By

Right.

>       specify suppression of the <literal>CONTEXT:</literal> portion of a message in
>       the postmaster log.  This should only be used for verbose debugging
>       messages where the repeated inclusion of context would bloat the log
> -     volume too much.
> +     too much.

Okay here.

>       A logical slot will emit each change just once in normal operation.
>       The current position of each slot is persisted only at checkpoint, so in
>       the case of a crash the slot may return to an earlier LSN, which will
> -     then cause recent changes to be resent when the server restarts.
> +     then cause recent changes to be re-sent when the server restarts.
>       Logical decoding clients are responsible for avoiding ill effects from
>       handling the same message more than once.  Clients may wish to record
>       the last LSN they saw when decoding and skip over any repeated data or
> [...]
>            It is safe to use <literal>off</literal> for logical replication:
>            If the subscriber loses transactions because of missing
> -          synchronization, the data will be resent from the publisher.
> +          synchronization, the data will be re-sent from the publisher.
>           </para>
> [...]
> -            /* prevent signal from being resent more than once */
> +            /* prevent signal from being re-sent more than once */
>              allow_autovacuum_cancel = false;

"resent" is wrong, but "re-sent" does not sound like the best choice
to me.  Shouldn't we just say "sent again" for all three places?
--
Michael

On Mon, Aug 31, 2020 at 04:28:20PM +0900, Michael Paquier wrote:
> On Tue, Aug 18, 2020 at 12:17:03PM -0500, Justin Pryzby wrote:
> >         The WAL sender process is currently performing
> > -       <function>pg_start_backup</function> to set up for
> > -       taking a base backup, and waiting for backup start
> > +       <function>pg_start_backup</function> to prepare to
> > +       take a base backup, and waiting for the start-of-backup
> >         checkpoint to finish.
> 
> Wouldn't it be more simple to use "to prepare for a base backup" here?

I think it's useful to say "prepare to take" since it's more specific..  It's
not "preparing to receive" or "preparing to scan" or "preparing to parse".

> >       Replication is only supported by tables, including partitioned tables.
> >       Attempts to replicate other types of relations such as views, materialized
> > -     views, or foreign tables, will result in an error.
> > +     views, or foreign tables will result in an error.
> >      </para>
> 
> I think that the original is fine.

I think this is indisputably wrong, but I realized that it's actually better
with an *additional* comma:

|       Attempts to replicate other types of relations COMMA such as views, materialized
|       views, or foreign tables, will result in an error.

> >    </para>
> >  
> > @@ -121,7 +121,7 @@ PostgreSQL documentation
> >    <title>Options</title>
> >  
> >     <para>
> > -    The following command-line options control the behavior.
> > +    The following command-line options control the behavior of this program.
> 
> "pg_verifybackup accepts the following command-line arguments:" is
> more consistent with the style of all the other tools.  This needs to
> be fixed.

> > -   to have problems. Also, files which were ignored in the previous step are
> > +   to have problems. Files which were ignored in the previous step are
> >     also ignored in this step.
> 
> No sure this needs to change

Two "also"s seems poor, and the first one detracts from the 2nd.

> >       the case of a crash the slot may return to an earlier LSN, which will
> > -     then cause recent changes to be resent when the server restarts.
> > +     then cause recent changes to be re-sent when the server restarts.
> >       Logical decoding clients are responsible for avoiding ill effects from
> >       handling the same message more than once.  Clients may wish to record
> >       the last LSN they saw when decoding and skip over any repeated data or
> > [...]
> >            It is safe to use <literal>off</literal> for logical replication:
> >            If the subscriber loses transactions because of missing
> > -          synchronization, the data will be resent from the publisher.
> > +          synchronization, the data will be re-sent from the publisher.
> >           </para>
> > [...]
> > -            /* prevent signal from being resent more than once */
> > +            /* prevent signal from being re-sent more than once */
> >              allow_autovacuum_cancel = false;
> 
> "resent" is wrong, but "re-sent" does not sound like the best choice
> to me.  Shouldn't we just say "sent again" for all three places?

I don't think so.

-- 
Justin

On Mon, Aug 31, 2020 at 08:42:08AM -0500, Justin Pryzby wrote:
> On Mon, Aug 31, 2020 at 04:28:20PM +0900, Michael Paquier wrote:
>> Wouldn't it be more simple to use "to prepare for a base backup" here?
>
> I think it's useful to say "prepare to take" since it's more specific..  It's
> not "preparing to receive" or "preparing to scan" or "preparing to parse".

Not sure I see the point in complicating the sentence here more than
necessary.

>>> -   to have problems. Also, files which were ignored in the previous step are
>>> +   to have problems. Files which were ignored in the previous step are
>>>     also ignored in this step.
>>
>> No sure this needs to change
>
> Two "also"s seems poor, and the first one detracts from the 2nd.

Ah, OK.  Indeed.

>> "resent" is wrong, but "re-sent" does not sound like the best choice
>> to me.  Shouldn't we just say "sent again" for all three places?
>
> I don't think so.

Well, using "sent again" has the advantage to about any ambiguity in
the way it gets read.  So I'd still prefer that when using the past
tense of "send" in those sentences.  Any opinions from others?
--
Michael

I've added a few more.

-- 
Justin

On Wed, Sep 09, 2020 at 09:37:42AM -0500, Justin Pryzby wrote:
> I've added a few more.

I have done an extra round of review on this patch series, and applied
what looked obvious to me (basically the points already discussed
upthread).  Some parts applied down to 9.6 for the docs.
--
Michael

On Thu, Sep 10, 2020 at 03:58:31PM +0900, Michael Paquier wrote:
> On Wed, Sep 09, 2020 at 09:37:42AM -0500, Justin Pryzby wrote:
> > I've added a few more.
> 
> I have done an extra round of review on this patch series, and applied
> what looked obvious to me (basically the points already discussed
> upthread).  Some parts applied down to 9.6 for the docs.

Thanks.  Here's the remainder, with some new ones.

-- 
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:
> Thanks.  Here's the remainder, with some new ones.

LGTM.  I tweaked one or two places a bit more, and pushed it.

            regards, tom lane