Horton

Thread

Commits

  1. Clear conn->errorMessage at successful completion of PQconnectdb().

  2. Avoid ECPG test failures in some GSS-capable environments.

  3. Try next host after a "cannot connect now" failure.

  4. Uniformly identify the target host in libpq connection failure reports.

  5. Allow pg_regress.c wrappers to postprocess test result files.

  6. In libpq, always append new error messages to conn->errorMessage.

  1. Multiple hosts in connection string failed to failover in non-hot standby mode

    Hubert Zhang <zhubert@vmware.com> — 2020-10-27T07:14:14Z

    Hi hackers,

Libpq has supported to specify multiple hosts in connection string and enable auto failover when the previous PostgreSQL instance cannot be accessed.
But when I tried to enable this feature for a non-hot standby, it cannot do the failover with the following messages.

psql: error: could not connect to server: FATAL:  the database system is starting up

Document says ' If a connection is established successfully, but authentication fails, the remaining hosts in the list are not tried.'
I'm wondering is it a feature by design or a bug? If it's a bug, I plan to fix it.

Thanks,
Hubert Zhang

  2. RE: Multiple hosts in connection string failed to failover in non-hot standby mode

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-10-27T09:30:36Z

    Please send emails in text format.  Your email was in HTML, and I changed this reply to text format.


From: Hubert Zhang <zhubert@vmware.com> 
> Libpq has supported to specify multiple hosts in connection string and enable auto failover when the previous PostgreSQL instance cannot be accessed.
> But when I tried to enable this feature for a non-hot standby, it cannot do the failover with the following messages.
> 
> psql: error: could not connect to server: FATAL:  the database system is starting up

Was the primary running and accepting connections when you encountered this error?  That is, if you specified host="host1 host2", host1 was the non-hot standby and host2 was a running primary?  Or only the non-hot standby was running?

If a primary was running, I'd say it's a bug...  Perhaps the following part in libpq gives up connection attempts wen the above FATAL error is returned from the server.  Maybe libpq should differentiate errors using SQLSTATE and continue connection attempts on other hosts.

[fe-connect.c]
                /* Handle errors. */
                if (beresp == 'E')
                {
                    if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
...
#endif

                    goto error_return;
                }

                /* It is an authentication request. */
                conn->auth_req_received = true;

                /* Get the type of request. */


Regards
Takayuki Tsunakawa

  3. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Noah Misch <noah@leadboat.com> — 2020-10-28T10:37:34Z

    On Tue, Oct 27, 2020 at 07:14:14AM +0000, Hubert Zhang wrote:
> Libpq has supported to specify multiple hosts in connection string and enable auto failover when the previous PostgreSQL instance cannot be accessed.
> But when I tried to enable this feature for a non-hot standby, it cannot do the failover with the following messages.
> 
> psql: error: could not connect to server: FATAL:  the database system is starting up
> 
> Document says ' If a connection is established successfully, but authentication fails, the remaining hosts in the list are not tried.'
> I'm wondering is it a feature by design or a bug? If it's a bug, I plan to fix it.

I felt it was a bug, but the community as a whole may or may not agree:
https://postgr.es/m/flat/16508-1a63222835164566%40postgresql.org

  4. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Hubert Zhang <zhubert@vmware.com> — 2020-10-28T10:41:50Z

    Was the primary running and accepting connections when you encountered this error?  That is, if you specified host="host1 host2", host1 was the non-hot standby and host2 was a running primary?  Or only the non-hot standby was running?

If a primary was running, I'd say it's a bug...  Perhaps the following part in libpq gives up connection attempts wen the above FATAL error is returned from the server.  Maybe libpq should differentiate errors using SQLSTATE and continue connection attempts on other hosts.
Yes, the primary was running, but non-hot standby is in front of the primary in connection string.
Hao Wu and I wrote a patch to fix this problem. Client side libpq should try another hosts in connection string when it is rejected by a non-hot standby, or the first host encounter some n/w problems during the libpq handshake.

Please send emails in text format.  Your email was in HTML, and I changed this reply to text format.
Thanks. Is this email in text format now? I just use outlook in chrome. Let me know if it still in html format.

Hubert & Hao Wu

________________________________
From: tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com>
Sent: Tuesday, October 27, 2020 5:30 PM
To: Hubert Zhang <zhubert@vmware.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: RE: Multiple hosts in connection string failed to failover in non-hot standby mode

Please send emails in text format.  Your email was in HTML, and I changed this reply to text format.


From: Hubert Zhang <zhubert@vmware.com>
> Libpq has supported to specify multiple hosts in connection string and enable auto failover when the previous PostgreSQL instance cannot be accessed.
> But when I tried to enable this feature for a non-hot standby, it cannot do the failover with the following messages.
>
> psql: error: could not connect to server: FATAL:  the database system is starting up

Was the primary running and accepting connections when you encountered this error?  That is, if you specified host="host1 host2", host1 was the non-hot standby and host2 was a running primary?  Or only the non-hot standby was running?

If a primary was running, I'd say it's a bug...  Perhaps the following part in libpq gives up connection attempts wen the above FATAL error is returned from the server.  Maybe libpq should differentiate errors using SQLSTATE and continue connection attempts on other hosts.

[fe-connect.c]
                /* Handle errors. */
                if (beresp == 'E')
                {
                    if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
...
#endif

                    goto error_return;
                }

                /* It is an authentication request. */
                conn->auth_req_received = true;

                /* Get the type of request. */


Regards
Takayuki Tsunakawa

  5. RE: Multiple hosts in connection string failed to failover in non-hot standby mode

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-10-28T10:59:28Z

    From: Hubert Zhang <zhubert@vmware.com> 
> Hao Wu and I wrote a patch to fix this problem. Client side libpq should try another hosts in connection string when it is rejected by a non-hot standby, or the first host encounter some n/w problems during the libpq handshake.

Thank you.  Please add it to the November Commitfest.


> Thanks. Is this email in text format now? I just use outlook in chrome. Let me know if it still in html format.

I'm afraid not.  The Outlook's title bar says that it's in HTML format.  I'm using Outlook 2016 client app on Windows 10.  I may have failed to convert my previous email to text, but it should be text format this time.


Regards
Takayuki Tsunakawa

  6. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-07T21:50:06Z

    Hubert Zhang <zhubert@vmware.com> writes:
> [ 0001-Enhance-libpq-to-support-multiple-host-for-non-hot-s.patch ]

I took a quick look at this.  TBH, I'd just drop the first three hunks,
as they've got nothing to do with any failure mode that there's evidence
for in this thread or the prior one, and I'm afraid they're more likely
to create trouble than fix it.

As for the last hunk, why is it after rather than before the SSL/GSS
checks?  I doubt that retrying with/without SSL is going to change
a CANNOT_CONNECT_NOW result, unless maybe by slowing things down to
the point where recovery has finished ;-)

The bigger picture though is

(1) what set of failures should we retry on?  I think CANNOT_CONNECT_NOW
is reasonable, but are there others?

(2) what does this do to the quality of the error messages in cases
where all the connection attempts fail?

I think that error message quality was not thought too much about
in the original development of the multi-host feature, so to some
extent I'm asking you to clean up someone else's mess.  Nonetheless,
I feel that we do need to clean it up before we do things that risk
making it even more confusing.

The problems that I see in this area are first that there's no
real standardization in libpq as to whether to append error messages
together or just flush preceding messages; and second that no effort
is made in multi-connection-attempt cases to separate the errors from
different attempts, much less identify which error goes with which
host or IP address.  I think we really need to put some work into
that.  In some cases you can infer what happened from breadcrumbs
we already put into the text, for example

$ psql -h localhost,/tmp -p 12345
psql: error: could not connect to server: Connection refused
        Is the server running on host "localhost" (::1) and accepting
        TCP/IP connections on port 12345?
could not connect to server: Connection refused
        Is the server running on host "localhost" (127.0.0.1) and accepting
        TCP/IP connections on port 12345?
could not connect to server: No such file or directory
        Is the server running locally and accepting
        connections on Unix domain socket "/tmp/.s.PGSQL.12345"?

but this doesn't seem particularly helpfully laid out to me, and we don't
provide the breadcrumbs at all for a lot of other error cases.

I'm vaguely imagining that we could do something more like

could not connect to host "localhost" (::1), port 12345: Connection refused
could not connect to host "localhost" (127.0.0.1), port 12345: Connection refused
could not connect to socket "/tmp/.s.PGSQL.12345": No such file or directory

Not quite sure if the "Is the server running" hint is worth preserving.
We'd have to reword it quite a bit, and it'd be very duplicative.

The implementation of this might involve sticking the initial string
(up to the colon, in this example) into conn->errorMessage speculatively
as we try each host.  If we then append an error to it and go around
again, we're good.  If we successfully connect, then the contents of
conn->errorMessage don't matter.

			regards, tom lane

  7. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-10T22:38:50Z

    I wrote:
> The problems that I see in this area are first that there's no
> real standardization in libpq as to whether to append error messages
> together or just flush preceding messages; and second that no effort
> is made in multi-connection-attempt cases to separate the errors from
> different attempts, much less identify which error goes with which
> host or IP address.  I think we really need to put some work into
> that.

I spent some time on this, and here is a patch set that tries to
improve matters.

0001 changes the libpq coding rules to be that all error messages should
be appended to conn->errorMessage, never overwritten (there are a couple
of exceptions in fe-lobj.c) and we reset conn->errorMessage to empty only
at the start of a connection request or new query.  This is something
that's been bugging me for a long time and I'm glad to get it cleaned up.
Formerly it seemed that a dartboard had been used to decide whether to use
"printfPQExpBuffer" or "appendPQExpBuffer"; now it's always the latter.
We can also get rid of several hacks that were used to get around the
mess and force appending behavior.

0002 then changes the reporting rules in fe-connect.c as I suggested,
so that you might get errors like this:

$ psql -h localhost,/tmp -p 12345
psql: error: could not connect to host "localhost" (::1), port 12345: Connection refused
        Is the server running on that host and accepting TCP/IP connections?
could not connect to host "localhost" (127.0.0.1), port 12345: Connection refused
        Is the server running on that host and accepting TCP/IP connections?
could not connect to socket "/tmp/.s.PGSQL.12345": No such file or directory
        Is the server running locally and accepting connections on that socket?

and we have a pretty uniform rule that errors coming back from a
connection attempt will be prefixed with the server address.

Then 0003 is the part of your patch that I'm happy with.  Given 0001+0002
we could certainly consider looping back and retrying for more cases, but
I still want to tread lightly on that.  I don't see a lot of value in
retrying errors that seem to be on the client side, such as failure to
set socket properties; and in general I'm hesitant to add untestable
code paths here.

I feel pretty good about 0001: it might be committable as-is.  0002 is
probably subject to bikeshedding, plus it has a problem in the ECPG tests.
Two of the error messages are now unstable because they expose
chosen-at-random socket paths:

diff -U3 /home/postgres/pgsql/src/interfaces/ecpg/test/expected/connect-test5.stderr /home/postgres/pgsql/src/interfaces/ecpg/test/results/connect-test5.stderr
--- /home/postgres/pgsql/src/interfaces/ecpg/test/expected/connect-test5.stderr 2020-08-04 14:59:45.617380050 -0400
+++ /home/postgres/pgsql/src/interfaces/ecpg/test/results/connect-test5.stderr  2021-01-10 16:12:22.539433702 -0500
@@ -36,7 +36,7 @@
 [NO_PID]: sqlca: code: 0, state: 00000
 [NO_PID]: ECPGconnect: opening database <DEFAULT> on <DEFAULT> port <DEFAULT>  for user regress_ecpg_user2
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: ECPGconnect: could not open database: FATAL:  database "regress_ecpg_user2" does not exist
+[NO_PID]: ECPGconnect: could not open database: could not connect to socket "/tmp/pg_regress-EbHubF/.s.PGSQL.58080": FATAL:  database "regress_ecpg_user2" does not exist
 
 [NO_PID]: sqlca: code: 0, state: 00000
 [NO_PID]: ecpg_finish: connection main closed
@@ -73,7 +73,7 @@
 [NO_PID]: sqlca: code: -220, state: 08003
 [NO_PID]: ECPGconnect: opening database <DEFAULT> on <DEFAULT> port <DEFAULT>  for user regress_ecpg_user2
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: ECPGconnect: could not open database: FATAL:  database "regress_ecpg_user2" does not exist
+[NO_PID]: ECPGconnect: could not open database: could not connect to socket "/tmp/pg_regress-EbHubF/.s.PGSQL.58080": FATAL:  database "regress_ecpg_user2" does not exist
 
 [NO_PID]: sqlca: code: 0, state: 00000
 [NO_PID]: ecpg_finish: connection main closed

I don't have any non-hacky ideas what to do about that.  The extra detail
seems useful to end users, but we don't have any infrastructure that
would allow filtering it out in the ECPG tests.

			regards, tom lane

  8. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-11T02:56:34Z

    I wrote:
> I feel pretty good about 0001: it might be committable as-is.  0002 is
> probably subject to bikeshedding, plus it has a problem in the ECPG tests.
> Two of the error messages are now unstable because they expose
> chosen-at-random socket paths:
> ...
> I don't have any non-hacky ideas what to do about that.  The extra detail
> seems useful to end users, but we don't have any infrastructure that
> would allow filtering it out in the ECPG tests.

So far the only solution that comes to mind is to introduce some
infrastructure to do that filtering.  0001-0003 below are unchanged,
0004 patches up the ecpg test framework with a rather ad-hoc filtering
function.  I'd feel worse about this if there weren't already a very
ad-hoc filtering function there ;-)

This set passes check-world for me; we'll soon see what the cfbot
thinks.

			regards, tom lane

  9. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Hubert Zhang <zhubert@vmware.com> — 2021-01-11T14:31:40Z

    Hi Tom,

I agree to get detailed error message for each failed host as your patch 0001.

As for patch 0004, find ':' after "could not connect to" may failed when error message like:
"could not connect to host "localhost" (::1), port 12345: Connection refused", where p2 will point to "::1" instead of ": Connection refused". But since it's only used for test case, we don't need to filter the error message precisely.

```
ecpg_filter_stderr(const char *resultfile, const char *tmpfile)
{
    ......
    char       *p1 = strstr(linebuf.data, "could not connect to ");
    if (p1)
    {
        char       *p2 = strchr(p1, ':');
        if (p2)
            memmove(p1 + 17, p2, strlen(p2) + 1);
    }
}
```

Thanks,
Hubert

________________________________
From: Tom Lane <tgl@sss.pgh.pa.us>
Sent: Monday, January 11, 2021 10:56 AM
To: Hubert Zhang <zhubert@vmware.com>
Cc: tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Multiple hosts in connection string failed to failover in non-hot standby mode

I wrote:
> I feel pretty good about 0001: it might be committable as-is.  0002 is
> probably subject to bikeshedding, plus it has a problem in the ECPG tests.
> Two of the error messages are now unstable because they expose
> chosen-at-random socket paths:
> ...
> I don't have any non-hacky ideas what to do about that.  The extra detail
> seems useful to end users, but we don't have any infrastructure that
> would allow filtering it out in the ECPG tests.

So far the only solution that comes to mind is to introduce some
infrastructure to do that filtering.  0001-0003 below are unchanged,
0004 patches up the ecpg test framework with a rather ad-hoc filtering
function.  I'd feel worse about this if there weren't already a very
ad-hoc filtering function there ;-)

This set passes check-world for me; we'll soon see what the cfbot
thinks.

                        regards, tom lane

  10. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-11T15:15:52Z

    Hubert Zhang <zhubert@vmware.com> writes:
> As for patch 0004, find ':' after "could not connect to" may failed when error message like:
> "could not connect to host "localhost" (::1), port 12345: Connection refused", where p2 will point to "::1" instead of ": Connection refused". But since it's only used for test case, we don't need to filter the error message precisely.

Excellent point, and I think that could happen on a Windows installation.
We can make it look for ": " instead of just ':', and that'll reduce the
odds of trouble.

			regards, tom lane

  11. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-06T16:26:51Z

    On Sun, Jan 10, 2021 at 05:38:50PM -0500, Tom Lane wrote:
> I wrote:
> > The problems that I see in this area are first that there's no
> > real standardization in libpq as to whether to append error messages
> > together or just flush preceding messages; and second that no effort
> > is made in multi-connection-attempt cases to separate the errors from
> > different attempts, much less identify which error goes with which
> > host or IP address.  I think we really need to put some work into
> > that.
> 
> I spent some time on this, and here is a patch set that tries to
> improve matters.
> 
> 0001 changes the libpq coding rules to be that all error messages should
> be appended to conn->errorMessage, never overwritten (there are a couple
> of exceptions in fe-lobj.c) and we reset conn->errorMessage to empty only
> at the start of a connection request or new query.  This is something
> that's been bugging me for a long time and I'm glad to get it cleaned up.
> Formerly it seemed that a dartboard had been used to decide whether to use
> "printfPQExpBuffer" or "appendPQExpBuffer"; now it's always the latter.
> We can also get rid of several hacks that were used to get around the
> mess and force appending behavior.
> 
> 0002 then changes the reporting rules in fe-connect.c as I suggested,
> so that you might get errors like this:
> 
> $ psql -h localhost,/tmp -p 12345
> psql: error: could not connect to host "localhost" (::1), port 12345: Connection refused
>         Is the server running on that host and accepting TCP/IP connections?
> could not connect to host "localhost" (127.0.0.1), port 12345: Connection refused
>         Is the server running on that host and accepting TCP/IP connections?
> could not connect to socket "/tmp/.s.PGSQL.12345": No such file or directory
>         Is the server running locally and accepting connections on that socket?
> 
> and we have a pretty uniform rule that errors coming back from a
> connection attempt will be prefixed with the server address.

52a10224 broke sqlsmith, of all things.

It was using errmsg as a test of success, instead of checking if the connection
result wasn't null:

     conn = PQconnectdb(conninfo.c_str());
     char *errmsg = PQerrorMessage(conn);
     if (strlen(errmsg))
         throw dut::broken(errmsg, "08001");

That's clearly the wrong thing to do, but maybe this should be described in the
release notes as a compatibility issue, in case other people had the same idea.
Clearing errorMessage during success is an option..

-- 
Justin

  12. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-06T17:22:27Z

    Justin Pryzby <pryzby@telsasoft.com> writes:
> 52a10224 broke sqlsmith, of all things.

> It was using errmsg as a test of success, instead of checking if the connection
> result wasn't null:

>      conn = PQconnectdb(conninfo.c_str());
>      char *errmsg = PQerrorMessage(conn);
>      if (strlen(errmsg))
>          throw dut::broken(errmsg, "08001");

> That's clearly the wrong thing to do, but maybe this should be described in the
> release notes as a compatibility issue, in case other people had the same idea.
> Clearing errorMessage during success is an option..

Hm.  I'd debated whether to clear conn->errorMessage at the end of
a successful connection sequence, and decided not to on the grounds
that it might be interesting info (eg it could tell you why you
ended up connected to server Y and not server X).  But perhaps
it's too much of a compatibility break for this small benefit.

I'm curious though why it took this long for anyone to complain.
I'd supposed that people were running sqlsmith against HEAD on
a pretty regular basis.

			regards, tom lane

  13. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-06T17:38:13Z

    On Thu, May 06, 2021 at 01:22:27PM -0400, Tom Lane wrote:
> I'm curious though why it took this long for anyone to complain.
> I'd supposed that people were running sqlsmith against HEAD on
> a pretty regular basis.

I think it's also becase sqlsmith would need to run against the v14 *client*
library.  I don't know about anyone else's workflow, but I tend not to "make
install", but work with binaries out of ./tmp_install.

There's a few changes needed on sqlsmith HEAD, but I guess nobody would have
gotten that far if the connection was failing (or rather, detected as such).

diff --git a/grammar.cc b/grammar.cc
index 62aa8e9..76491ff 100644
--- a/grammar.cc
+++ b/grammar.cc
@@ -327,7 +327,11 @@ query_spec::query_spec(prod *p, struct scope *s, bool lateral) :
 
   search = bool_expr::factory(this);
 
-  if (d6() > 2) {
+  if (d6() > 4) {
+    ostringstream cons;
+    cons << "order by 1 fetch first " << d100() + d100() << " rows with ties";
+    limit_clause = cons.str();
+  } else if (d6() > 2) {
     ostringstream cons;
     cons << "limit " << d100() + d100();
     limit_clause = cons.str();
diff --git a/postgres.cc b/postgres.cc
index f2a3627..1c0c55f 100644
--- a/postgres.cc
+++ b/postgres.cc
@@ -30,6 +30,7 @@ bool pg_type::consistent(sqltype *rvalue)
   case 'c': /* composite type */
   case 'd': /* domain */
   case 'r': /* range */
+  case 'm': /* multirange */
   case 'e': /* enum */
     return this == t;

  14. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Andreas Seltenreich <seltenreich@gmx.de> — 2021-05-06T20:24:21Z

    > On Thu, May 06, 2021 at 01:22:27PM -0400, Tom Lane wrote:
>> I'm curious though why it took this long for anyone to complain.
>> I'd supposed that people were running sqlsmith against HEAD on
>> a pretty regular basis.

Last time I ran it was November 27.  I'm neglecting it on my spare time
and there is hardly any opportunity to sneak it onto my agenda at work.
I'll do my best to try to get either of these fixed.

Justin Pryzby writes:
> I think it's also becase sqlsmith would need to run against the v14 *client*
> library.  I don't know about anyone else's workflow, but I tend not to "make
> install", but work with binaries out of ./tmp_install.

My playbooks don't grab the client libraries of the test target either.
I'll change them.

> There's a few changes needed on sqlsmith HEAD, but I guess nobody would have
> gotten that far if the connection was failing (or rather, detected as such).

Thanks for the patch.

regards,
andreas

  15. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-05-11T07:29:28Z

    On Thu, May 06, 2021 at 01:22:27PM -0400, Tom Lane wrote:
> Hm.  I'd debated whether to clear conn->errorMessage at the end of
> a successful connection sequence, and decided not to on the grounds
> that it might be interesting info (eg it could tell you why you
> ended up connected to server Y and not server X).  But perhaps
> it's too much of a compatibility break for this small benefit.
> 
> I'm curious though why it took this long for anyone to complain.
> I'd supposed that people were running sqlsmith against HEAD on
> a pretty regular basis.

FWIW, I think that the case of getting some information about any
failed connections while a connection has been successfully made
within the scope of the connection string parameters provided by the
user is rather thin, and I really feel that this is going to cause
more pain to users than this is worth it.  So my vote would be to
clean up conn->errorMessage after a successful connection.

Now, I would not mind either if we finish by taking a decision here
after beta1, to see if there are actual complains on the matter based
on the feedback we get.
--
Michael

  16. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Daniel Gustafsson <daniel@yesql.se> — 2021-05-17T11:06:51Z

    > On 11 May 2021, at 09:29, Michael Paquier <michael@paquier.xyz> wrote:

> FWIW, I think that the case of getting some information about any
> failed connections while a connection has been successfully made
> within the scope of the connection string parameters provided by the
> user is rather thin, and I really feel that this is going to cause
> more pain to users than this is worth it.  So my vote would be to
> clean up conn->errorMessage after a successful connection.

Agreed, given how conservative we typically are with backwards compatibility it
seems a too thin benefit to warrant potential breakage.

My vote would too be to restore the behavior by clearing conn->errorMessage.

--
Daniel Gustafsson		https://vmware.com/

  17. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-31T01:25:00Z

    On Thu, May 06, 2021 at 01:22:27PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > 52a10224 broke sqlsmith, of all things.
> 
> > It was using errmsg as a test of success, instead of checking if the connection
> > result wasn't null:
> 
> >      conn = PQconnectdb(conninfo.c_str());
> >      char *errmsg = PQerrorMessage(conn);
> >      if (strlen(errmsg))
> >          throw dut::broken(errmsg, "08001");
> 
> > That's clearly the wrong thing to do, but maybe this should be described in the
> > release notes as a compatibility issue, in case other people had the same idea.
> > Clearing errorMessage during success is an option..
> 
> Hm.  I'd debated whether to clear conn->errorMessage at the end of
> a successful connection sequence, and decided not to on the grounds
> that it might be interesting info (eg it could tell you why you
> ended up connected to server Y and not server X).  But perhaps
> it's too much of a compatibility break for this small benefit.

I don't care if applications break because they check the errorMessage instead
of the return value...

..But I think it's not useful to put details into errorMessage on success,
unless you're going to document that.  It would never have occurred to me to
look there, or that it would even be safe.

-- 
Justin

  18. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-05-31T02:00:55Z

    On Sun, May 30, 2021 at 08:25:00PM -0500, Justin Pryzby wrote:
> ..But I think it's not useful to put details into errorMessage on success,
> unless you're going to document that.  It would never have occurred to me to
> look there, or that it would even be safe.

Yeah.  On the contrary, it could be confusing if one sees an error
message but there is nothing to worry about, because things are
working in the scope of what the user wanted at connection time.
--
Michael

  19. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-05-31T03:33:51Z

    On Mon, May 31, 2021 at 11:00:55AM +0900, Michael Paquier wrote:
> Yeah.  On the contrary, it could be confusing if one sees an error
> message but there is nothing to worry about, because things are
> working in the scope of what the user wanted at connection time.

In my recent quest to look at GSSAPI builds on Windows, I have bumped
into another failure that's related to this thread.  hamerkop
summarizes the situation here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2021-05-29%2010%3A15%3A42

There are two failures like this one as errorMessage piles up on
failures, as of connect/test5:
-[NO_PID]: ECPGconnect: connection to server failed: FATAL:  database
 "regress_ecpg_user2" does not exist
+[NO_PID]: ECPGconnect: connection to server failed: could not
 initiate GSSAPI security context: Unspecified GSS failure.  Minor
 code may provide more information: Credential cache is empty
+connection to server failed: FATAL:  database "regress_ecpg_user2"
 does not exist 
--
Michael

  20. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-31T04:05:12Z

    Michael Paquier <michael@paquier.xyz> writes:
> In my recent quest to look at GSSAPI builds on Windows, I have bumped
> into another failure that's related to this thread.  hamerkop
> summarizes the situation here:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2021-05-29%2010%3A15%3A42
> There are two failures like this one as errorMessage piles up on
> failures, as of connect/test5:
> -[NO_PID]: ECPGconnect: connection to server failed: FATAL:  database
>  "regress_ecpg_user2" does not exist
> +[NO_PID]: ECPGconnect: connection to server failed: could not
>  initiate GSSAPI security context: Unspecified GSS failure.  Minor
>  code may provide more information: Credential cache is empty
> +connection to server failed: FATAL:  database "regress_ecpg_user2"
>  does not exist 

Yeah, I was looking at that earlier today.  Evidently libpq is
trying a GSS-encrypted connection, and that doesn't work, so
it falls back to a regular connection where we get the expected
error.  Probably all the connections in this test are hitting the
GSS failure, but only the ones where the second attempt fails
show a visible issue.

What is not clear is why GSS is acting that way.  We wouldn't
have tried a GSS connection unless pg_GSS_have_cred_cache
succeeded ... so how come that worked but then gss_init_sec_context
complained "Credential cache is empty"?

My rough guess is that Windows has implemented the GSS APIs in
such a way that what pg_GSS_have_cred_cache is testing isn't
sufficient to tell whether a sane credential actually exists.

Or there's something particularly weird about how hamerkop
is set up.

			regards, tom lane

  21. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-05-31T06:31:08Z

    On Mon, May 31, 2021 at 12:05:12AM -0400, Tom Lane wrote:
> Yeah, I was looking at that earlier today.  Evidently libpq is
> trying a GSS-encrypted connection, and that doesn't work, so
> it falls back to a regular connection where we get the expected
> error.  Probably all the connections in this test are hitting the
> GSS failure, but only the ones where the second attempt fails
> show a visible issue.

Yep.  This wastes cycles.

> What is not clear is why GSS is acting that way.  We wouldn't
> have tried a GSS connection unless pg_GSS_have_cred_cache
> succeeded ... so how come that worked but then gss_init_sec_context
> complained "Credential cache is empty"?
> 
> My rough guess is that Windows has implemented the GSS APIs in
> such a way that what pg_GSS_have_cred_cache is testing isn't
> sufficient to tell whether a sane credential actually exists.
> 
> Or there's something particularly weird about how hamerkop
> is set up.

I suspect that's just the way the upstream installation works with a
credentials cache created from the beginning, as I see the same
behavior and the same error on my own host for HEAD with a KRB5 server
set up once the upstream installation runs.  Leaving the specific
topic of this thread aside for one moment, would there be an argument
for just enforcing gssencmode=disable in this set of tests down to 12?
It is worth noting that the problem does not show up in 12 and 13 once
the compilation works, because we just mask the error there, but the
code path is still taken.

Another thing that strikes me as incorrect is that we don't unset
PGGSSENCMODE or PGGSSLIB in TestLib.pm.  Just noting it on the way..
--
Michael

  22. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-31T13:36:20Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Mon, May 31, 2021 at 12:05:12AM -0400, Tom Lane wrote:
>> What is not clear is why GSS is acting that way.  We wouldn't
>> have tried a GSS connection unless pg_GSS_have_cred_cache
>> succeeded ... so how come that worked but then gss_init_sec_context
>> complained "Credential cache is empty"?

> I suspect that's just the way the upstream installation works with a
> credentials cache created from the beginning, as I see the same
> behavior and the same error on my own host for HEAD with a KRB5 server
> set up once the upstream installation runs.

Interesting --- I was considering running such a test locally, but
didn't get round to it yet.

> Leaving the specific
> topic of this thread aside for one moment, would there be an argument
> for just enforcing gssencmode=disable in this set of tests down to 12?

It seems like the ideal solution would be to make pg_GSS_have_cred_cache
smarter, so that we don't attempt a GSS connection cycle here.  But if
we can't, adding gssencmode=disable to these test cases is what I was
thinking about, too.

> Another thing that strikes me as incorrect is that we don't unset
> PGGSSENCMODE or PGGSSLIB in TestLib.pm.  Just noting it on the way..

Agreed, that seems bogus.

			regards, tom lane

  23. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-06-01T00:56:47Z

    On Mon, May 31, 2021 at 09:36:20AM -0400, Tom Lane wrote:
> Interesting --- I was considering running such a test locally, but
> didn't get round to it yet.

Just to be clear, that's my Windows dev box.

> It seems like the ideal solution would be to make pg_GSS_have_cred_cache
> smarter, so that we don't attempt a GSS connection cycle here.

I am not sure yet what would be adapted here.  That requires diving a
bit into the upstream code.

>> Another thing that strikes me as incorrect is that we don't unset
>> PGGSSENCMODE or PGGSSLIB in TestLib.pm.  Just noting it on the way..
>
> Agreed, that seems bogus.

There may be others, and I have not checked yet.  I'd rather do a
backpatch for this part, would you agree?
--
Michael

  24. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-01T01:07:38Z

    Michael Paquier <michael@paquier.xyz> writes:
>>> Another thing that strikes me as incorrect is that we don't unset
>>> PGGSSENCMODE or PGGSSLIB in TestLib.pm.  Just noting it on the way..

>> Agreed, that seems bogus.

> There may be others, and I have not checked yet.  I'd rather do a
> backpatch for this part, would you agree?

+1

			regards, tom lane

  25. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-06-01T07:34:44Z

    On Mon, May 31, 2021 at 09:07:38PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>>> Agreed, that seems bogus.
> 
>> There may be others, and I have not checked yet.  I'd rather do a
>> backpatch for this part, would you agree?
> 
> +1

Playing with all those variables and broken values here and there, I
have been able to break a bunch of tests.  Most of the failures were
in the authentication and SSL tests, but there were also fancier
cases.  For example, PGCLIENTENCODING would cause a failure with
pg_ctl, for any TAP test.

I got surprised that enforcing values for most of the PGSSL* ones did
not cause a failure when it came to the certs, CRLs keys and root
certs now.  Still, I think that we'd be safer to cancel these as
well.

Attached is the list I am finishing with.  I'd like to fix that, so
please let me know if there are any comments or objections.
--
Michael

  26. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-06T21:27:49Z

    It seems like nobody's terribly interested in figuring out why
pg_GSS_have_cred_cache() is misbehaving on Windows.  So I took
a look at disabling GSSENC in these test cases to try to silence
hamerkop's test failure that way.  Here's a proposed patch.
It relies on setenv() being available, but I think that's fine
because we link the ECPG test programs with libpgport.

			regards, tom lane

  27. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-06-07T06:43:42Z

    On Sun, Jun 06, 2021 at 05:27:49PM -0400, Tom Lane wrote:
> It seems like nobody's terribly interested in figuring out why
> pg_GSS_have_cred_cache() is misbehaving on Windows.

I have been investigating that for a couple of hours in total, but
nothing to report yet.

> So I took
> a look at disabling GSSENC in these test cases to try to silence
> hamerkop's test failure that way.  Here's a proposed patch.
> It relies on setenv() being available, but I think that's fine
> because we link the ECPG test programs with libpgport.

No, that's not it.  The compilation of the tests happens when
triggering the tests as of ecpgcheck() in vcregress.pl so I think that
this is going to fail.  This requires at least the addition of a
reference to libpgport in ecpg_regression.proj, perhaps more.
--
Michael

  28. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-07T14:38:03Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Jun 06, 2021 at 05:27:49PM -0400, Tom Lane wrote:
>> So I took
>> a look at disabling GSSENC in these test cases to try to silence
>> hamerkop's test failure that way.  Here's a proposed patch.
>> It relies on setenv() being available, but I think that's fine
>> because we link the ECPG test programs with libpgport.

> No, that's not it.  The compilation of the tests happens when
> triggering the tests as of ecpgcheck() in vcregress.pl so I think that
> this is going to fail.  This requires at least the addition of a
> reference to libpgport in ecpg_regression.proj, perhaps more.

Hmm.  We do include "-lpgcommon -lpgport" when building the ecpg test
programs on Unix, so I'd assumed that the MSVC scripts did the same.
Is there a good reason not to make them do so?

			regards, tom lane

  29. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-06-08T03:24:31Z

    On Mon, Jun 07, 2021 at 10:38:03AM -0400, Tom Lane wrote:
> Hmm.  We do include "-lpgcommon -lpgport" when building the ecpg test
> programs on Unix, so I'd assumed that the MSVC scripts did the same.
> Is there a good reason not to make them do so?

I was looking at that this morning, and yes we need to add more
references here.  Actually, adding only libpgport.lib allows the
compilation and the tests to work, but I agree to add also
libpgcommon.lib so as we don't fall into the same compilation trap
again in the future.

Now, I also see that using pgwin32_setenv() instead of
src/port/setenv.c causes cl to be confused once we update
ecpg_regression.proj because it cannot find setenv().  Bringing the
question, why is it necessary to have both setenv.c and
pgwin32_setenv() on HEAD?  setenv.c should be enough once you have the
fallback implementation of putenv() available.

Attached is the patch I am finishing with, that also brings all this
stuff closer to what I did in 12 and 13 for hamerkop.  The failing
test is passing for me now with MSVC and GSSAPI builds.

Thoughts?
--
Michael

  30. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-08T15:21:34Z

    Michael Paquier <michael@paquier.xyz> writes:
> Now, I also see that using pgwin32_setenv() instead of
> src/port/setenv.c causes cl to be confused once we update
> ecpg_regression.proj because it cannot find setenv().  Bringing the
> question, why is it necessary to have both setenv.c and
> pgwin32_setenv() on HEAD?  setenv.c should be enough once you have the
> fallback implementation of putenv() available.

IIUC, what you are proposing to do is replace pgwin32_setenv with
src/port/setenv.c.  I don't think that's an improvement.  setenv.c
leaks memory on repeat calls, because it cannot know what
pgwin32_setenv knows about how putenv works on that platform.

It'd be okay to do it like that for the ECPG tests, perhaps,
because we don't really care about small leaks in those.
But I don't want it to happen across-the-board.

Thinking more, the real problem is that use of libpgport
goes hand-in-hand with #including port.h; it's not going
to work real well if you do one without the other.
And I don't think we want to include port.h in the ECPG
test programs, because those are trying to model the
environment that typical user applications see.

Alternatives seem to be

(1) allow just this one ECPG test to include port.h (or
probably c.h).  However, there's a whole other can of worms
there, which is that I wonder if we aren't doing it wrong
on the Unix side by linking libpgport when we shouldn't.
We've not been bit by that yet, but I wonder if it isn't
just a matter of time.  The MSVC build, by not linking
those libs in the first place, is really doing this the
correct way.

(2) Let pg_regress_ecpg.c pass down the environment setting.

(3) Don't try to use the environment variable for this
purpose.  I'd originally tried to change test5.pgc to just
specify gssmode=disable in-line, but that only works
nicely for one of the two failing cases.  The other one
is testing the case of a completely defaulted connection
target, so there's no place to add an option without
breaking the only unique aspect of that test case.

(2) is starting to seem attractive now that we've seen
the downsides of (1) and (3).

(BTW, I just noticed that regress.c is unsetenv'ing the
SSL connection environment variables, but not the GSS ones.
Seens like that needs work similar to 8279f68a1.)

			regards, tom lane

  31. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-06-09T01:52:10Z

    On Tue, Jun 08, 2021 at 11:21:34AM -0400, Tom Lane wrote:
> IIUC, what you are proposing to do is replace pgwin32_setenv with
> src/port/setenv.c.  I don't think that's an improvement.  setenv.c
> leaks memory on repeat calls, because it cannot know what
> pgwin32_setenv knows about how putenv works on that platform.

Is gaur the only animal that needs this file, by the way?

> (1) allow just this one ECPG test to include port.h (or
> probably c.h).  However, there's a whole other can of worms
> there, which is that I wonder if we aren't doing it wrong
> on the Unix side by linking libpgport when we shouldn't.
> We've not been bit by that yet, but I wonder if it isn't
> just a matter of time.  The MSVC build, by not linking
> those libs in the first place, is really doing this the
> correct way.

I don't really want to include this stuff in the ECPG tests just to
bypass an environment configuration.

> (2) Let pg_regress_ecpg.c pass down the environment setting.
> 
> (3) Don't try to use the environment variable for this
> purpose.  I'd originally tried to change test5.pgc to just
> specify gssmode=disable in-line, but that only works
> nicely for one of the two failing cases.  The other one
> is testing the case of a completely defaulted connection
> target, so there's no place to add an option without
> breaking the only unique aspect of that test case.

> (2) is starting to seem attractive now that we've seen
> the downsides of (1) and (3).

FWIW, I'd be rather in favor of doing (3) because this remains simple
just to take care of an edge case, even if that partially breaks the
promise to rely on a default connection.

(4) would be to revisit the decision to make libpq report all the
errors stored in its stack with multiple attempts.  That would bring
back the buildfarm to green at least, and we still need to take a
decision about that for 14 anyway as it involves a compatibility
breakage.  But I agree that we also should do something for 12~ for
those tests.

> (BTW, I just noticed that regress.c is unsetenv'ing the
> SSL connection environment variables, but not the GSS ones.
> Seens like that needs work similar to 8279f68a1.)

Yes, I saw that I was able to break things is many fancy ways when
working on 8279f68a1, but the list of parameters to reset needs to
diverge a bit compared to the TAP tests.
--
Michael

  32. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-09T02:42:08Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Jun 08, 2021 at 11:21:34AM -0400, Tom Lane wrote:
>> IIUC, what you are proposing to do is replace pgwin32_setenv with
>> src/port/setenv.c.  I don't think that's an improvement.  setenv.c
>> leaks memory on repeat calls, because it cannot know what
>> pgwin32_setenv knows about how putenv works on that platform.

> Is gaur the only animal that needs this file, by the way?

I think it is.  setenv has been in POSIX for awhile, so probably
only very old systems would need that.  (This is why I don't care
that much that setenv.c leaks memory.  But we can't start using it
on platforms where we *do* care about performance.)

>> (3) Don't try to use the environment variable for this
>> purpose.  I'd originally tried to change test5.pgc to just
>> specify gssmode=disable in-line, but that only works
>> nicely for one of the two failing cases.  The other one
>> is testing the case of a completely defaulted connection
>> target, so there's no place to add an option without
>> breaking the only unique aspect of that test case.

> FWIW, I'd be rather in favor of doing (3) because this remains simple
> just to take care of an edge case, even if that partially breaks the
> promise to rely on a default connection.

Yeah, it doesn't seem like we need to test that case all that
badly.  I'd be okay with dropping that test; or maybe we could
fix things so that the default case succeeds?

			regards, tom lane

  33. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-09T16:05:10Z

    I wrote:
> ...  I'd be okay with dropping that test; or maybe we could
> fix things so that the default case succeeds?

Here's a draft patch that renames regress_ecpg_user2 to ecpg2_regression,
which matches the name of one of the databases used, allowing the test
cases with defaulted database name to succeed.  That gets rid of one of
the problematic diffs.  As it stood, though, that meant that connect/test5
wasn't exercising the connection-failure code path at all, which didn't
seem like what we want.  So I adjusted the second place that had been
failing to again fail on no-such-database, and stuck in gssencmode=disable
so that we shouldn't get any test diff on hamerkop.

			regards, tom lane

  34. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-06-10T00:46:26Z

    On Wed, Jun 09, 2021 at 12:05:10PM -0400, Tom Lane wrote:
> Here's a draft patch that renames regress_ecpg_user2 to ecpg2_regression,
> which matches the name of one of the databases used, allowing the test
> cases with defaulted database name to succeed.  That gets rid of one of
> the problematic diffs.

Yeah, I agree that this does not matter much for this one, as we want
to stress the quotes and the grammar for the connections here, as
99a5619 implies.  It is good to check for the failure path as well, so
what you have here looks fine to me.

> As it stood, though, that meant that connect/test5
> wasn't exercising the connection-failure code path at all, which didn't
> seem like what we want.  So I adjusted the second place that had been
> failing to again fail on no-such-database, and stuck in gssencmode=disable
> so that we shouldn't get any test diff on hamerkop.

Using ecpg2_regression for the role goes a bit against the recent rule
to not create any role not suffixed by "regress_" as part of the
regression tests, but I am fine to live with that here.

The changes for test1 with MinGW look right, I have not been able to
test them.
--
Michael

  35. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-10T03:15:25Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jun 09, 2021 at 12:05:10PM -0400, Tom Lane wrote:
>> Here's a draft patch that renames regress_ecpg_user2 to ecpg2_regression,

> Using ecpg2_regression for the role goes a bit against the recent rule
> to not create any role not suffixed by "regress_" as part of the
> regression tests, but I am fine to live with that here.

Oh dear, I forgot to check that carefully.  I'd been thinking the rule was
that such names must *contain* "regress", but looking at user.c, it's
stricter:

#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
	if (strncmp(stmt->role, "regress_", 8) != 0)
		elog(WARNING, "roles created by regression test cases should have names starting with \"regress_\"");
#endif

Meanwhile, the rule for database names is:

#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
	if (IsUnderPostmaster && strstr(dbname, "regression") == NULL)
		elog(WARNING, "databases created by regression test cases should have names including \"regression\"");
#endif

So unless we want to relax one or both of those, we can't have a user
name that matches the database name.

Now I'm inclined to go back to the first-draft patch I had, which just
dropped the first problematic test case, and added gssencmode=disable
to the second one.

			regards, tom lane

  36. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-10T14:47:50Z

    I wrote:
> Now I'm inclined to go back to the first-draft patch I had, which just
> dropped the first problematic test case, and added gssencmode=disable
> to the second one.

Done that way.  If we figure out why the GSS code is acting strangely
on hamerkop, maybe this can be reverted --- but it seems like we already
spent more time than is justified looking for band-aids.

			regards, tom lane

  37. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-13T20:09:26Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sun, May 30, 2021 at 08:25:00PM -0500, Justin Pryzby wrote:
>> ..But I think it's not useful to put details into errorMessage on success,
>> unless you're going to document that.  It would never have occurred to me to
>> look there, or that it would even be safe.

> Yeah.  On the contrary, it could be confusing if one sees an error
> message but there is nothing to worry about, because things are
> working in the scope of what the user wanted at connection time.

I got around to looking at this issue today, and verified that only one
place needs to be changed, as attached.

Although I was initially thinking that maybe we should leave the code
as-is, I now agree that resetting errorMessage is a good idea, because
what tends to be in it at this point is something like

"connection to server on socket "/tmp/.s.PGSQL.5432" failed: "

(ie the string made by emitHostIdentityInfo).  Anybody who does
look at that is likely to be confused, because the connection
*didn't* fail.

There might be some value in my original idea of preserving a trace of
the servers we tried before succeeding.  But it would take additional
work to present it in a non-confusing way, and given the lack of any
field requests for that, I'm not excited about doing it right now.
(One could also argue that it ought to get tied into the PQtrace
facilities somehow, rather than being done in this ad-hoc way.)

			regards, tom lane

  38. Re: Multiple hosts in connection string failed to failover in non-hot standby mode

    Michael Paquier <michael@paquier.xyz> — 2021-09-14T00:38:26Z

    On Mon, Sep 13, 2021 at 04:09:26PM -0400, Tom Lane wrote:
> I got around to looking at this issue today, and verified that only one
> place needs to be changed, as attached.

Thanks!  This looks fine to me at quick glance.
--
Michael