Re: doc review for v13

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=&gt;
> (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