RE: [PGdocs] fix description for handling pf non-ASCII characters

Dear Karl,

Thank you for reviewing! PSA new version.

> I see a few problems with the English and style of the patches
> and am commenting below and have signed up as a reviewer.

Your effort is quite helpful for me.

> At
> commitfest.postgresql.org I have marked the thread
> as needing author attention.  Hayato, you will need
> to mark the thread as needing review when you have
> replied to this message.

Sure. I will change the status after posting the patch.

Before replying your comments, I thought I should show the difference between
versions. Regarding old versions (here PG15 was used), non-ASCIIs (like Japanese) are
replaced with '?'.

```
psql (15.4)
Type "help" for help.

postgres=# SET application_name TO 'あああ';
SET
postgres=# SHOW application_name ;
 application_name
------------------
 ?????????
(1 row)
```

As for the HEAD, as my patch said, non-ASCIIs are replaced
with hexadecimal representations. (Were my terminologies correct?).

```
psql (17devel)
Type "help" for help.

postgres=# SET application_name TO 'あああ';
SET
postgres=# SHOW application_name ;
           application_name
--------------------------------------
 \xe3\x81\x82\xe3\x81\x82\xe3\x81\x82
(1 row)
```

Note that non-printable ASCIIs are also replaced with the same rule.

```
psql (15.4)
Type "help" for help.

postgres=# SET application_name TO E'\x03';
SET
postgres=# SHOW application_name ;
 application_name
------------------
 ?
(1 row)

psql (17devel)
Type "help" for help.

postgres=# SET application_name TO E'\x03';
SET
postgres=# SHOW application_name ;
 application_name
------------------
 \x03
(1 row)
```

> Now, to reviewing the patch:
>
> First, it is now best practice in the PG docs to
> put a line break at the end of each sentence.
> At least for the sentences on the lines you change.
> (No need to update the whole document!)  Please
> do this in the next version of your patch.  I don't
> know if this is a requirement for acceptance by
> a committer, but it won't hurt.

I didn't know that. Could you please tell me if you have a source? Anyway,
I put a line break for each sentences for now.

> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index e700782d3c..a4ce99ba4d 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -7040,9 +7040,8 @@ local0.*    /var/log/postgresql
>          The name will be displayed in the
> <structname>pg_stat_activity</structname> view and included in CSV log
> entries.  It can also be included in regular log entries via the <xref
> linkend="guc-log-line-prefix"/> parameter.
> -        Only printable ASCII characters may be used in the
> -        <varname>application_name</varname> value. Other characters
> will be
> -        replaced with question marks (<literal>?</literal>).
> +        Non-ASCII characters used in the
> <varname>application_name</varname>
> +        will be replaced with hexadecimal strings.
>         </para>
>        </listitem>
>       </varlistentry>
>
> Don't use the future tense to describe the system's behavior.  Instead
> of "will be" write "are".  (Yes, this change would be an improvement
> on the original text.  We should fix it while we're working on it
> and our attention is focused.)
>
> It is more accurate, if I understand the issue, to say that characters
> are replaced with hexadecimal _representations_ of the input bytes.
> Finally, it would be good to know what representation we're talking
> about.  Perhaps just give the \xhh example and say: the Postgres
> C-style escaped hexadecimal byte value.  And hyperlink to
> https://www.postgresql.org/docs/16/sql-syntax-lexical.html#SQL-SYNTAX-ST
> RINGS-ESCAPE
>
> (The docbook would be, depending on text you want to link:
>
> <link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal
> byte value</link>.
>
> I think.  You link to id="someidvalue" attribute values.)

IIUC the word " Postgres" cannot be used in the doc.
Based on your all comments, I changed as below. How do you think?

```
        Characters that are not printable ASCII, like <literal>\x03</literal>,
        are replaced with the <productname>PostgreSQL</productname>
        <link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal byte value</link>.
```


> @@ -8037,10 +8036,9 @@ COPY postgres_log FROM
> '/full/path/to/logfile.csv' WITH csv; <para>
>          The name can be any string of less
>          than <symbol>NAMEDATALEN</symbol> characters (64 characters
> in
> a standard
> -        build). Only printable ASCII characters may be used in the
> -        <varname>cluster_name</varname> value. Other characters will be
> -        replaced with question marks (<literal>?</literal>).  No name
> is shown
> -        if this parameter is set to the empty string
> <literal>''</literal> (which is
> +        build). Non-ASCII characters used in the
> <varname>cluster_name</varname>
> +        will be replaced with hexadecimal strings. No name is shown if
> this
> +        parameter is set to the empty string <literal>''</literal>
> (which is the default). This parameter can only be set at server start.
>         </para>
>        </listitem>
>
> Same review as for the first patch hunk.

Fixed like above. You can refer the patch.

> diff --git a/doc/src/sgml/postgres-fdw.sgml
> b/doc/src/sgml/postgres-fdw.sgml index 5062d712e7..98785e87ea 100644
> --- a/doc/src/sgml/postgres-fdw.sgml
> +++ b/doc/src/sgml/postgres-fdw.sgml
> @@ -1067,9 +1067,8 @@ postgres=# SELECT postgres_fdw_disconnect_all();
>        of any length and contain even non-ASCII characters.  However
> when it's passed to and used as <varname>application_name</varname>
>        in a foreign server, note that it will be truncated to less than
> -      <symbol>NAMEDATALEN</symbol> characters and anything other than
> -      printable ASCII characters will be replaced with question
> -      marks (<literal>?</literal>).
> +      <symbol>NAMEDATALEN</symbol> characters and non-ASCII
> characters
> will be
> +      replaced with hexadecimal strings.
>        See <xref linkend="guc-application-name"/> for details.
>       </para>
>
> Same review as for the first patch hunk.

Fixed like above.

> Since the both of you have looked and confirmed that the
> actual behavior matches the new documentation I have not
> done this.

I showed the result again, please see.

> But, have either of you checked that we're really talking about
> replacing everything outside the 7-bit ASCII encodings?
> My reading of the commit referenced in the first email of this
> thread says that it's everything outside of the _printable_
> ASCII encodings, ASCII values outside of the range 32 to 127,
> inclusive.
>
> Please check.  The docs should probably say "printable ASCII",
> or "non-printable ASCII", depending.  I think the meaning
> of "printable ASCII" is widely enough known to be 32-127.
> So "printable" is good enough, right?

For me, "non-printable ASCII" sounds like control characters. So that I used the
sentence "Characters that are not printable ASCII ... are replaced with...".
Please tell me if you have better explanation?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED