shihao zhong <[email protected]> writes:
>> We should address this case as well, or is it reasonable to ignore it as
>> an edge case?
> I can reproduce it, and I think we should fix it. Guessing the encoding
> gettext uses keeps missing cases.
> v6 does two things. It treats OUTPUT_CHARSET as set if it was set when
> psql started, since gettext keeps using that value.
I thought that it'd be a good idea to push the infrastructure part of
this (ie, inventing addQueryComment) separately, since that step is
bulky, boring, and different between the branches. So I did that,
and now we just need to discuss what logic to put into
addQueryComment. I've attached a v7 to show what remains to do.
I'm not convinced by the OUTPUT_CHARSET handling in v6: it only
detects the situation where OUTPUT_CHARSET is set at the moment we're
trying to output a label. If you are worried that gettext will latch
onto a no-longer-present setting then that's not really good enough.
What I put into v7 was an addition in exec_command_setenv() to
remember if we ever did \setenv OUTPUT_CHARSET. AFAIK that is the
only pathway for psql env vars to change after startup, so that seems
like it ought to be bulletproof.
Having said that, I'm really quite unconvinced that we should be
expending any effort on OUTPUT_CHARSET at all. If someone is using
that to select an encoding that doesn't match psql's locale, they
are going to have lots more problems besides this one, and it's
all self-inflicted damage. So my vote is to rip out all the logic
related to OUTPUT_CHARSET.
The other thing I'm not super pleased about is invoking
pg_get_encoding_from_locale() each time through. That seems like a
moderately expensive operation. I also wonder whether the result
can change intra-run and whether gettext will track it if so.
regards, tom lane
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5737f62d2da..4e256a3b932 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2971,6 +2971,9 @@ exec_command_setenv(PsqlScanState scan_state, bool active_branch,
/* Set variable to the value of the next argument */
setenv(envvar, envval, 1);
success = true;
+ /* Hack: track whether we ever set OUTPUT_CHARSET */
+ if (strcmp(envvar, "OUTPUT_CHARSET") == 0)
+ pset.output_charset = true;
}
free(envvar);
free(envval);
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 788555b9782..9b3ae1ed238 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -28,6 +28,7 @@
#include "fe_utils/mbprint.h"
#include "fe_utils/string_utils.h"
#include "mainloop.h"
+#include "mb/pg_wchar.h"
#include "portability/instr_time.h"
#include "settings.h"
@@ -2630,9 +2631,37 @@ command_no_begin(const char *query)
void
addQueryComment(PQExpBuffer buf, const char *label)
{
-#if defined(ENABLE_NLS)
- /* This will become conditional once we've settled on the tests to use */
- label = _(label);
+ /*
+ * We must not translate the label unless we know that it is representable
+ * in the server's encoding; for now, restrict ourselves to the case where
+ * server_encoding == client_encoding. Even then there's a hazard that
+ * gettext might produce some other encoding. Normally gettext emits the
+ * encoding of LC_CTYPE, but OUTPUT_CHARSET can override that. gettext
+ * may keep using the OUTPUT_CHARSET it saw first, so give up if that was
+ * set on startup or has been set anytime since then. On Windows the
+ * encoding gettext uses is not certain to match what we would compute, so
+ * don't try at all there.
+ */
+#if defined(ENABLE_NLS) && !defined(WIN32)
+ const char *server_enc = PQparameterStatus(pset.db, "server_encoding");
+ int client_enc = PQclientEncoding(pset.db);
+
+ if (!pset.output_charset &&
+ server_enc != NULL &&
+ pg_char_to_encoding(server_enc) == client_enc &&
+ pg_get_encoding_from_locale(NULL, false) == client_enc)
+ {
+ const char *translated = _(label);
+ int len = strlen(translated);
+
+ /*
+ * In case the checks above miss something, make sure the server
+ * accepts the bytes. With equal client and server encodings, this is
+ * the check the server does.
+ */
+ if (pg_encoding_verifymbstr(client_enc, translated, len) == len)
+ label = translated;
+ }
#endif
printfPQExpBuffer(buf, "/* %s */\n", label);
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index a3d088d80cd..80fa45a166a 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -134,6 +134,7 @@ typedef struct _psqlSettings
bool notty; /* stdin or stdout is not a tty (as determined
* on startup) */
+ bool output_charset; /* OUTPUT_CHARSET has ever been set */
enum trivalue getPassword; /* prompt the user for a username and password */
FILE *cur_cmd_source; /* describe the status of the current main
* loop */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index bb33c7c14d7..20ed28f3db9 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -135,6 +135,7 @@ main(int argc, char *argv[])
pg_logging_set_pre_callback(log_pre_callback);
pg_logging_set_locus_callback(log_locus_callback);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("psql"));
+ pset.output_charset = (getenv("OUTPUT_CHARSET") != NULL);
if (argc > 1)
{
