Thread: typos

In docs and comments.  Mostly for v15.
Maybe Fabien will want to comment on the pgbench one.

( Added Joe and Robert for 0011 )

On Mon, 11 Apr 2022 at 14:03, Justin Pryzby <pryzby@telsasoft.com> wrote:
> In docs and comments.  Mostly for v15.

0001:

Should this not use <productname>PostgreSQL</productname>? (new to master)

0002:

The patch looks good. (new to v12)

0003:

The patch looks good. (new to master)

0004:

The patch looks good. (new to master)

0005:

I'm not entirely certain this is an improvement.  Your commit message
I'd say is not true going by git grep "compression algorithm". There
are 3 matches in the docs and take [1], for example. I'd say in that
one it's better to use "algorithm".  In that case, "method" could be
talking about client or server.

That makes me wonder if the change you're proposing is an improvement or not.

0006:

The patch looks good. (new to master)

0007:

-        When the <option>--max-tries</option> option is used, the
transaction with
-        serialization or deadlock error cannot be retried if the total time of
+        When the <option>--max-tries</option> option is used, a
transaction with
+        serialization or deadlock error will not be retried if the
total time of

Shouldn't this be slightly clearer and say "a transaction which fails
due to a serialization anomaly or a deadlock"?

-               database server / the syntax error in the meta command / thread
+               database server / syntax error in the meta command / thread

Should we not separate these items out with commas?

-   the client is aborted. Otherwise, if an SQL fails with serialization or
+   the client is aborted. Otherwise, if an SQL command fails with
serialization or
    deadlock errors, the client is not aborted. In such cases, the current

I'd say "if an SQL command fails due to a serialization anomaly or due
to deadlocking".

(new to master)

0008:

The patch looks good. (new to master)

0009:

The patch looks good. (new to master)

0010:

I don't understand this change.

0011:

I can't quite parse the original. I might not have enough context
here.  Robert, Joe? (new to master)

0012:

This seems to contain some documentation fixes too. The patch claims
it's just for comments.

- * pages that are outwith that range.
+ * pages that are outside that range.

I personally don't see the issue here, but I'm Scottish. I think the
best transaction is just "outside of" rather than replacing with just
"outside".

All the other changes look fine.

0013:

I think we should fix all these, regardless of how old the mistake is.

0014:

- * shouldn't PANIC just because we can't guarantee the the backup has been
+ * shouldn't PANIC just because we can't guarantee the backup has been

"that the"

0015:

Patch looks fine.

0016:
0017:

I'm not really sure if we should fix these or not.  From having a
quick look at some of them it seems we'd be adding churn to some
pretty old code. Happy to hear what others think.

0018:

The patch looks good.

0019:

-1. pgindent will fix these.

I will start pushing the less controversial of these, after a bit of squashing.

David

[1] https://www.postgresql.org/docs/devel/app-pgbasebackup.html

On Mon, 11 Apr 2022 at 16:39, David Rowley <dgrowleyml@gmail.com> wrote:
> I will start pushing the less controversial of these, after a bit of squashing.

I just committed 3 separate commits for the following:

Committed: 0001 + 0003 + 0004 + 0006 + 0007 (modified) + 0008 + 0009 +
0012 (doc parts)
Committed: 0012 (remainder) + 0013 + 0014 + 0018
Committed: 0015

I skipped:
0002 (skipped as we should backpatch)
0005 (unsure if the proposed patch is better)
0010 (I can't follow this change)
0011 (Could do with input from Robert and Joe)

and also skipped:
0016 (unsure if we should change these of pgindent is not touching it)
0017 (unsure if we should change these of pgindent is not touching it)
0019 (pgindent will get these when the time comes)

I'll wait for feedback on the ones I didn't use.

Are you able to rebase the remainder? Probably with the exception of 0019.

Thanks for finding all these!

David

On Mon, Apr 11, 2022 at 04:39:30PM +1200, David Rowley wrote:
> I'm not entirely certain this is an improvement.  Your commit message
> I'd say is not true going by git grep "compression algorithm". There
> are 3 matches in the docs and take [1], for example. I'd say in that
> one it's better to use "algorithm".  In that case, "method" could be
> talking about client or server.

I am not wedded to this change; but, for context, I wrote this patch before
basebackup supported multiple compression ...  "things".  I didn't touch
basebackup here, since Robert defended that choice of words in another thread
(starting at 20220320194050.GX28503@telsasoft.com).

This change is for pg_column_compression(), and the only other use of
"compression algorithm" in the docs is in pgcrypto (which is in contrib).  That
the docs consistently use "method" suggests continuing to use that rather than
something else.  It could be described in some central place (like if we
support common syntax between interfaces which expose compression).

> 0010:
> I don't understand this change.

The commit message mentions 959f6d6a1, which makes newbindir optional.  But the
documentation wasn't updated, and seems to indicate that it's still required.
https://www.postgresql.org/docs/devel/pgupgrade.html

> 0011:
> I can't quite parse the original. I might not have enough context
> here.  Robert, Joe? (new to master)

See the link in the commit message where someone else reported the same
problem.

> 0019:
> -1. pgindent will fix these.

But two of those are from 2016.

Thanks for amending and pushing those.  There's some more less obvious ones
attached.

Amit or Masahiko may want to comment on 0012 (doc review: Add ALTER
SUBSCRIPTION ... SKIP).

On Mon, Apr 11, 2022 at 7:10 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Apr 11, 2022 at 04:39:30PM +1200, David Rowley wrote:
> > I'm not entirely certain this is an improvement.  Your commit message
> > I'd say is not true going by git grep "compression algorithm". There
> > are 3 matches in the docs and take [1], for example. I'd say in that
> > one it's better to use "algorithm".  In that case, "method" could be
> > talking about client or server.
>
> I am not wedded to this change; but, for context, I wrote this patch before
> basebackup supported multiple compression ...  "things".  I didn't touch
> basebackup here, since Robert defended that choice of words in another thread
> (starting at 20220320194050.GX28503@telsasoft.com).
>
> This change is for pg_column_compression(), and the only other use of
> "compression algorithm" in the docs is in pgcrypto (which is in contrib).  That
> the docs consistently use "method" suggests continuing to use that rather than
> something else.  It could be described in some central place (like if we
> support common syntax between interfaces which expose compression).
>
> > 0010:
> > I don't understand this change.
>
> The commit message mentions 959f6d6a1, which makes newbindir optional.  But the
> documentation wasn't updated, and seems to indicate that it's still required.
> https://www.postgresql.org/docs/devel/pgupgrade.html
>
> > 0011:
> > I can't quite parse the original. I might not have enough context
> > here.  Robert, Joe? (new to master)
>
> See the link in the commit message where someone else reported the same
> problem.
>
> > 0019:
> > -1. pgindent will fix these.
>
> But two of those are from 2016.
>
> Thanks for amending and pushing those.  There's some more less obvious ones
> attached.
>
> Amit or Masahiko may want to comment on 0012 (doc review: Add ALTER
> SUBSCRIPTION ... SKIP).

Thank you for the patch! I've looked at 0012 patch. Regarding the
following part:

    <function>pg_replication_origin_advance()</function></link> function
-   transaction.  Before using this function, the subscription needs
to be disabled
+   XXX? transaction.  Before using this function, the subscription
needs to be disabled
    temporarily either by <command>ALTER SUBSCRIPTION ...
DISABLE</command> or, the

we can remove "transaction", it seems a typo. The rest looks good to me.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Mon, Apr 11, 2022 at 3:55 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Apr 11, 2022 at 7:10 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > Amit or Masahiko may want to comment on 0012 (doc review: Add ALTER
> > SUBSCRIPTION ... SKIP).
>
> Thank you for the patch! I've looked at 0012 patch. Regarding the
> following part:
>
>     <function>pg_replication_origin_advance()</function></link> function
> -   transaction.  Before using this function, the subscription needs
> to be disabled
> +   XXX? transaction.  Before using this function, the subscription
> needs to be disabled
>     temporarily either by <command>ALTER SUBSCRIPTION ...
> DISABLE</command> or, the
>
> we can remove "transaction", it seems a typo.
>

Right.

> The rest looks good to me.
>

+1. I'll take care of pushing this one tomorrow unless we have more
comments on this part.

-- 
With Regards,
Amit Kapila.

On Mon, Apr 11, 2022 at 4:56 AM David Rowley <dgrowleyml@gmail.com> wrote:
> 0011 (Could do with input from Robert and Joe)

Seems like a reasonable change to me.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Apr 11, 2022 at 4:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 11, 2022 at 3:55 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Apr 11, 2022 at 7:10 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > Amit or Masahiko may want to comment on 0012 (doc review: Add ALTER
> > > SUBSCRIPTION ... SKIP).
> >
>
> +1. I'll take care of pushing this one tomorrow unless we have more
> comments on this part.
>

I have pushed this one.

-- 
With Regards,
Amit Kapila.

On 2022-Apr-11, David Rowley wrote:

> and also skipped:
> 0016 (unsure if we should change these of pgindent is not touching it)
> 0017 (unsure if we should change these of pgindent is not touching it)

I verified that pgindent will indeed not touch these changes by running
before and after.  (I accepted one comment placement from that run that
touched a neighboring line.)

I think pgindent is right not to modify vertical space very much, since
in many cases it amounts to a subjective decision.  The patch seemed a
(small) improvement, and it seems hard to make too much of a fuss about
such things.  Pushed them as a single commit.

I hadn't noticed that Justin had posted a refreshed patch series, so I
don't know if the new ones match what I pushed.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
 PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

On Wed, Apr 13, 2022 at 07:29:34PM +0200, Alvaro Herrera wrote:
> On 2022-Apr-11, David Rowley wrote:
> 
> > and also skipped:
> > 0016 (unsure if we should change these of pgindent is not touching it)
> > 0017 (unsure if we should change these of pgindent is not touching it)
> 
> I verified that pgindent will indeed not touch these changes by running
> before and after.  (I accepted one comment placement from that run that
> touched a neighboring line.)
> 
> I think pgindent is right not to modify vertical space very much, since
> in many cases it amounts to a subjective decision.  The patch seemed a
> (small) improvement, and it seems hard to make too much of a fuss about
> such things.  Pushed them as a single commit.
> 
> I hadn't noticed that Justin had posted a refreshed patch series, so I
> don't know if the new ones match what I pushed.

There were no changes - I had resent the patches that removed blank lines so it
was apparent that they were "outstanding" / under discussion.

There's (only) a few remaining.

On Mon, 11 Apr 2022 at 22:10, Justin Pryzby <pryzby@telsasoft.com> wrote:
> Thanks for amending and pushing those.  There's some more less obvious ones
> attached.

Here are my notes from yesterday that I made when reviewing and
pushing many of the 2nd batch of patches.

0001: Pushed and back patched to v12

0002: Didn't push. Compression method/algorithm.

0003: Pushed and backpatched to v13

0004: Pushed (reviewed by Robert)

0005: Alvaro Pushed
0006: Alvaro Pushed

0007: Not pushed. No space after comment and closing */  pgindent
fixed one of these but not the other 2.  I've not looked into why
pgindent does 1 and not the other 2.

0008: Pushed

I've left out the following change as it does not seem to be bringing
any sort of consistency to the docs overall. It only brings
consistency to a single source file in the docs.

-      You need <productname>zstd</productname>, if you want to support
+      You need <productname>ZSTD</productname>, if you want to support

See: git grep -i ">zstd<"

0009:

This contains a few fixes that look correct. Not sure if the following
has any use as a change:

-    See the description of the respective commands and programs for the
-    respective details.  Note that you can mix locale providers on different
+    See the description of the respective commands and programs for
+    details.  Note that you can mix locale providers at different

0010: Pushed

0011: Not pushed. Not sure if this is worth the change.

0012: Amit Pushed

0013: Not pushed. Adds a missing comma.

David

(For the future, just to make discussions easier, it would be good if
you could have git format-patch -v N to give a unique version number
to these patches)

On Thu, 14 Apr 2022 at 05:40, Justin Pryzby <pryzby@telsasoft.com> wrote:
> There's (only) a few remaining.

I've pushed 0001 and 0002 of the 3rd batch of patches. I left 0003 as
I just didn't feel it was a meaningful enough improvement.

From docs/, if I do:

$ git grep ", which is the default" | wc -l
9

$ git grep ", the default" | wc -l
64

You're proposing to make the score 10, 63.  I'm not sure if that's a
good direction to go in.

David

On Thu, Apr 14, 2022 at 09:39:42AM +1200, David Rowley wrote:
> On Thu, 14 Apr 2022 at 05:40, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > There's (only) a few remaining.
> 
> I've pushed 0001 and 0002 of the 3rd batch of patches. I left 0003 as

Thanks

> I just didn't feel it was a meaningful enough improvement.
> 
> From docs/, if I do:
> 
> $ git grep ", which is the default" | wc -l
> 9
> 
> $ git grep ", the default" | wc -l
> 64
> 
> You're proposing to make the score 10, 63.  I'm not sure if that's a
> good direction to go in.

Well, I'm proposing to change the only instance of this:

$ git grep -F ", the default)"
doc/src/sgml/ref/create_publication.sgml:   partition's row filter (if the parameter is false, the default) or the
root

Maybe what's needed is more like this.

   If the publication contains a partitioned table, and the publication parameter
   <literal>publish_via_partition_root</literal> is false (the default), then the
   row filter is taken from the partition; otherwise, the row filter is taken
   from the root partitioned table.

I'll plan to keep this around and may come back to it later.

On Thu, Apr 14, 2022 at 08:56:22AM +1200, David Rowley wrote:
> I've left out the following change as it does not seem to be bringing
> any sort of consistency to the docs overall. It only brings
> consistency to a single source file in the docs.
> 
> -      You need <productname>zstd</productname>, if you want to support
> +      You need <productname>ZSTD</productname>, if you want to support
> 
> See: git grep -i ">zstd<"

It may not be worth changing just this one line, but the reason I included it
here is for consistency:

$ git grep 'zstd.*product' doc
doc/src/sgml/config.sgml:        <literal>zstd</literal> (if <productname>PostgreSQL</productname>
$ git grep 'ZSTD.*product' doc
doc/src/sgml/install-windows.sgml:     <term><productname>ZSTD</productname></term>
doc/src/sgml/install-windows.sgml:      Required for supporting <productname>ZSTD</productname> compression
doc/src/sgml/installation.sgml:      You need <productname>ZSTD</productname>, if you want to support
doc/src/sgml/installation.sgml:         Build with <productname>ZSTD</productname> compression support.

If we were to change it, maybe they should all say "Zstandard (zstd)".  ZSTD
looks like an acronym, which I think it is not, and Zstandard indicates how to
pronounce it.

CCing Amit K, because I propose a few relatively minor changes to
logical rep docs.

On 2022-Apr-13, Justin Pryzby wrote:

> $ git grep -F ", the default)"
> doc/src/sgml/ref/create_publication.sgml:   partition's row filter (if the parameter is false, the default) or the
root
> 
> Maybe what's needed is more like this.
> 
>    If the publication contains a partitioned table, and the publication parameter
>    <literal>publish_via_partition_root</literal> is false (the default), then the
>    row filter is taken from the partition; otherwise, the row filter is taken
>    from the root partitioned table.

Yeah, more invasive rewording seems called for.  I propose this:

   For publications containing partitioned tables, the row filter for each
   partition is taken from the published partitioned table if the
   publication parameter <literal>publish_via_partition_root</literal> is true,
   or from the partition itself if it is false (the default).

I think we should also mention that this parameter affects row filters,
in the <varlistentry> for the WITH clause.  Currently it has

        <term><literal>publish_via_partition_root</literal> (<type>boolean</type>)</term>
        <listitem>
         <para>
          This parameter determines whether changes in a partitioned table (or
          on its partitions) contained in the publication will be published
          using the identity and schema of the partitioned table rather than
          that of the individual partitions that are actually changed; the
          latter is the default.  Enabling this allows the changes to be
          replicated into a non-partitioned table or a partitioned table
          consisting of a different set of partitions.
         </para>

I propose to add 

        <term><literal>publish_via_partition_root</literal> (<type>boolean</type>)</term>
        <listitem>
         <para>
          This parameter determines whether changes in a partitioned table (or
          on its partitions) contained in the publication will be published
          using the identity and schema of the partitioned table rather than
          that of the individual partitions that are actually changed; the
          latter is the default.  Enabling this allows the changes to be
          replicated into a non-partitioned table or a partitioned table
          consisting of a different set of partitions.
         </para>

        <para>
         This parameter also affects how row filters are chosen for partitions;
         see below for details.
        </para>

More generally, I think we need to connect the WHERE keyword with "row
filters" more explicitly.  Right now, the parameter reference says

      If the optional <literal>WHERE</literal> clause is specified, rows for
      which the <replaceable class="parameter">expression</replaceable>
      evaluates to false or null will not be published. Note that parentheses
      are required around the expression. It has no effect on
      <literal>TRUNCATE</literal> commands.

I propose to make it "If the optional WHERE clause is specified, it
defines a <firstterm>row filter</firstterm> expression.  Rows for which
the row filter expression evaluates to false ..."


> $ git grep 'zstd.*product' doc
> doc/src/sgml/config.sgml:        <literal>zstd</literal> (if <productname>PostgreSQL</productname>
> $ git grep 'ZSTD.*product' doc
> doc/src/sgml/install-windows.sgml:     <term><productname>ZSTD</productname></term>
> doc/src/sgml/install-windows.sgml:      Required for supporting <productname>ZSTD</productname> compression
> doc/src/sgml/installation.sgml:      You need <productname>ZSTD</productname>, if you want to support
> doc/src/sgml/installation.sgml:         Build with <productname>ZSTD</productname> compression support.

I don't see any official sources calling it all-uppercase ZSTD.  In a
quick non-scientific survey, most seem to use Zstd or zstd.  The
non-abbreviated official name is Zstandard, but it's hard to find any
places using that spelling, and I don't think our docs are a place to
educate people on what the official name or pronunciation is.

I propose we standardize on <productname>Zstd</productname> everywhere.
Users can look it up if they're really interested.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Tue, Apr 19, 2022 at 4:35 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Yeah, more invasive rewording seems called for.  I propose this:
>
>    For publications containing partitioned tables, the row filter for each
>    partition is taken from the published partitioned table if the
>    publication parameter <literal>publish_via_partition_root</literal> is true,
>    or from the partition itself if it is false (the default).
>
> I think we should also mention that this parameter affects row filters,
> in the <varlistentry> for the WITH clause.  Currently it has
>
>         <term><literal>publish_via_partition_root</literal> (<type>boolean</type>)</term>
>         <listitem>
>          <para>
>           This parameter determines whether changes in a partitioned table (or
>           on its partitions) contained in the publication will be published
>           using the identity and schema of the partitioned table rather than
>           that of the individual partitions that are actually changed; the
>           latter is the default.  Enabling this allows the changes to be
>           replicated into a non-partitioned table or a partitioned table
>           consisting of a different set of partitions.
>          </para>
>
> I propose to add
>
>         <term><literal>publish_via_partition_root</literal> (<type>boolean</type>)</term>
>         <listitem>
>          <para>
>           This parameter determines whether changes in a partitioned table (or
>           on its partitions) contained in the publication will be published
>           using the identity and schema of the partitioned table rather than
>           that of the individual partitions that are actually changed; the
>           latter is the default.  Enabling this allows the changes to be
>           replicated into a non-partitioned table or a partitioned table
>           consisting of a different set of partitions.
>          </para>
>
>         <para>
>          This parameter also affects how row filters are chosen for partitions;
>          see below for details.
>         </para>
>

Your proposed changes look good to me but I think all these places
need to mention 'column list' as well because the behavior is the same
for it.

> More generally, I think we need to connect the WHERE keyword with "row
> filters" more explicitly.  Right now, the parameter reference says
>
>       If the optional <literal>WHERE</literal> clause is specified, rows for
>       which the <replaceable class="parameter">expression</replaceable>
>       evaluates to false or null will not be published. Note that parentheses
>       are required around the expression. It has no effect on
>       <literal>TRUNCATE</literal> commands.
>
> I propose to make it "If the optional WHERE clause is specified, it
> defines a <firstterm>row filter</firstterm> expression.  Rows for which
> the row filter expression evaluates to false ..."
>

Looks good to me.

-- 
With Regards,
Amit Kapila.

On 2022-Apr-20, Amit Kapila wrote:

> Your proposed changes look good to me but I think all these places
> need to mention 'column list' as well because the behavior is the same
> for it.

Hmm, you're right.  Added that, and changed the wording somewhat because
some things read awkwardly.  Here's the result in patch form.

Column lists seems not mentioned in logical-replication.sgml, either.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La verdad no siempre es bonita, pero el hambre de ella sí"

On 2022-Apr-19, Alvaro Herrera wrote:

> I propose we standardize on <productname>Zstd</productname> everywhere.
> Users can look it up if they're really interested.

So the attached.

There are other uses of <literal>zstd</literal>, but those are referring to the
executable program.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)

On Wed, Apr 20, 2022 at 11:32:08PM +0200, Alvaro Herrera wrote:
> On 2022-Apr-19, Alvaro Herrera wrote:
> 
> > I propose we standardize on <productname>Zstd</productname> everywhere.
> > Users can look it up if they're really interested.
> 
> So the attached.
> 
> There are other uses of <literal>zstd</literal>, but those are referring to the
> executable program.

This one shouldn't be changed, or not like this?

> @@ -560,7 +560,7 @@ $ENV{PROVE_TESTS}='t/020*.pl t/010*.pl'
>      <varlistentry>
>       <term><varname>ZSTD</varname></term>
>       <listitem><para>
> -      Path to a <application>zstd</application> command. The default is
> +      Path to a <application>Zstd</application> command. The default is
>        <literal>zstd</literal>, which will search for a command by that
>        name in the configured <envar>PATH</envar>.
>       </para></listitem>

Maybe it should say s/a/the/, like:

-      Path to a <application>zstd</application> command. The default is
+      Path to the <application>zstd</application> command. The default is

On Wed, Apr 20, 2022 at 5:31 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Apr-20, Amit Kapila wrote:
>
> > Your proposed changes look good to me but I think all these places
> > need to mention 'column list' as well because the behavior is the same
> > for it.
>
> Hmm, you're right.  Added that, and changed the wording somewhat because
> some things read awkwardly.  Here's the result in patch form.
>

LGTM.

-- 
With Regards,
Amit Kapila.

On Wed, Apr 20, 2022 at 11:32:08PM +0200, Alvaro Herrera wrote:
> So the attached.
>
> --- a/doc/src/sgml/install-windows.sgml
> +++ b/doc/src/sgml/install-windows.sgml
> @@ -307,9 +307,9 @@ $ENV{MSBFLAGS}="/m";
>      </varlistentry>
>
>      <varlistentry>
> -     <term><productname>ZSTD</productname></term>
> +     <term><productname>Zstd</productname></term>
>       <listitem><para>
> -      Required for supporting <productname>ZSTD</productname> compression
> +      Required for supporting <productname>Zstd</productname> compression

Looking at the zstd project itself for reference or just wiki-sensei,
I don't think that this is correct:
https://github.com/facebook/zstd
https://en.wikipedia.org/wiki/Zstd

Their README uses "zstd" in lower-case, while "Zstd" (first letter
upper-case) is used at the beginning of a sentence.
--
Michael

On 21.04.22 06:36, Michael Paquier wrote:
> On Wed, Apr 20, 2022 at 11:32:08PM +0200, Alvaro Herrera wrote:
>> So the attached.
>>
>> --- a/doc/src/sgml/install-windows.sgml
>> +++ b/doc/src/sgml/install-windows.sgml
>> @@ -307,9 +307,9 @@ $ENV{MSBFLAGS}="/m";
>>       </varlistentry>
>>   
>>       <varlistentry>
>> -     <term><productname>ZSTD</productname></term>
>> +     <term><productname>Zstd</productname></term>
>>        <listitem><para>
>> -      Required for supporting <productname>ZSTD</productname> compression
>> +      Required for supporting <productname>Zstd</productname> compression
> 
> Looking at the zstd project itself for reference or just wiki-sensei,
> I don't think that this is correct:
> https://github.com/facebook/zstd
> https://en.wikipedia.org/wiki/Zstd
> 
> Their README uses "zstd" in lower-case, while "Zstd" (first letter
> upper-case) is used at the beginning of a sentence.

It is referred to as "Zstandard" at both of those places.  Maybe we 
should use that.  That is also easier to pronounce.

On 2022-Apr-21, Peter Eisentraut wrote:

> It is referred to as "Zstandard" at both of those places.  Maybe we should
> use that.  That is also easier to pronounce.

Yeah, I looked at other places (such as Yann Collet's blog) and I agree
that Zstandard seems to be the accepted spelling of the product.  Pushed
that way.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes...  Fixed.
        http://archives.postgresql.org/message-id/482D1632.8010507@sigaev.ru

I found a bunch more typos; a couple from codespell, and several which are the
result of looking for previously-reported typos, like:

time git log origin --grep '[tT]ypo' --word-diff -U1 |grep -Eo '\[-[[:lower:]]+-\]' |sed 's/^\[-//; s/-\]$//' |sort -u
|grep-Fxvwf /usr/share/dict/words >badwords.txt
 
time grep -rhoFwf badwords.txt doc |sort -u >not-badwords.txt
time grep -Fxvwf not-badwords.txt ./badwords.txt >./badwords.txt.new
time grep -rhoIFwf ./badwords.txt.new src --incl='*.[chly]' --incl='*.p[lm]' |sort |uniq -c |sort -nr |less

On Tue, May 10, 2022 at 09:03:34PM -0500, Justin Pryzby wrote:
> I found a bunch more typos; a couple from codespell, and several which are the
> result of looking for previously-reported typos, like:

Thanks, applied 0002.

Regarding 0001, I don't really know which one of {AND,OR}ed or
{AND,OR}-ed is better.  Note that the code prefers the former, but
your patch changes the docs to use the latter.
--
Michael

On Thu, Apr 14, 2022 at 08:56:22AM +1200, David Rowley wrote:
> 0007: Not pushed. No space after comment and closing */  pgindent
> fixed one of these but not the other 2.  I've not looked into why
> pgindent does 1 and not the other 2.

> -/* get operation priority  by its code*/
> +/* get operation priority by its code */

pgindent never touches comments that start in column zero.  (That's why many
column-0 comments are wrapped to widths other than the standard 78.)

On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote:

     # Use larger ccache cache, as this task compiles with multiple compilers /
     # flag combinations
-    CCACHE_MAXSIZE: "1GB"
+    CCACHE_MAXSIZE: "1G"

In 0006, I am not sure how much this matters.  Perhaps somebody more
fluent with Cirrus, though, has a different opinion..

  * pointer to this structure.  The information here must be sufficient to
  * properly initialize each new TableScanDesc as workers join the scan, and it
- * must act as a information what to scan for those workers.
+ * must provide information what to scan for those workers.

This comment in 0009 is obviously incorrect, but I am not sure whether
your new suggestion is an improvement.  Do workers provide such
information or has this structure some information that the workers
rely on?

Not sure that the whitespace issue in 0021 for the header of inval.c
is worth caring about.

0014 and 0013 do not reduce the translation workload, as the messages
include some stuff specific to the GUC names accessed to, or some
specific details about the code paths triggered.

The rest has been applied where they matter.
--
Michael

On Tue, Jan 3, 2023 at 12:58 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote:
>
>      # Use larger ccache cache, as this task compiles with multiple compilers /
>      # flag combinations
> -    CCACHE_MAXSIZE: "1GB"
> +    CCACHE_MAXSIZE: "1G"
>
> In 0006, I am not sure how much this matters.
>

The other places in that file use M, so maybe, this is more consistent.

One minor comment:
-    spoken in Belgium (BE), with a <acronym>UTF-8</acronym> character set
+    spoken in Belgium (BE), with a <acronym>UTF</acronym>-8 character set

Shouldn't this be <acronym>UTF8</acronym> as we are using in func.sgml?

-- 
With Regards,
Amit Kapila.

On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:
> One minor comment:
> -    spoken in Belgium (BE), with a <acronym>UTF-8</acronym> character set
> +    spoken in Belgium (BE), with a <acronym>UTF</acronym>-8 character set
>
> Shouldn't this be <acronym>UTF8</acronym> as we are using in func.sgml?

Yeah, I was wondering as well why this change is not worse, which is
why I left it out of 33ab0a2.  There is an acronym for UTF in
acronym.sgml, which makes sense to me, but that's the only place where
this is used.  To add more on top of that, the docs basically need
only UTF8, and we have three references to UTF-16, none of them using
the <acronym> markup.
--
Michael

On 03.01.23 09:41, Michael Paquier wrote:
> On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:
>> One minor comment:
>> -    spoken in Belgium (BE), with a <acronym>UTF-8</acronym> character set
>> +    spoken in Belgium (BE), with a <acronym>UTF</acronym>-8 character set
>>
>> Shouldn't this be <acronym>UTF8</acronym> as we are using in func.sgml?
> 
> Yeah, I was wondering as well why this change is not worse, which is
> why I left it out of 33ab0a2.  There is an acronym for UTF in
> acronym.sgml, which makes sense to me, but that's the only place where
> this is used.  To add more on top of that, the docs basically need
> only UTF8, and we have three references to UTF-16, none of them using
> the <acronym> markup.

The thing is called "UTF-8".  Here, we are not talking about the 
PostgreSQL identifier.

On Tue, Jan 03, 2023 at 04:28:29PM +0900, Michael Paquier wrote:
> On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote:
> 
>      # Use larger ccache cache, as this task compiles with multiple compilers /
>      # flag combinations
> -    CCACHE_MAXSIZE: "1GB"
> +    CCACHE_MAXSIZE: "1G"
> 
> In 0006, I am not sure how much this matters.  Perhaps somebody more
> fluent with Cirrus, though, has a different opinion..

It's got almost nothing to do with cirrus.  It's an environment
variable, and we're using a suffix other than what's
supported/documented by ccache, which only happens to work.

> 0014 and 0013 do not reduce the translation workload, as the messages
> include some stuff specific to the GUC names accessed to, or some
> specific details about the code paths triggered.

It seems to matter because otherwise the translators sometimes re-type
the view name, which (not surprisingly) can get messed up, which is how
I mentioned having noticed this.

On Tue, Jan 03, 2023 at 05:41:58PM +0900, Michael Paquier wrote:
> On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:
> > One minor comment:
> > -    spoken in Belgium (BE), with a <acronym>UTF-8</acronym>
> > character set
> > +    spoken in Belgium (BE), with a <acronym>UTF</acronym>-8
> > character set
> > 
> > Shouldn't this be <acronym>UTF8</acronym> as we are using in
> > func.sgml?
> 
> Yeah, I was wondering as well why this change is not worse, which is
> why I left it out of 33ab0a2.  There is an acronym for UTF in
> acronym.sgml, which makes sense to me, but that's the only place where 
> this is used.  To add more on top of that, the docs basically need
> only UTF8, and we have three references to UTF-16, none of them using
> the <acronym> markup.

I changed it for consistency, as it's the only thing that says <>UTF-8<>
anywhere, and charset.sgml already says <>UTF<>-8 elsewhere.

Alternately, I suggest to change charset to say <>UTF8<> in both places.

-- 
Justin

On Tue, Jan 03, 2023 at 03:39:22PM -0600, Justin Pryzby wrote:
> On Tue, Jan 03, 2023 at 04:28:29PM +0900, Michael Paquier wrote:
> > On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote:
> > 
> >      # Use larger ccache cache, as this task compiles with multiple compilers /
> >      # flag combinations
> > -    CCACHE_MAXSIZE: "1GB"
> > +    CCACHE_MAXSIZE: "1G"
> > 
> > In 0006, I am not sure how much this matters.  Perhaps somebody more
> > fluent with Cirrus, though, has a different opinion..
> 
> It's got almost nothing to do with cirrus.  It's an environment
> variable, and we're using a suffix other than what's
> supported/documented by ccache, which only happens to work.
> 
> > 0014 and 0013 do not reduce the translation workload, as the messages
> > include some stuff specific to the GUC names accessed to, or some
> > specific details about the code paths triggered.
> 
> It seems to matter because otherwise the translators sometimes re-type
> the view name, which (not surprisingly) can get messed up, which is how
> I mentioned having noticed this.
> 
> On Tue, Jan 03, 2023 at 05:41:58PM +0900, Michael Paquier wrote:
> > On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:
> > > One minor comment:
> > > -    spoken in Belgium (BE), with a <acronym>UTF-8</acronym>
> > > character set
> > > +    spoken in Belgium (BE), with a <acronym>UTF</acronym>-8
> > > character set
> > > 
> > > Shouldn't this be <acronym>UTF8</acronym> as we are using in
> > > func.sgml?
> > 
> > Yeah, I was wondering as well why this change is not worse, which is
> > why I left it out of 33ab0a2.  There is an acronym for UTF in
> > acronym.sgml, which makes sense to me, but that's the only place where 
> > this is used.  To add more on top of that, the docs basically need
> > only UTF8, and we have three references to UTF-16, none of them using
> > the <acronym> markup.
> 
> I changed it for consistency, as it's the only thing that says <>UTF-8<>
> anywhere, and charset.sgml already says <>UTF<>-8 elsewhere.
> 
> Alternately, I suggest to change charset to say <>UTF8<> in both places.

As attached.
This also fixes "specualtive" in Amit's recent commit.

-- 
Justin

On Tue, Jan 10, 2023 at 10:27 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Tue, Jan 03, 2023 at 03:39:22PM -0600, Justin Pryzby wrote:
> > On Tue, Jan 03, 2023 at 04:28:29PM +0900, Michael Paquier wrote:
> > > On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote:
> > >
> > >      # Use larger ccache cache, as this task compiles with multiple compilers /
> > >      # flag combinations
> > > -    CCACHE_MAXSIZE: "1GB"
> > > +    CCACHE_MAXSIZE: "1G"
> > >
> > > In 0006, I am not sure how much this matters.  Perhaps somebody more
> > > fluent with Cirrus, though, has a different opinion..
> >
> > It's got almost nothing to do with cirrus.  It's an environment
> > variable, and we're using a suffix other than what's
> > supported/documented by ccache, which only happens to work.
> >
> > > 0014 and 0013 do not reduce the translation workload, as the messages
> > > include some stuff specific to the GUC names accessed to, or some
> > > specific details about the code paths triggered.
> >
> > It seems to matter because otherwise the translators sometimes re-type
> > the view name, which (not surprisingly) can get messed up, which is how
> > I mentioned having noticed this.
> >
> > On Tue, Jan 03, 2023 at 05:41:58PM +0900, Michael Paquier wrote:
> > > On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:
> > > > One minor comment:
> > > > -    spoken in Belgium (BE), with a <acronym>UTF-8</acronym>
> > > > character set
> > > > +    spoken in Belgium (BE), with a <acronym>UTF</acronym>-8
> > > > character set
> > > >
> > > > Shouldn't this be <acronym>UTF8</acronym> as we are using in
> > > > func.sgml?
> > >
> > > Yeah, I was wondering as well why this change is not worse, which is
> > > why I left it out of 33ab0a2.  There is an acronym for UTF in
> > > acronym.sgml, which makes sense to me, but that's the only place where
> > > this is used.  To add more on top of that, the docs basically need
> > > only UTF8, and we have three references to UTF-16, none of them using
> > > the <acronym> markup.
> >
> > I changed it for consistency, as it's the only thing that says <>UTF-8<>
> > anywhere, and charset.sgml already says <>UTF<>-8 elsewhere.
> >
> > Alternately, I suggest to change charset to say <>UTF8<> in both places.
>
> As attached.
> This also fixes "specualtive" in Amit's recent commit.
>

Thanks for noticing this. I'll take care of this and some other typo
patches together.

-- 
With Regards,
Amit Kapila.

On Tue, Jan 10, 2023 at 12:24:40PM +0530, Amit Kapila wrote:
> Thanks for noticing this. I'll take care of this and some other typo
> patches together.

Does this include 0010?  I was just looking at the whole set and this
one looked like a cleanup worth on its own so I was going to handle
it, until I saw your update.  If you are also looking at that, I won't
stand in your way, of course :)
--
Michael

On Tue, Jan 10, 2023 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jan 10, 2023 at 12:24:40PM +0530, Amit Kapila wrote:
> > Thanks for noticing this. I'll take care of this and some other typo
> > patches together.
>
> Does this include 0010?  I was just looking at the whole set and this
> one looked like a cleanup worth on its own so I was going to handle
> it, until I saw your update.  If you are also looking at that, I won't
> stand in your way, of course :)
>

I have not yet started, so please go ahead.

-- 
With Regards,
Amit Kapila.

On Tue, Jan 10, 2023 at 01:55:56PM +0530, Amit Kapila wrote:
> I have not yet started, so please go ahead.

Okay, I have looked at that and fixed the whole new things, including
the typo you have introduced.  0001~0004 have been left out, as of the
same reasons as upthread.
--
Michael

Some more accumulated/new typos.

On Wed, Feb 08, 2023 at 09:56:44AM -0600, Justin Pryzby wrote:
> Some more accumulated/new typos.

0001 has been a debate for a long time, and it depends on the way SQL
is spelled.  For reference:
$ git grep -i " an sql" -- *.c | wc -l
63
$ git grep -i " a sql" -- *.c | wc -l
135

0005 can indeed fix a lot of confusion around the spaces after an
"else if" block.  Is that something that could be automated with the
indentation, though?  Same remark for 0009 and 0010.

Applied 0002, 0003, 0004, 0006, after rewording a bit 0003 to mention
the compression type.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Feb 08, 2023 at 09:56:44AM -0600, Justin Pryzby wrote:
>> Some more accumulated/new typos.

> 0005 can indeed fix a lot of confusion around the spaces after an
> "else if" block.  Is that something that could be automated with the
> indentation, though?  Same remark for 0009 and 0010.

I see your point about 0005, but I've never seen pgindent remove
vertical whitespace once it's been added.  Not sure what it'd take
to teach it to do so, or whether we'd like the results.

I'd reject 0009 and 0010 altogether --- they don't add any readability
that's worth the potential increase in back-patch problems.

            regards, tom lane