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

From: "Karl O. Pinc" <[email-masked]>
Date: Tue, 26 Sep 2023 01:03:28 -0500

Hello Hayato and Jian,

On Tue, 4 Jul 2023 01:30:56 +0000
"Hayato Kuroda (Fujitsu)" <[email-masked]> wrote:

> Dear Jian,

> > looks fine. Do you need to add to commitfest?  
> 
> Thank you for your confirmation. ! I registered to following:
> 
> https://commitfest.postgresql.org/44/4437/

The way the Postgres commitfest process works is that
someone has to update the page to mark "reviewed" and the
reviewer has to use the commitfest website to pass
the patches to a committer.

I see a few problems with the English and style of the patches
and am commenting below and have signed up as a reviewer.  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.

Jian, you might want to sign on as a reviewer as well.
It can be nice to have that record of your participation.

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.

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-STRINGS-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.)

@@ -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.

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.

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

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?

Regards,

Karl <[email-masked]>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 26 Sep 2023 13:40:26 +0000

Dear Karl,

Thank you for reviewing! PSA new version.

> and am commenting below and have signed up as a reviewer.

Your effort is quite helpful for me.

> At

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)
```

> 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.

> 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>.
```

> Same review as for the first patch hunk.

Fixed like above. You can refer the patch.

> Same review as for the first patch hunk.

Fixed like above.

> actual behavior matches the new documentation I have not

I showed the result again, please see.

> 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

---

From: "Karl O. Pinc" <[email-masked]>
Date: Tue, 26 Sep 2023 12:45:53 -0500

On Tue, 26 Sep 2023 13:40:26 +0000

> Your effort is quite helpful for me.

You're welcome.

> 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.
> 

I think you're terminology is correct, but see my
suggestion at bottom.

Never hurts to have output to look at.  I noticed here
and when reading the patch that changed the output --
the patch is operating on bytes, not characters per-se.

> I didn't know that. Could you please tell me if you have a source?

I thought I could find an email but the search is taking
forever.  If I find something I'll let you know.
I could even be mis-remembering, but it's a nice
practice regardless.

There are a number of undocumented conventions.
Another is the use of gender neutral language.

The place to discuss doc conventions/styles would
be the pgsql-docs list.  (If you felt like
asking there.)

You could try submitting another patch to add various
doc conventions to the official docs at
https://www.postgresql.org/docs/current/docguide-style.html
:-)

> Anyway, I put a line break for each sentences for now.

Thanks.

A related thing that's nice to have is to limit the line
length of the documentation source to 80 characters or less.
79 is probably best.  Since the source text around your patch
conforms to this convention you should also.

> IIUC the word " Postgres" cannot be used in the doc.

I think you're right.

>         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>. ```

> I showed the result again, please see.

Should the committer be interested, your patch applies cleanly
and the docs build as expected.

Also, based on the comments in the
patch which changed the system's behavior, I believe that
your patch updates all the relevant places in the documentation.

> 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?

Your explanation sounds good to me.

I now think that you should consider another change to your wording.
Instead of starting with "Characters that are not printable ASCII ..."
consider writing "The bytes of the string which are not printable ASCII
...".  Notice above that characters (e.g. あ) generate output for
each non-ASCII byte (e.g. \xe3\x81\x82).  So my thought is that
the docs should be talking about bytes.

For the last hunk you'd change around "anything".  Write:
"... it will be truncated to less than NAMEDATALEN characters and
the bytes of the string which are not printable ASCII characters ...".

Notice that I have also changed "that" to "which" just above.  
I _think_ this is better English.  See sense 3 of:
https://en.wiktionary.org/wiki/which
See also the first paragraph of:
https://en.wikipedia.org/wiki/Relative_pronoun

If the comments above move you, send another patch.  Seems to me
we're close to sending this on to a committer.

Regards,

Karl <[email-masked]>

---

From: Tom Lane <[email-masked]>
Date: Tue, 26 Sep 2023 14:01:23 -0400

> I _think_ this is better English.

No, I'm pretty sure you're mistaken.  It's been a long time since
high school English, but the way I think this works is that "that"
introduces a restrictive clause, which narrows the scope of what
you are saying.  That is, you say "that" when you want to talk
about only the bytes of the string that aren't ASCII.  But "which"
introduces a non-restrictive clause that adds information or
commentary.  If you say "bytes of the string which are not ASCII",
you are effectively making a side assertion that no byte of the
string is ASCII.  Which is not the meaning you want here.

A smell test that works for native speakers (not sure how helpful
it is for others) is: if the sentence would read well with commas
or parens added before and after the clause, then it's probably
non-restrictive and should use "which".  If it looks wrong that way
then it's a restrictive clause and should use "that".

			regards, tom lane

---

From: "Karl O. Pinc" <[email-masked]>
Date: Tue, 26 Sep 2023 16:50:20 -0500 (CDT)

> string is ASCII.  Which is not the meaning you want here.

Makes sense to me.  "That" it is.

Thanks for the help. I never would have figured that out.

---

From: Peter Smith <[email-masked]>
Date: Wed, 27 Sep 2023 16:48:30 +1000

Hi Kuroda-san.

Here are my review comments for your v3 patch.

TBH, I felt the new text descriptions deviated a bit too much from the
originals. IMO only quite a small tweak was needed, so my suggested
text in the comments below reflects that.

======
Commit message.

1.
missing description

======
src/sgml/config.sgml

2. application_name:

-        Only printable ASCII characters may be used in the
-        <varname>application_name</varname> value. Other characters will be
-        replaced with question marks (<literal>?</literal>).
+        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>.

BEFORE
Other characters will be replaced with question marks (<literal>?</literal>).

SUGGESTION
Other characters will be replaced with <link
linkend="sql-syntax-strings-escape">C-style escaped hexadecimal byte
values</link>.

~~~

3. cluster_name:

-        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
-        the default). This parameter can only be set at server start.
+        build).
+        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>.
+        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.

<same as previous review comment #2>

======
src/sgml/postgres-fdw.sgml

4.
      <para>
       <varname>postgres_fdw.application_name</varname> can be any string
-      of any length and contain even non-ASCII characters.  However when
-      it's passed to and used as <varname>application_name</varname>
+      of any length and contain even characters that are not printable ASCII.
+      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>).
+      printable ASCII characters are replaced with the
<productname>PostgreSQL</productname>
+      <link linkend="sql-syntax-strings-escape">C-style escaped
hexadecimal byte value</link>.
       See <xref linkend="guc-application-name"/> for details.
      </para>

~

AFAICT the first change wasn't necessary.

~

As for the 2nd change:

BEFORE
... and anything other than printable ASCII characters will be
replaced with question marks (<literal>?</literal>).

SUGGESTION
... and anything other than printable ASCII characters will be
replaced with <link linkend="sql-syntax-strings-escape">C-style
escaped hexadecimal byte values</link>.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 27 Sep 2023 12:58:54 +0000

Dear Karl,

Thank you for reviewing!

> conforms to this convention you should also.

IIUC it is not hard limit, but I followed this.

> and the docs build as expected.

Yeah, but cfbot accepted previous version. Did you have anything in your mind?

> your patch updates all the relevant places in the documentation.

Thanks. Actually, I think it should be backpatched to PG16 because the commit was
done last year. I will make the patch for it after deciding the explanation.

> the bytes of the string which are not printable ASCII characters ...".

Hmm, what you said looked right. But as Peter pointed out [1], the fix seems too
much. So I attached three version of patches. How do you think?
For me, type C is best.

A. A patch which completely follows your comments. The name is "v3-0001-...patch".
   Cfbot tests it.
B. A patch which completely follows Peter's comments [1]. The name is "Peter_v3-....txt".
C. A patch which follows both comments. Based on b, but some comments
   (Don't use the future tense, "Other characters"->"The bytes of other characters"...)
   were picked. The name is "Both_v3-....txt".

[1]: https://www.postgresql.org/message-id/CAHut%2BPvEbKC8ABA_daX-XPNOTFzuAmHGhjPj%3DtPZYQskRHECOg%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

From c3a2c68fd74bce14af6df7c773317d496cf75209 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email-masked]>
Date: Wed, 27 Sep 2023 07:31:07 +0000
Subject: [PATCH v32] Fix description for handling of non-printable ASCII
 characters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

45b1a67a changed the behavior when characters that are not printable ASCII were
used for three configuration parameters (application_name, cluster_name, and
postgres_fdw.application_name), but it was not documented. This commit fixes
that.

PG15 and prior:

```

PG16 and later:

```
postgres=# SET application_name TO 'あああ';
SET
postgres=# SHOW application_name ;
           application_name
--------------------------------------
 \xe3\x81\x82\xe3\x81\x82\xe3\x81\x82
(1 row)
```
---
 doc/src/sgml/config.sgml       | 17 +++++++++++------
 doc/src/sgml/postgres-fdw.sgml |  7 ++++---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 38684af5b1..587bdf3d72 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6893,8 +6893,10 @@ local0.*    /var/log/postgresql
         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>).
+        <varname>application_name</varname> value.
+        The bytes of other characters are replaced with
+        <link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal
+        byte values</link>.
        </para>
       </listitem>
      </varlistentry>
@@ -7890,10 +7892,13 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
         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
-        the default). This parameter can only be set at server start.
+        <varname>cluster_name</varname> value.
+        The bytes of other characters are replaced with
+        <link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal
+        byte values</link>.
+        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>
      </varlistentry>
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 5062d712e7..9894d61a98 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1067,9 +1067,10 @@ 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 the bytes of other than
+      printable ASCII characters are replaced with <link
+      linkend="sql-syntax-strings-escape">C-style escaped hexadecimal byte
+      values</link>.
       See <xref linkend="guc-application-name"/> for details.
      </para>
 
-- 
2.27.0

From 5f4dc139a3a401d22793e49d9a51caa00ba2ecad Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email-masked]>
Date: Wed, 27 Sep 2023 07:31:07 +0000
Subject: [PATCH v3] Fix description for handling of non-printable ASCII
 characters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

45b1a67a changed the behavior when characters that are not printable ASCII were
used for three configuration parameters (application_name, cluster_name, and
postgres_fdw.application_name), but it was not documented. This commit fixes
that.

PG15 and prior:

```

PG16 and later:

```
postgres=# SET application_name TO 'あああ';
SET
postgres=# SHOW application_name ;
           application_name
--------------------------------------
 \xe3\x81\x82\xe3\x81\x82\xe3\x81\x82
(1 row)
```
---
 doc/src/sgml/config.sgml       | 17 +++++++++++------
 doc/src/sgml/postgres-fdw.sgml |  7 ++++---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 38684af5b1..fdd93002bc 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6893,8 +6893,10 @@ local0.*    /var/log/postgresql
         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>).
+        <varname>application_name</varname> value.
+        Other characters will be replaced with
+        <link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal
+        byte values</link>.
        </para>
       </listitem>
      </varlistentry>
@@ -7890,10 +7892,13 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
         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
-        the default). This parameter can only be set at server start.
+        <varname>cluster_name</varname> value.
+        Other characters will be replaced with
+        <link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal
+        byte values</link>.
+        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>
      </varlistentry>
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 5062d712e7..522f736144 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1067,9 +1067,10 @@ 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 anything other than printable
+      ASCII characters will be replaced with <link
+      linkend="sql-syntax-strings-escape">C-style escaped hexadecimal byte
+      values</link>.
       See <xref linkend="guc-application-name"/> for details.
      </para>
 
-- 
2.27.0

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 27 Sep 2023 12:59:47 +0000

Dear Peter,

Thank you for reviewing!

> text in the comments below reflects that.

Good point, my patch may be too much.

> missing description

Added.
If we should use only printable ascii as a commit message, I can use '\x03'
instead of 'あああ'.

> escaped hexadecimal byte values</link>.

They seem good, but they conflict with Karl's comments.
I made three patches based on comments [1], could you check?

[1]: https://www.postgresql.org/message-id/TYAPR01MB58663EB061888B2715A39217F5C2A%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 27 Sep 2023 13:00:41 +0000

Dear Tom,

> then it's a restrictive clause and should use "that".

Thanks for giving your opinion. The suggestion is quite helpful for me,
non-native speaker. If you check my patch [1] I'm very happy.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Karl O. Pinc" <[email-masked]>
Date: Wed, 27 Sep 2023 08:59:24 -0500

On Wed, 27 Sep 2023 12:58:54 +0000

> Yeah, but cfbot accepted previous version. Did you have anything in

No.  I'm letting the committer know everything I've checked
so that they can decide what they want to check.

> Hmm, what you said looked right. But as Peter pointed out [1], the
> fix seems too much. So I attached three version of patches. How do
> you think? For me, type C is best.
> 
> A. A patch which completely follows your comments. The name is
> "v3-0001-...patch". Cfbot tests it.
> C. A patch which follows both comments. Based on
> b, but some comments (Don't use the future tense, "Other
> characters"->"The bytes of other characters"...) were picked. The
> name is "Both_v3-....txt".

I also like C.  Fewer words is better.  So long
as nothing is left unsaid fewer words make for clarity.

However, in the last hunk, "of other than" does not read well.
Instead of writing
"and the bytes of other than printable ASCII characters"
you want "and the bytes that are not printable ASCII characters".
That would be my suggestion.  

Regards,

Karl <[email-masked]>

---

From: Peter Smith <[email-masked]>
Date: Thu, 28 Sep 2023 09:49:03 +1000

> That would be my suggestion.

I also prefer Option C, but...

~~~

V

V

IIUC all of these 3 places can have exactly the same wording change
(e.g. like Karl's last suggestion [1]).

SUGGESTION
Any bytes that are not printable ASCII characters are replaced with
<link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal
byte values</link>.

======
[1] https://www.postgresql.org/message-id/20230927085924.4198c3d2%40slate.karlpinc.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Karl O. Pinc" <[email-masked]>
Date: Wed, 27 Sep 2023 19:30:36 -0500

On Thu, 28 Sep 2023 09:49:03 +1000
Peter Smith <[email-masked]> wrote:

> hexadecimal
> hexadecimal

I don't see the utility in having exactly the same phrase everywhere,
especially since the last hunk is modifying the end of a long
sentence.  (Apologies if I'm mis-reading what Peter wrote above.)

I like short sentences.  So I prefer "The bytes of other characters"
rather than "Any bytes that are not printable ASCII characters"
for the first 2 hunks.  In context I don't see the need to repeat
the whole "printable ASCII characters" part that appears in the
preceding sentence of both hunks.  "Other" is clear, IMHO.

But because I like short sentences I now think that it's a good
idea to break the long sentence of the last hunk into two.
Add a period and use the Peter's SUGGESTION above as the
text for the second sentence.

Is this desireable?

Regards,

Karl <[email-masked]>

P.S. Hayato, it is good practice to cc everybody who has
replied to a thread.  At least I think that's what I see,
it's not just people being lazy with reply-all. So I'm
adding Tom Lane back to the thread.  He can tell us otherwise
if I'm wrong to add him back.

---

From: Peter Smith <[email-masked]>
Date: Thu, 28 Sep 2023 11:13:40 +1000

> preceding sentence of both hunks.  "Other" is clear, IMHO.

I had in mind something like a SHIFT-JIS encoding where a single
"character" may include some trail bytes that happen to be in the
ASCII printable range. AFAIK because the new logic is processing
bytes, not characters, I thought the end result could be a mix of
escaped and unescaped bytes for the single SJIS character. In that
context, I felt "The bytes of other characters" was not quite
accurate.

But now looking at PostgreSQL-supported character sets [1] I saw SJIS
is not supported anyhow. Unfortunately, I am not familiar enough with
other encodings to know if there is still a chance of similar
printable ASCII trail bytes so I am fine with whatever wording is
chosen.

> text for the second sentence.

+1.

======
[1] https://www.postgresql.org/docs/current/multibyte.html

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Tom Lane <[email-masked]>
Date: Wed, 27 Sep 2023 21:19:31 -0400

Peter Smith <[email-masked]> writes:
> escaped and unescaped bytes for the single SJIS character.

It will not, because ...

> printable ASCII trail bytes so I am fine with whatever wording is

... trailing bytes that could be mistaken for ASCII are precisely
the property that causes us to reject an encoding as not backend-safe.
So this code doesn't need to consider that hazard, and processing the
string byte-by-byte is perfectly OK.

I'd be inclined to keep the text as simple as possible and not focus on
the distinction between bytes and characters.

			regards, tom lane

---

From: Peter Smith <[email-masked]>
Date: Thu, 28 Sep 2023 11:41:26 +1000

> the property that causes us to reject an encoding as not backend-safe.

Oh, that is good to know. Thanks for the information.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 28 Sep 2023 02:48:28 +0000

Dear Karl,

Thank you for giving comments! PSA new version.
I attached two patches - one is for HEAD, and another one is for REL_16_STABLE
branch. As shown below, PG16 has the same behavior.

```
psql (16beta3)
Type "help" for help.

> > > as nothing is left unsaid fewer words make for clarity.

Okay, basically I used C.

> > +      <symbol>NAMEDATALEN</symbol> characters and the bytes of
> other
> > than

Right, here we cannot use exactly the same sentence.

> preceding sentence of both hunks.  "Other" is clear, IMHO.

Based on the suggestion [1], I removed the word "byte".
(Sorry, but a comment from senior members has higher priority)

> text for the second sentence.

Right, the sentence is separated into two.

[1]: https://www.postgresql.org/message-id/803569.1695863971%40sss.pgh.pa.us

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 28 Sep 2023 02:50:55 +0000

Dear Peter,

Thank you for reviewing!

> > > I also prefer Option C, but...

Okay, C was chosen.

> > > <link linkend="sql-syntax-strings-escape">C-style escaped hexadecimal

Hmm, I felt that using exactly the same wording seemed strange here, so similar
words were used. Also, based on the comment [1], "byte" was removed.

> printable ASCII trail bytes so I am fine with whatever wording is

Based on the discussion [1], I did not handle the part.

> > text for the second sentence.

OK, divided.

New patch is available in [2].

[1]: https://www.postgresql.org/message-id/803569.1695863971%40sss.pgh.pa.us
[2]: https://www.postgresql.org/message-id/TYAPR01MB5866DD962CA4FC03E338C6BBF5C1A%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 28 Sep 2023 03:23:30 +0000

> branch. As shown below, PG16 has the same behavior.

Hmm, cfbot got angry because it tried to apply both of patches. To avoid it, I repost renamed patch.
(I'm happy if we can specify the target branch of patches)

Sorry for inconvenience.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

From 8f56b1cd93157a3c8b8f1c703c2752c0727e798c Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email-masked]>
Date: Wed, 27 Sep 2023 07:31:07 +0000
Subject: [PATCH v40] Fix description for handling of non-printable ASCII
 characters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PG15 and prior:

```

PG16 and later:

```
postgres=# SET application_name TO 'あああ';
SET
postgres=# SHOW application_name ;
           application_name
--------------------------------------
 \xe3\x81\x82\xe3\x81\x82\xe3\x81\x82
(1 row)
```
---
 doc/src/sgml/config.sgml       | 15 +++++++++------
 doc/src/sgml/postgres-fdw.sgml |  6 +++---
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4647539e40..126c147fc6 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7012,8 +7012,9 @@ local0.*    /var/log/postgresql
         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>).
+        <varname>application_name</varname> value.
+        Other characters are replaced with <link
+        linkend="sql-syntax-strings-escape">C-style hexadecimal escapes</link>.
        </para>
       </listitem>
      </varlistentry>
@@ -8009,10 +8010,12 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
         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
-        the default). This parameter can only be set at server start.
+        <varname>cluster_name</varname> value.
+        Other characters are replaced with <link
+        linkend="sql-syntax-strings-escape">C-style hexadecimal escapes</link>.
+        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>
      </varlistentry>
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 5062d712e7..c177fd41bc 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1067,9 +1067,9 @@ 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.
+      Anything other than printable ASCII characters are replaced with <link
+      linkend="sql-syntax-strings-escape">C-style hexadecimal escapes</link>.
       See <xref linkend="guc-application-name"/> for details.
      </para>
 
-- 
2.27.0

---

From: Michael Paquier <[email-masked]>
Date: Thu, 28 Sep 2023 12:54:33 +0900

> (I'm happy if we can specify the target branch of patches)

I was looking at this thread overall, the three v3 flavors of the doc
changes and v4.

The simplicity of the change in v4 seems like the best approach to me,
so +1 for that (including the mention to "C-style").
--
Michael

---

From: Peter Smith <[email-masked]>
Date: Thu, 28 Sep 2023 14:23:59 +1000

v4 LGTM.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Karl O. Pinc" <[email-masked]>
Date: Thu, 28 Sep 2023 00:58:28 -0500

On Thu, 28 Sep 2023 12:54:33 +0900

> so +1 for that (including the mention to "C-style").

I agree with Tom that it's not worth spending anyone's attention
on bytes v.s. characters.

So I'm marking the patch ready for committer.
(I have not tried the version that patches against PGv16.)

Thank you everyone, especially Hayato, for spending time
and making this better.

Regards,

Karl <[email-masked]>