On Tue, Feb 22, 2022 at 07:10:44PM -0500, Wietse Venema wrote:

> There are two #ifdef SNAPSHOT blocks.
> 
> - The one in dict_pgsql_lookup() returns not found when SMTPUTF8
>   is enabled, but a query is not valid UTF8.
> 
> - The one in plpgsql_connect_single() sets the PgSQL client encoding
>    to UTF8, and if SNAPSHOT is not defined, leaves it at LATIN1
>    encoding which is what was used historically.
> 
> How do we avoid unexpected breakages in a stable release?
> 
> I suggest that we leave the encoding at LATIN1 when SMTPUTF8 is
> disabled. That is historical behavior and that minimizes the risk
> of breakages.
> 
> We can then use the UTF8 encoding when SMTPUTF8 is enabled. It is
> the only encoding that makes sense with SMTPUTF8.

Sure, but there is some small risk that a site that enables UTF8
nevertheless is currently using a database with a server-side encoding
of LATIN1 and somehow avoiding actual use of UTF8 lookup keys.

This risk is probably quite modest.  Introducing a dictionary
configuration keyword is fairly simple, with the proposed behaviour as a
default, but then also a mechanism to disable UTF8 if desired.

--- src/global/dict_pgsql.c
+++ src/global/dict_pgsql.c
@@ -207,6 +207,7 @@ typedef struct {
     char   *result_format;
     void   *ctx;
     int     expansion_limit;
+    int     utf8_enable;
     char   *username;
     char   *password;
     char   *dbname;
@@ -722,6 +723,7 @@ static void pgsql_parse_config(DICT_PGSQL *dict_pgsql, 
const char *pgsqlcf)
     dict_pgsql->password = cfg_get_str(p, "password", "", 0, 0);
     dict_pgsql->dbname = cfg_get_str(p, "dbname", "", 1, 0);
     dict_pgsql->result_format = cfg_get_str(p, "result_format", "%s", 1, 0);
+    dict_pgsql->utf8_enable = cfg_get_bool(p, "utf8_enable", 1);
 
     /*
      * XXX: The default should be non-zero for safety, but that is not

The new dictionary parameter can then be used a guard to conditionally
suppress use of the UTF8 encoding in the PgSQL driver.  But if that's a
bridge too far for a stable release, so be it.

(Of course the documentation would then have to describe the parater as
applicable from a certain patch level and up, IIRC there were some
precedents for this sort of emergency UI expansion, though none
recently).

-- 
    Viktor.

Reply via email to