Re: doc review for v13 - Mailing list pgsql-hackers

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