Date: Tue, 26 Sep 2023 01:03:28 -0500
From: "Karl O. Pinc" <kop@karlpinc.com>
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, 'jian he'
 <jian.universality@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PGdocs] fix description for handling pf non-ASCII characters
Message-ID: <20230926010328.2c556046@slate.karlpinc.com>
In-Reply-To: <TYAPR01MB5866CD2A823B196FBE7F19B9F52EA@TYAPR01MB5866.jpnprd01.prod.outlook.com>
References: <TYAPR01MB586631D0961BF9C44893FAB1F523A@TYAPR01MB5866.jpnprd01.prod.outlook.com>
	<CACJufxFkp8c0ap6YhyXRpVkjsB8pwZjWH9Hqc1tVF0aNWFRzFA@mail.gmail.com>
	<TYAPR01MB58662BC412E1290FC3348973F525A@TYAPR01MB5866.jpnprd01.prod.outlook.com>
	<CACJufxHn1=Phy6E=1PDO0AMXhP1S+XrP5Cioa_rynYpO7gC+=g@mail.gmail.com>
	<TYAPR01MB5866CD2A823B196FBE7F19B9F52EA@TYAPR01MB5866.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20230926010328.2c556046%40slate.karlpinc.com>
Precedence: bulk

Hello Hayato and Jian,

On Tue, 4 Jul 2023 01:30:56 +0000
"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> 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 <kop@karlpinc.com>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein