Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Mon, Dec 5, 2016 at 1:59 PM, Mithun Cy  wrote:
> On Mon, Dec 5, 2016 at 11:23 PM, Robert Haas  wrote:
>>I think that you need a restoreErrorMessage call here:
>>/* Skip any remaining addresses for this host. */
>>conn->addr_cur = NULL;
>>if (conn->whichhost + 1 < conn->nconnhost)
>>{
>>conn->status = CONNECTION_NEEDED
>>restoreErrorMessage(conn, );
>>goto keep_going;
>>}
>
> Right after seeing transaction is read-only we have restored the saved
> message so I think we do not need one more restore there.
>   if (strncmp(val, "on", 2) == 0)
>   {
>   PQclear(res);
> + restoreErrorMessage(conn, );

D'oh!  You're correct, of course.

Committed without that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Mon, Dec 5, 2016 at 11:23 PM, Robert Haas  wrote:
>I think that you need a restoreErrorMessage call here:
>/* Skip any remaining addresses for this host. */
>conn->addr_cur = NULL;
>if (conn->whichhost + 1 < conn->nconnhost)
>{
>conn->status = CONNECTION_NEEDED
>restoreErrorMessage(conn, );
>goto keep_going;
>}

Right after seeing transaction is read-only we have restored the saved
message so I think we do not need one more restore there.
  if (strncmp(val, "on", 2) == 0)
  {
  PQclear(res);
+ restoreErrorMessage(conn, );

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Gavin Flower]

On 05/12/16 17:00, Mithun Cy wrote:
[...]
errorMessage even outside PQconnectPoll. But that seems not required. 
Attacting the new patch which fixes the same.


--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com 

[...]

Is that meant to be 'attaching' or 'attacking'???



MORE SERIOUSLY
thanks for your and others efforts for making pg great!


Cheers,
Gavin


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Sun, Dec 4, 2016 at 11:00 PM, Mithun Cy  wrote:
> On Fri, Dec 2, 2016 at 8:54 PM, Robert Haas  wrote:
>> Couldn't this just be a variable in PQconnectPoll(), instead of adding
>> a new structure member?
>
> I have fixed same with a local variable in PQconnectPoll, Initally I thought
> savedMessage should have same visiblitly as errorMessage so we can restore
> the errorMessage even outside PQconnectPoll. But that seems not required.
> Attacting the new patch which fixes the same.

I think that you need a restoreErrorMessage call here:

/* Skip any remaining addresses for this host. */
conn->addr_cur = NULL;
if (conn->whichhost + 1 < conn->nconnhost)
{
conn->status = CONNECTION_NEEDED;
restoreErrorMessage(conn, );
goto keep_going;
}

Updated patch attached with that change, the removal of a useless
hunk, and the removal of "inline" from the new functions, which seems
like second-guessing of whatever decision the compiler chooses to
make.

Thoughts?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 3b9b263..24fc12d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1762,6 +1762,39 @@ connectDBComplete(PGconn *conn)
 	}
 }
 
+/*
+ * This subroutine saves conn->errorMessage, which will be restored back by
+ * restoreErrorMessage subroutine.
+ */
+static bool
+saveErrorMessage(PGconn *conn, PQExpBuffer savedMessage)
+{
+	initPQExpBuffer(savedMessage);
+	if (PQExpBufferBroken(savedMessage))
+	{
+		printfPQExpBuffer(>errorMessage,
+		  libpq_gettext("out of memory\n"));
+		return false;
+	}
+
+	appendPQExpBufferStr(savedMessage,
+		 conn->errorMessage.data);
+	resetPQExpBuffer(>errorMessage);
+	return true;
+}
+
+/*
+ * Restores saved error messages back to conn->errorMessage.
+ */
+static void
+restoreErrorMessage(PGconn *conn, PQExpBuffer savedMessage)
+{
+	appendPQExpBufferStr(savedMessage, conn->errorMessage.data);
+	resetPQExpBuffer(>errorMessage);
+	appendPQExpBufferStr(>errorMessage, savedMessage->data);
+	termPQExpBuffer(savedMessage);
+}
+
 /* 
  *		PQconnectPoll
  *
@@ -1795,6 +1828,7 @@ PQconnectPoll(PGconn *conn)
 	PGresult   *res;
 	char		sebuf[256];
 	int			optval;
+	PQExpBufferData savedMessage;
 
 	if (conn == NULL)
 		return PGRES_POLLING_FAILED;
@@ -2792,11 +2826,26 @@ keep_going:		/* We will come back to here until there is
 if (conn->target_session_attrs != NULL &&
 	strcmp(conn->target_session_attrs, "read-write") == 0)
 {
+	/*
+	 * We are yet to make a connection. Save all existing error
+	 * messages until we make a successful connection state.
+	 * This is important because PQsendQuery is going to reset
+	 * conn->errorMessage and we will loose error messages
+	 * related to previous hosts we have tried to connect and
+	 * failed.
+	 */
+	if (!saveErrorMessage(conn, ))
+		goto error_return;
+
 	conn->status = CONNECTION_OK;
 	if (!PQsendQuery(conn,
 	 "show transaction_read_only"))
+	{
+		restoreErrorMessage(conn, );
 		goto error_return;
+	}
 	conn->status = CONNECTION_CHECK_WRITABLE;
+	restoreErrorMessage(conn, );
 	return PGRES_POLLING_READING;
 }
 
@@ -2841,11 +2890,18 @@ keep_going:		/* We will come back to here until there is
 			if (conn->target_session_attrs != NULL &&
 strcmp(conn->target_session_attrs, "read-write") == 0)
 			{
+if (!saveErrorMessage(conn, ))
+	goto error_return;
+
 conn->status = CONNECTION_OK;
 if (!PQsendQuery(conn,
  "show transaction_read_only"))
+{
+	restoreErrorMessage(conn, );
 	goto error_return;
+}
 conn->status = CONNECTION_CHECK_WRITABLE;
+restoreErrorMessage(conn, );
 return PGRES_POLLING_READING;
 			}
 
@@ -2858,13 +2914,20 @@ keep_going:		/* We will come back to here until there is
 
 		case CONNECTION_CHECK_WRITABLE:
 			{
+if (!saveErrorMessage(conn, ))
+	goto error_return;
+
 conn->status = CONNECTION_OK;
 if (!PQconsumeInput(conn))
+{
+	restoreErrorMessage(conn, );
 	goto error_return;
+}
 
 if (PQisBusy(conn))
 {
 	conn->status = CONNECTION_CHECK_WRITABLE;
+	restoreErrorMessage(conn, );
 	return PGRES_POLLING_READING;
 }
 
@@ -2878,6 +2941,7 @@ keep_going:		/* We will come back to here until there is
 	if (strncmp(val, "on", 2) == 0)
 	{
 		PQclear(res);
+		restoreErrorMessage(conn, );
 
 		/* Not writable; close connection. */
 		appendPQExpBuffer(>errorMessage,
@@ -2895,6 +2959,7 @@ keep_going:		/* We will come back to here until there is
 

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Fri, Dec 2, 2016 at 8:54 PM, Robert Haas  wrote:
> Couldn't this just be a variable in PQconnectPoll(), instead of adding
> a new structure member?

I have fixed same with a local variable in PQconnectPoll, Initally I
thought savedMessage should have same visiblitly as errorMessage so we can
restore the errorMessage even outside PQconnectPoll. But that seems not
required. Attacting the new patch which fixes the same.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


failover-to-new-master-bug-fix-02.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Thu, Dec 1, 2016 at 5:00 AM, Mithun Cy  wrote:
> On Wed, Nov 30, 2016 at 7:14 AM, Mithun Cy 
> wrote:
>> Thanks, send query resets the errorMessage. Will fix same.
>> PQsendQuery()->PQsendQueryStart()->resetPQExpBuffer(>errorMessage);
>
> Please find the patch which fixes this bug. conn->errorMessage as per
> definition only stores current error message, a new member
> conn->savedMessage is introduced to save and restore error messages related
> to previous hosts which we failed to connect.

Couldn't this just be a variable in PQconnectPoll(), instead of adding
a new structure member?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Wed, Nov 30, 2016 at 7:14 AM, Mithun Cy 
wrote:
> Thanks, send query resets the errorMessage. Will fix same.
>
*PQsendQuery()->PQsendQueryStart()->resetPQExpBuffer(>errorMessage);*

*Please find the patch which fixes this bug. **conn->errorMessage as per
definition only stores current error message, a new member*
*conn->savedMessage is introduced to save and restore error messages
related to previous hosts which we failed to connect.*

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


failover-to-new-writable-session_bugfix_01.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Wed, Nov 30, 2016 at 1:44 AM, Robert Haas  wrote:
 >On Tue, Nov 29, 2016 at 2:19 PM, Kuntal Ghosh 
wrote:
 >> I was doing some testing with the patch and I found some inconsistency
 >> in the error message.

 > Hmm, maybe the query buffer is getting cleared someplace in there.  We
 > might need to save/restore it.

 Thanks, send query resets the errorMessage. Will fix same.

* PQsendQuery()->PQsendQueryStart()->resetPQExpBuffer(>errorMessage);*

Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 29, 2016 at 3:14 PM, Robert Haas  wrote:
> On Tue, Nov 29, 2016 at 2:19 PM, Kuntal Ghosh
>  wrote:
>> On Thu, Nov 24, 2016 at 5:46 PM, Mithun Cy  
>> wrote:
>>> I have taken this suggestion now renamed target_server_type to
>>> target_session_attrs with possible 2 values "read-write", "any".
>>> May be we could expand to "readonly" and "prefer-readonly" in next patch
>>> proposal. Attaching the patch for same.
>> I was doing some testing with the patch and I found some inconsistency
>> in the error message.
>> I've a read-only server running on port 5433 and no server on 5436 and 5438.
>>
>> command: bin/psql
>> 'postgresql://localhost:5436,localhost:5433,localhost:5438/postgres?target_session_attrs=read-write'
>>
>> I get the following error message.
>>
>> psql: could not make a writable connection to server "localhost:5433"
>> could not connect to server: Connection refused
>> Is the server running on host "localhost" (::1) and accepting
>> TCP/IP connections on port 5438?
>> 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 5438?
>>
>> It didn't show any error message for port 5436. But, if I modify the
>> connection string as following:
>>
>> command: bin/psql
>> 'postgresql://localhost:5433,localhost:5436,localhost:5438/postgres?target_session_attrs=read-write'
>>
>> I get the following error message:
>>
>> psql: could not make a writable connection to server "localhost:5433"
>> could not connect to server: Connection refused
>> Is the server running on host "localhost" (::1) and accepting
>> TCP/IP connections on port 5436?
>> 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 5436?
>> could not connect to server: Connection refused
>> Is the server running on host "localhost" (::1) and accepting
>> TCP/IP connections on port 5438?
>> 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 5438?
>
> Hmm, maybe the query buffer is getting cleared someplace in there.  We
> might need to save/restore it.

Not the query buffer.  conn->errorMessage.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 29, 2016 at 2:19 PM, Kuntal Ghosh
 wrote:
> On Thu, Nov 24, 2016 at 5:46 PM, Mithun Cy  wrote:
>> I have taken this suggestion now renamed target_server_type to
>> target_session_attrs with possible 2 values "read-write", "any".
>> May be we could expand to "readonly" and "prefer-readonly" in next patch
>> proposal. Attaching the patch for same.
> I was doing some testing with the patch and I found some inconsistency
> in the error message.
> I've a read-only server running on port 5433 and no server on 5436 and 5438.
>
> command: bin/psql
> 'postgresql://localhost:5436,localhost:5433,localhost:5438/postgres?target_session_attrs=read-write'
>
> I get the following error message.
>
> psql: could not make a writable connection to server "localhost:5433"
> could not connect to server: Connection refused
> Is the server running on host "localhost" (::1) and accepting
> TCP/IP connections on port 5438?
> 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 5438?
>
> It didn't show any error message for port 5436. But, if I modify the
> connection string as following:
>
> command: bin/psql
> 'postgresql://localhost:5433,localhost:5436,localhost:5438/postgres?target_session_attrs=read-write'
>
> I get the following error message:
>
> psql: could not make a writable connection to server "localhost:5433"
> could not connect to server: Connection refused
> Is the server running on host "localhost" (::1) and accepting
> TCP/IP connections on port 5436?
> 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 5436?
> could not connect to server: Connection refused
> Is the server running on host "localhost" (::1) and accepting
> TCP/IP connections on port 5438?
> 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 5438?

Hmm, maybe the query buffer is getting cleared someplace in there.  We
might need to save/restore it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Kuntal Ghosh]

On Thu, Nov 24, 2016 at 5:46 PM, Mithun Cy  wrote:
> I have taken this suggestion now renamed target_server_type to
> target_session_attrs with possible 2 values "read-write", "any".
> May be we could expand to "readonly" and "prefer-readonly" in next patch
> proposal. Attaching the patch for same.
I was doing some testing with the patch and I found some inconsistency
in the error message.
I've a read-only server running on port 5433 and no server on 5436 and 5438.

command: bin/psql
'postgresql://localhost:5436,localhost:5433,localhost:5438/postgres?target_session_attrs=read-write'

I get the following error message.

psql: could not make a writable connection to server "localhost:5433"
could not connect to server: Connection refused
Is the server running on host "localhost" (::1) and accepting
TCP/IP connections on port 5438?
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 5438?

It didn't show any error message for port 5436. But, if I modify the
connection string as following:

command: bin/psql
'postgresql://localhost:5433,localhost:5436,localhost:5438/postgres?target_session_attrs=read-write'

I get the following error message:

psql: could not make a writable connection to server "localhost:5433"
could not connect to server: Connection refused
Is the server running on host "localhost" (::1) and accepting
TCP/IP connections on port 5436?
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 5436?
could not connect to server: Connection refused
Is the server running on host "localhost" (::1) and accepting
TCP/IP connections on port 5438?
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 5438?

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Thu, Nov 24, 2016 at 7:16 AM, Mithun Cy  wrote:
> On Wed, Nov 23, 2016 at 10:19 PM, Catalin Iacob 
> wrote:
>On Tue, Nov 22, 2016 at 8:38 AM, Tsunakawa, Takayuki
>  wrote:
>  >> If you want to connect to a server where the transaction is read-only,
> then shouldn't the connection parameter be something like
>>>"target_session_attrs=readonly"?  That represents exactly what the code
> does.
>
>  >FWIW I find this to be a reasonable compromise. To keep the analogy
>  >with the current patch it would be more something like
> "target_session_attrs=read_write|any".
>
> I have taken this suggestion now renamed target_server_type to
> target_session_attrs with possible 2 values "read-write", "any".

I didn't hear any objections to this approach and, after some thought,
I think it's good.  So I've committed this after rewriting the
documentation, some cosmetic cleanup, and changing the logic so that
if one IP for a host turns out to be read-only, we skip all remaining
IPs for that host, not just the one that we tested already.  This
seems likely to be what users will want.

I recognize that this isn't going to be please everybody 100%, but
there's still room for followup patches to improve things further.
One thing I really like about the target_session_attrs renaming is
that it seems to leave the door open to a variety of things we might
want to do later.  The "read-write" value we now support is closely
related to the query we use for testing ("show
transaction_read_only").  If we later want to filter based on some
other server property, we can add additional values with associated
SQL commands for each.  Of course that could get a little crazy if
overdone, but hopefully we won't overdo it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Wed, Nov 23, 2016 at 10:19 PM, Catalin Iacob 
wrote:
   On Tue, Nov 22, 2016 at 8:38 AM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:
 >> If you want to connect to a server where the transaction is read-only,
then shouldn't the connection parameter be something like
 >>"target_session_attrs=readonly"?  That represents exactly what the code
does.

 >FWIW I find this to be a reasonable compromise. To keep the analogy
 >with the current patch it would be more something like
"target_session_attrs=read_write|any".

I have taken this suggestion now renamed target_server_type to
target_session_attrs with possible 2 values "read-write", "any".
May be we could expand to "readonly" and "prefer-readonly" in next patch
proposal. Attaching the patch for same.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


failover-to-new-writable-session-05.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Catalin Iacob]

On Tue, Nov 22, 2016 at 8:38 AM, Tsunakawa, Takayuki
 wrote:
> transaction_read_only=on does not mean the standby.  As the manual article on 
> hot standby says, they are different.

> I'm afraid that if the current patch is committed, you will lose interest in 
> the ideal solution.

I agree with Robert that lacking the ideal solution shouldn't block a
good enough solution today. But I find the current patch as it stands
too confusing to pass the "good enough" bar.

> Then if the current patch is out as v10, there would be a concern about 
> incompatibility when we pursue the ideal solution in a later release.  That 
> is, "should we continue to report > that this server is standby even if it's 
> actually a primary with transaction_read_only is on, to maintain 
> compatibility with the older release."

Agreed. I am worried that this will end up being a wart:
target_server_type=primary doesn't really mean primary it means that
by default the session is not read only which by the way is usually
the case for primaries but that can be changed.

> If you want to connect to a server where the transaction is read-only, then 
> shouldn't the connection parameter be something like 
> "target_session_attrs=readonly"?  That represents exactly what the code does.

FWIW I find this to be a reasonable compromise. To keep the analogy
with the current patch it would be more something like
"target_session_attrs=read_write|any" and the docs could point out "by
the way, if you didn't tweak default_transaction_read_only read_write
will only pick a primary so this can be used for failover".
The functionality is the same but changing the name removes the
ambiguity in the semantics and leaves the door open for a future real
primary/replica functionality which includes logical replication with
whatever semantics are deemed appropriate then.

We do lose pgjdbc compatibility but the current patch doesn't have
that 100% anyway: in pgjdbc it's called targetServerType and it
accepts master instead of primary and it also accepts slave and
preferSlave
I notice at 
https://github.com/pgjdbc/www/blob/master/documentation/head/connect.md
that pgjdbc says "The master/slave distinction is currently done by
observing if the server allows writes" which, due to the "currently"
part seems to acknowledge this is not ideal and leaves the door open
for changes in semantics but I don't think changing semantics is going
to fly for Postgres itself.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Sun, Nov 13, 2016 at 9:02 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
>> Great, committed.  There's still potentially more work to be done here,
>> because my patch omits some features that were present in Victor's original
>> submission, like setting the failover timeout, optionally randomizing the
>> order of the hosts, and distinguishing between master and standby servers;
>> Victor, or anyone, please feel free to submit separate patches for those
>> things.
>
> The attached patch fixes some bugs and make a clarification for doc.  Could 
> you check and test the authentication stuff as I don't have an environment at 
> hand?

I don't have an environment at hand, either, but I think your fixes
are correct, so I have committed them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

An updated patch with some fixes for bugs reported earlier,

A. failover_to_new_master_v4.patch
Default value "any" is added to target_server_type parameter during its
definition.

B. libpq-failover-smallbugs_02.patch
Fixed the issue raised by [PATCH] pgpassfile connection option
,
Now added host and port name along with pgpass file error message when ever
authentication fails and added same to file libpq-failover-smallbugs.patch
(bugs reported and fixed by Tsunakawa). I did review of original bugs
reported and fixes it seems okay for me. I could not test GSS and SSPI
authentication as it appears they needed windows.


failover_to_new_master_v4.patch
Description: Binary data


libpq-failover-smallbugs_02.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Craig Ringer]

On 21 November 2016 at 00:08, Mithun Cy  wrote:

>> Please avoid adding another round trip by using a GUC_REPORTed variable
>> (ParameterStatus entry).  If you want to support this libpq failover with
>> >pre-10 servers, you can switch the method of determining the primary based
>> on the server version.  But I don't think it's worth supporting older
>> servers > at the price of libpq code complexity.
>
> Currently there is no consensus around this. For now, making this patch to
> address failover to next primary as similar to JDBC seems sufficient for me.
> On next proposal of patch I think we can try to extend as you have proposed

FWIW, I agree. Working first, then improved.

>> I haven't tracked the progress of logical replication, but will
>> target_server_type be likely to be usable with it?  How will
>> target_server_type fit logical   > replication?
>
> I tried to check logical replication WIP patch, not very sure how to
> accomodate same.

Logical replication downstreams won't force transactions to read-only.
A significant part of the appeal of logical replication is that you
can use temp tables and unlogged tables on the downstream, and even
use persistent tables so long as they don't clash with the upstream.
More sophisticated models even allow the downstream to safely write to
replicated tables by doing conflict resolution.

So as it stands, this patch would consider any logical replication
downstream a 'master'.

That's fine for now IMO. Determining whether a server is a logical
replica isn't that simple, nor is it a clear-cut yes/no answer, and I
think it's out of the scope of this patch to deal with it. Figure it
out once logical replication lands.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> I am very strict about regressing the performance of things that we already
> have, but I try not to make a policy that a new feature must be as fast
> as it could ever be.  That could result in us having very few new features.

I see.  I like your attitude toward new features.  But I don't think now is the 
time to compromise this feature and rush to the commit.


> Also, I am not saying that we should not change this in time for v10.
> I'm saying that I don't think it should be a requirement for the next patch
> to be committed in this area to introduce a whole new mechanism for
> determining whether something is a master or a standby.  Love it or hate
> it, pgsql-jdbc has already implemented something in this area and it does
> something useful -- without requiring a wire protocol change.  Now you and
> Kevin are trying to say that what they did is all wrong, but I don't agree.
> There are very many users for whom the pgsql-jdbc approach will do exactly
> what they need, and no doubt some for whom it won't.  Getting a patch that
> mimics that approach committed is *progress*.  Improving it afterwards,
> whether for v10 or some later release, is also good.

transaction_read_only=on does not mean the standby.  As the manual article on 
hot standby says, they are different.

https://www.postgresql.org/docs/devel/static/hot-standby.html

[Excerpt]
--
In normal operation, “read-only” transactions are allowed to update sequences 
and to use LISTEN, UNLISTEN, and NOTIFY, so Hot Standby sessions operate under 
slightly tighter restrictions than ordinary read-only sessions. It is possible 
that some of these restrictions might be loosened in a future release.
...
Users will be able to tell whether their session is read-only by issuing SHOW 
transaction_read_only. In addition, a set of functions (Table 9.79, “Recovery 
Information Functions”) allow users to access information about the standby 
server. These allow you to write programs that are aware of the current state 
of the database. These can be used to monitor the progress of recovery, or to 
allow you to write complex programs that restore the database to particular 
states.
--


I'm afraid that if the current patch is committed, you will lose interest in 
the ideal solution.  Then if the current patch is out as v10, there would be a 
concern about incompatibility when we pursue the ideal solution in a later 
release.  That is, "should we continue to report that this server is standby 
even if it's actually a primary with transaction_read_only is on, to maintain 
compatibility with the older release."

If you want to connect to a server where the transaction is read-only, then 
shouldn't the connection parameter be something like 
"target_session_attrs=readonly"?  That represents exactly what the code does.


> There is a saying that one should not let the perfect be the enemy of the
> good.  I believe that saying applies here.

True, so I suggested not including the support for older servers for a while.  
Shall we find the real enemy -- what's preventing the ideal solution?  I know 
my knowledge is still far less than you, so I may be missing something 
difficult.  So, I'd like Mithun to share the difficulty.

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Sun, Nov 20, 2016 at 8:48 PM, Tsunakawa, Takayuki
 wrote:
>> True, but raising the bar for this feature so that it doesn't get done is
>> also bad.  It can be improved in a later patch.
>
> I thought you are very strict about performance, so I hesitate to believe you 
> forgive the additional round trip.  libpq failover is a new feature in v10, 
> so I think it should provide the best user experience for v10 client+server 
> users from the start.  If the concern is the time for development, then 
> support for older releases can be added in a later patch.
>
> There are still several months left for v10.  Why don't we try the best?  
> Could you share the difficulty?

I am very strict about regressing the performance of things that we
already have, but I try not to make a policy that a new feature must
be as fast as it could ever be.  That could result in us having very
few new features.  Of course, it also matters how frequently the
overhead is likely to be incurred.  A little overhead on each tuple
visibility check is a much bigger problem than a little overhead on
each CREATE TABLE statement, which in turn is a much bigger problem
than a little overhead on each connection attempt.  For good
performance, connections must last a while, so a little extra time
setting one up doesn't seem like a showstopper to me.  Anyway, anybody
who finds this mechanism too expensive doesn't have to use it; they
can implement something else instead.

Also, I am not saying that we should not change this in time for v10.
I'm saying that I don't think it should be a requirement for the next
patch to be committed in this area to introduce a whole new mechanism
for determining whether something is a master or a standby.  Love it
or hate it, pgsql-jdbc has already implemented something in this area
and it does something useful -- without requiring a wire protocol
change.  Now you and Kevin are trying to say that what they did is all
wrong, but I don't agree.  There are very many users for whom the
pgsql-jdbc approach will do exactly what they need, and no doubt some
for whom it won't.  Getting a patch that mimics that approach
committed is *progress*.  Improving it afterwards, whether for v10 or
some later release, is also good.

There is a saying that one should not let the perfect be the enemy of
the good.  I believe that saying applies here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> On Thu, Nov 17, 2016 at 10:08 PM, Craig Ringer 
> wrote:
> > We can and probably should have both.
> >
> > If the server tells us on connect whether it's a standby or not, use that.
> >
> > Otherwise, ask it.
> >
> > That way we don't pay the round-trip cost and get the log spam when
> > talking to newer servers that send us something useful in the startup
> > packet, but we can still query it on older servers. Graceful fallback.
> >
> > Every round trip is potentially very expensive. Having libpq do them
> > unnecessarily is bad.
> 
> True, but raising the bar for this feature so that it doesn't get done is
> also bad.  It can be improved in a later patch.

I thought you are very strict about performance, so I hesitate to believe you 
forgive the additional round trip.  libpq failover is a new feature in v10, so 
I think it should provide the best user experience for v10 client+server users 
from the start.  If the concern is the time for development, then support for 
older releases can be added in a later patch.

There are still several months left for v10.  Why don't we try the best?  Could 
you share the difficulty?


Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: Mithun Cy [mailto:mithun...@enterprisedb.com]
> > +   {"target_server_type", "PGTARGETSERVERTYPE",
> DefaultTargetServerType, NULL,
> > +   "Target-Server-Type", "", 6,
> 
> Thanks fixed.

+   {"target_server_type", "PGTARGETSERVERTYPE", NULL, NULL,

The default value field is still NULL.


> > Please avoid adding another round trip by using a GUC_REPORTed variable
> (ParameterStatus entry).  If you want to support this libpq failover with
> >pre-10 servers, you can switch the method of determining the primary based
> on the server version.  But I don't think it's worth supporting older
> servers > at the price of libpq code complexity.
> 
> Currently there is no consensus around this. For now, making this patch
> to address failover to next primary as similar to JDBC seems sufficient
> for me.
> On next proposal of patch I think we can try to extend as you have proposed

I don't think show transaction is correct, because it's affected by 
default_transaction_read_only and ALTER DATABASE/USER SET 
transaction_read_only.  transaction_read_only is a transaction attribute, not 
the server type.  What we want to use to determine the connection target should 
be not only whether the transaction is read only, but also other attributes 
such as whether temporary tables can be used (only standby), maintenance 
commands can be executed (VACUUM and ANALYZE can run when transaction_read_only 
is on, but not on standby), etc.  And how about other DBMSs?  Do we really want 
to determine the target based on transaction_read_only while e.g. Oracle is 
based on primary/standby?

If you really want the transaction_read_only attribute for your application, 
then your app should execute "SET default_transaction_read_only = on" upon 
connection, or ALTER DATABASE/USER SET default_transaction_read_only = on in 
advance.


Regards
Takayuki Tsunakawa






-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Fri, Nov 18, 2016 at 6:39 AM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:
 >Typo.   , and "observering" -> "observing".

Thanks fixed.

> +   {"target_server_type", "PGTARGETSERVERTYPE",
DefaultTargetServerType, NULL,
> +   "Target-Server-Type", "", 6,

Thanks fixed.

> Please avoid adding another round trip by using a GUC_REPORTed variable
(ParameterStatus entry).  If you want to support this libpq failover with
>pre-10 servers, you can switch the method of determining the primary based
on the server version.  But I don't think it's worth supporting older
servers > at the price of libpq code complexity.

Currently there is no consensus around this. For now, making this patch to
address failover to next primary as similar to JDBC seems sufficient for me.
On next proposal of patch I think we can try to extend as you have proposed

>Please consider supporting "standby" and "prefer_standby" like PgJDBC.
They are useful without load balancing when multiple standbys are used for
HA.

I think they become more relevant with load-balancing. And, making it
usable when we extend this feature to have load-balancing makes sense to
me.

> I haven't tracked the progress of logical replication, but will
target_server_type be likely to be usable with it?  How will
target_server_type fit logical   > replication?

I tried to check logical replication WIP patch, not very sure how to
accomodate same.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


failover_to_new_master_v3.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Craig Ringer]

On 19 November 2016 at 01:29, Robert Haas  wrote:

>> We can and probably should have both.
>>
>> If the server tells us on connect whether it's a standby or not, use that.
>>
>> Otherwise, ask it.
>>
>> That way we don't pay the round-trip cost and get the log spam when
>> talking to newer servers that send us something useful in the startup
>> packet, but we can still query it on older servers. Graceful fallback.
>>
>> Every round trip is potentially very expensive. Having libpq do them
>> unnecessarily is bad.
>
> True, but raising the bar for this feature so that it doesn't get done
> is also bad.  It can be improved in a later patch.

Good point. Starting with the followup query method seems fine.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Thu, Nov 17, 2016 at 10:08 PM, Craig Ringer  wrote:
> On 17 November 2016 at 10:57, Robert Haas  wrote:
>> On Wed, Nov 16, 2016 at 9:00 PM, Tsunakawa, Takayuki
>>  wrote:
>>> Do we really want to enable libpq failover against pre-V10 servers?  I 
>>> don't think so, as libpq is a part of PostgreSQL and libpq failover is a 
>>> new feature in PostgreSQL 10.  At least, as one user, I don't want 
>>> PostgreSQL to sacrifice another round trip to establish a connection.  As a 
>>> developer, I don't want libpq code more complex than necessary (the 
>>> proposed patch adds a new state to the connection state machine.)  And I 
>>> think it's natural for the server to return the server attribute 
>>> (primary/standby, writable, etc.) as a response to the Startup message like 
>>> server_version, standard_conforming_strings and server_encoding.
>>
>> Well, generally speaking, a new feature that works against older
>> server is better than one that doesn't.  Of course, if that entails
>> making other compromises then you have to decide whether it's worth
>> it, but SELECT pg_is_in_recovery() and SHOW transaction_read_only
>> exist in older versions so if we pick either of those methods then it
>> will just work.  If we decide to invent some completely new method of
>> distinguishing masters from standbys, then it might not, but that
>> would be a drawback of such a choice, not a benefit.
>
> We can and probably should have both.
>
> If the server tells us on connect whether it's a standby or not, use that.
>
> Otherwise, ask it.
>
> That way we don't pay the round-trip cost and get the log spam when
> talking to newer servers that send us something useful in the startup
> packet, but we can still query it on older servers. Graceful fallback.
>
> Every round trip is potentially very expensive. Having libpq do them
> unnecessarily is bad.

True, but raising the bar for this feature so that it doesn't get done
is also bad.  It can be improved in a later patch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Craig Ringer]

On 17 November 2016 at 10:57, Robert Haas  wrote:
> On Wed, Nov 16, 2016 at 9:00 PM, Tsunakawa, Takayuki
>  wrote:
>> Do we really want to enable libpq failover against pre-V10 servers?  I don't 
>> think so, as libpq is a part of PostgreSQL and libpq failover is a new 
>> feature in PostgreSQL 10.  At least, as one user, I don't want PostgreSQL to 
>> sacrifice another round trip to establish a connection.  As a developer, I 
>> don't want libpq code more complex than necessary (the proposed patch adds a 
>> new state to the connection state machine.)  And I think it's natural for 
>> the server to return the server attribute (primary/standby, writable, etc.) 
>> as a response to the Startup message like server_version, 
>> standard_conforming_strings and server_encoding.
>
> Well, generally speaking, a new feature that works against older
> server is better than one that doesn't.  Of course, if that entails
> making other compromises then you have to decide whether it's worth
> it, but SELECT pg_is_in_recovery() and SHOW transaction_read_only
> exist in older versions so if we pick either of those methods then it
> will just work.  If we decide to invent some completely new method of
> distinguishing masters from standbys, then it might not, but that
> would be a drawback of such a choice, not a benefit.

We can and probably should have both.

If the server tells us on connect whether it's a standby or not, use that.

Otherwise, ask it.

That way we don't pay the round-trip cost and get the log spam when
talking to newer servers that send us something useful in the startup
packet, but we can still query it on older servers. Graceful fallback.

Every round trip is potentially very expensive. Having libpq do them
unnecessarily is bad.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Mithun Cy
> I am adding next version of the patch it have following fixes.
> Tsunakawa's comments
> 
> 1.  PGconn->target_server_type is now freed in freePGconn() 2.  Added
> PGTARGETSERVERTYPE.
> 
> 
> Additional comments from others
> 3.  Moved from SELECT pg_is_in_recovery() to SHOW transaction_read_only
> now should handle different kind of replication, as we recognise server
> to which writable connection can be made as primary. Very exactly like JDBC
> driver. Also documented about it.
> 4. renamed words from master to primary.

Thank you.  The following items need addressing.  Some of them require some 
more discussion to reach consensus, and I hope they will settle down soon.  
After checking the progress for a week or so, I'll mark the CommitFest entry as 
"ready for committer" or "waiting on author".

(1)
+server. Set this to any, if you want to connect to
+A server is recognized as a primary/standby by observering whether it

Typo.   , and "observering" -> "observing".


(2)
+   {"target_server_type", "PGTARGETSERVERTYPE", NULL, NULL,
+   "Target server type", "", 6,

Looking at existing parameters, the default value is defined as a macro, and 
the display label is a sequence of words separated by "-".  i.e.

+   {"target_server_type", "PGTARGETSERVERTYPE", DefaultTargetServerType, 
NULL,
+   "Target-Server-Type", "", 6,


(3)
Please avoid adding another round trip by using a GUC_REPORTed variable 
(ParameterStatus entry).  If you want to support this libpq failover with 
pre-10 servers, you can switch the method of determining the primary based on 
the server version.  But I don't think it's worth supporting older servers at 
the price of libpq code complexity.


(4)
Please consider supporting "standby" and "prefer_standby" like PgJDBC.  They 
are useful without load balancing when multiple standbys are used for HA.


(5)
I haven't tracked the progress of logical replication, but will 
target_server_type be likely to be usable with it?  How will target_server_type 
fit logical replication?

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Thu, Nov 17, 2016 at 8:27 AM, Robert Haas  wrote:
>but SELECT pg_is_in_recovery() and SHOW transaction_read_only
>exist in older versions so if we pick either of those methods then it
>will just work.

I am adding next version of the patch it have following fixes.
Tsunakawa's comments
1.  PGconn->target_server_type is now freed in freePGconn()
2.  Added PGTARGETSERVERTYPE.

*Additional comments from others*
3.  Moved from SELECT pg_is_in_recovery() to SHOW transaction_read_only now
should handle different kind of replication, as we recognise server to
which writable connection can be made as primary. Very exactly like JDBC
driver. Also documented about it.
4. renamed words from master to primary.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


failover_to_new_master-v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Wed, Nov 16, 2016 at 9:00 PM, Tsunakawa, Takayuki
 wrote:
> Do we really want to enable libpq failover against pre-V10 servers?  I don't 
> think so, as libpq is a part of PostgreSQL and libpq failover is a new 
> feature in PostgreSQL 10.  At least, as one user, I don't want PostgreSQL to 
> sacrifice another round trip to establish a connection.  As a developer, I 
> don't want libpq code more complex than necessary (the proposed patch adds a 
> new state to the connection state machine.)  And I think it's natural for the 
> server to return the server attribute (primary/standby, writable, etc.) as a 
> response to the Startup message like server_version, 
> standard_conforming_strings and server_encoding.

Well, generally speaking, a new feature that works against older
server is better than one that doesn't.  Of course, if that entails
making other compromises then you have to decide whether it's worth
it, but SELECT pg_is_in_recovery() and SHOW transaction_read_only
exist in older versions so if we pick either of those methods then it
will just work.  If we decide to invent some completely new method of
distinguishing masters from standbys, then it might not, but that
would be a drawback of such a choice, not a benefit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Hmm, let's go back to the JDBC method, then.  "show transaction_read_only"
> will return true on a standby, but presumably also on any other non-writable
> node.  You could even force it to be true artificially if you wanted to
> force traffic off of a node, using ALTER {SYSTEM|USER ...|DATABASE ..} SET
> default_transaction_read_only = on
> 
> I think that would address Alvaro's concern, and it's nicer anyway if libpq
> and JDBC are doing the same thing.

If you prefer consistency between libpq and JDBC, then we could correct JDBC.  
People here should know the server state well, and be able to figure out a good 
specification.

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> In my understanding pg_is_in_recovery() returns true if it's a standby node.
> However, even if it returns other than true, the server is not necessarily
> a primary. Even it's not configured as a streaming replication primary,
> it returns other than true.
> 
> So if your intention is finding a primary, I am not sure if
> pg_is_in_recovery() is the best solution.

Yes, I don't think pg_is_in_recovery() is the best, but there doesn't seem to 
be a better solution.  pg_is_in_recovery(), as its name clearly suggests, 
returns true if the server is performing recovery.  For example, it returns 
true if hot_standby=on is present in postgresql.conf and the recovery from 
backup is in progress.  It's not a standby.

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> On Mon, Nov 14, 2016 at 8:09 PM, Tsunakawa, Takayuki
>  wrote:
> > From: pgsql-hackers-ow...@postgresql.org
> >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Mithun Cy
> >> Thanks, my concern is suppose you have 3 server in cluster A(new
> >> version), B(new version), C(old version). If we implement as above
> >> only new servers will send ParameterStatus message to indicate what
> >> type of server we are connected. Server C will not send same. So we
> >> will not be able to use new feature "failover to new master" for such
> a kind of cluster.
> >
> > No, the streaming replication requires the same major release for all
> member servers, so there's no concern about the mixed-version cluster.
> 
> True, but there is a concern about a newer libpq connecting to older servers.
> If we mimic what JDBC is already doing, we'll be compatible and you'll be
> able to use this feature with a v10 libpq without worrying about whether
> the target server is also v10.  If we invent something new on the server
> side, then you'll need to be sure you have both a v10 libpq and v10 server.

Do we really want to enable libpq failover against pre-V10 servers?  I don't 
think so, as libpq is a part of PostgreSQL and libpq failover is a new feature 
in PostgreSQL 10.  At least, as one user, I don't want PostgreSQL to sacrifice 
another round trip to establish a connection.  As a developer, I don't want 
libpq code more complex than necessary (the proposed patch adds a new state to 
the connection state machine.)  And I think it's natural for the server to 
return the server attribute (primary/standby, writable, etc.) as a response to 
the Startup message like server_version, standard_conforming_strings and 
server_encoding.

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Wed, Nov 16, 2016 at 12:00 PM, Catalin Iacob  wrote:
> On Wed, Nov 16, 2016 at 2:50 PM, Robert Haas  wrote:
>> Hmm, let's go back to the JDBC method, then.  "show
>> transaction_read_only" will return true on a standby, but presumably
>> also on any other non-writable node.  You could even force it to be
>> true artificially if you wanted to force traffic off of a node, using
>> ALTER {SYSTEM|USER ...|DATABASE ..} SET default_transaction_read_only
>> = on
>>
>> I think that would address Alvaro's concern, and it's nicer anyway if
>> libpq and JDBC are doing the same thing.
>
> Not sure I agree that using this is a good idea in the first place.
>
> But if we end up using this, I really think the docs should be very
> explicit about what's implemented and not just say master/any. With
> the master/any docs in the patch I would be *very* surprised if a
> master is skipped only because it was configured with
> default_transaction_read_only = on.

It seems like it is going to be difficult to please everyone here
100%, because there are multiple conflicting priorities.  But we can
definitely document whatever choices we make.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Catalin Iacob]

On Wed, Nov 16, 2016 at 2:50 PM, Robert Haas  wrote:
> Hmm, let's go back to the JDBC method, then.  "show
> transaction_read_only" will return true on a standby, but presumably
> also on any other non-writable node.  You could even force it to be
> true artificially if you wanted to force traffic off of a node, using
> ALTER {SYSTEM|USER ...|DATABASE ..} SET default_transaction_read_only
> = on
>
> I think that would address Alvaro's concern, and it's nicer anyway if
> libpq and JDBC are doing the same thing.

Not sure I agree that using this is a good idea in the first place.

But if we end up using this, I really think the docs should be very
explicit about what's implemented and not just say master/any. With
the master/any docs in the patch I would be *very* surprised if a
master is skipped only because it was configured with
default_transaction_read_only = on.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Kevin Grittner]

[moving this branch of discussion to pgsql-jdbc]

On Tue, Nov 15, 2016 at 10:31 PM, Mithun Cy  wrote:

> JDBC is sending "show transaction_read_only" to find whether it
> is master or not.

If true, that's insane.  That can be different on each connection
to the cluster and can change tens of thousands of times per second
on any connection!

I know of one large shop that sets default_transaction_read_only =
true because 99% of their transactions are read only and they use
serializable transactions -- which run faster and with less
contention when transactions which don't need to write are flagged
as read only.  It seems safer to them to only turn off the read
only property for transactions which might need to write.

> Victor's patch also started with it, but later it was transformed into
> pg_is_in_recovery
> by him as it appeared more appropriate to identify the master / slave.

I don't know whether that is ideal, but it is sure a lot better
than something which can change with every transaction -- or even
within a transaction (in one direction).

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 15, 2016 at 11:31 PM, Mithun Cy  wrote:
>> > So I am tempted to just
>> > hold my nose and hard-code the SQL as JDBC is presumably already
>> doing.
>
> JDBC is sending "show transaction_read_only" to find whether it is master or
> not.
> Victor's patch also started with it, but later it was transformed into
> pg_is_in_recovery
> by him as it appeared more appropriate to identify the master / slave.

Hmm, let's go back to the JDBC method, then.  "show
transaction_read_only" will return true on a standby, but presumably
also on any other non-writable node.  You could even force it to be
true artificially if you wanted to force traffic off of a node, using
ALTER {SYSTEM|USER ...|DATABASE ..} SET default_transaction_read_only
= on

I think that would address Alvaro's concern, and it's nicer anyway if
libpq and JDBC are doing the same thing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tatsuo Ishii]

> JDBC is sending "show transaction_read_only" to find whether it is master
> or not.
> Victor's patch also started with it, but later it was transformed into
> pg_is_in_recovery
> by him as it appeared more appropriate to identify the master / slave.

In my understanding pg_is_in_recovery() returns true if it's a standby
node. However, even if it returns other than true, the server is not
necessarily a primary. Even it's not configured as a streaming
replication primary, it returns other than true.

So if your intention is finding a primary, I am not sure if
pg_is_in_recovery() is the best solution.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

>
> > So I am tempted to just
> > hold my nose and hard-code the SQL as JDBC is presumably already
> doing.


JDBC is sending "show transaction_read_only" to find whether it is master
or not.
Victor's patch also started with it, but later it was transformed into
pg_is_in_recovery
by him as it appeared more appropriate to identify the master / slave.

ConnectionFactoryImpl.java:685

+ private boolean isMaster(QueryExecutor queryExecutor, Logger logger)
+ throws SQLException, IOException {
+byte[][] results = SetupQueryRunner.run(queryExecutor, "show
transaction_read_only", true);
+String value = queryExecutor.getEncoding().decode(results[0]);
+return value.equalsIgnoreCase("off");
}

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 15, 2016 at 12:53 PM, Catalin Iacob  wrote:
> On Tue, Nov 15, 2016 at 5:58 PM, Robert Haas  wrote:
>> On Tue, Nov 15, 2016 at 9:42 AM, Alvaro Herrera
>>  wrote:
>>> I would rather come up with something that works in both cases that we
>>> can extend internally later, say pg_is_primary_node() or something like
>>> that instead; and we implement it initially by returning the inverse of
>>> pg_is_in_recovery() for replicated non-logical flocks, while we figure
>>> out what to do in logical replication.  Otherwise it will be harder to
>>> change later if we embed it in libpq, and we may be forced into
>>> supporting nonsensical situations such as having pg_is_in_recovery()
>>> return true for logical replication primary nodes.
>>
>> I don't think we'll be backed into a corner like that, because we can
>> always make this contingent on server version.  libpq will have that
>> available.
>
> But even with server version checking code, that code will be inside
> libpq so there will be old libpq versions in the field that won't know
> the proper query to send to new server versions.

Good point.  pg_is_writable_node() sounds good, then, and we can still
send pg_is_in_recovery() if we're connected to a pre-10 version.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Alvaro Herrera]

Catalin Iacob wrote:
> On Tue, Nov 15, 2016 at 3:42 PM, Alvaro Herrera
>  wrote:
> > FWIW I'm not sure "primary" is the right term here either.  I think what
> > we really want to know is whether the node can accept writes; maybe
> > "pg_is_writable_node".
> 
> This made me think of another complication: what about cascading
> replication where the middle node is both a master and a slave? You'd
> probably not want to fallback to the middle node even though it is a
> master for its downstream.

Exactly my thought, hence the above suggestion.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Catalin Iacob]

On Tue, Nov 15, 2016 at 3:42 PM, Alvaro Herrera
 wrote:
> FWIW I'm not sure "primary" is the right term here either.  I think what
> we really want to know is whether the node can accept writes; maybe
> "pg_is_writable_node".

This made me think of another complication: what about cascading
replication where the middle node is both a master and a slave? You'd
probably not want to fallback to the middle node even though it is a
master for its downstream.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Catalin Iacob]

On Tue, Nov 15, 2016 at 5:58 PM, Robert Haas  wrote:
> On Tue, Nov 15, 2016 at 9:42 AM, Alvaro Herrera
>  wrote:
>> I would rather come up with something that works in both cases that we
>> can extend internally later, say pg_is_primary_node() or something like
>> that instead; and we implement it initially by returning the inverse of
>> pg_is_in_recovery() for replicated non-logical flocks, while we figure
>> out what to do in logical replication.  Otherwise it will be harder to
>> change later if we embed it in libpq, and we may be forced into
>> supporting nonsensical situations such as having pg_is_in_recovery()
>> return true for logical replication primary nodes.
>
> I don't think we'll be backed into a corner like that, because we can
> always make this contingent on server version.  libpq will have that
> available.

But even with server version checking code, that code will be inside
libpq so there will be old libpq versions in the field that won't know
the proper query to send to new server versions.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 15, 2016 at 9:42 AM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> On Mon, Nov 14, 2016 at 2:38 AM, Mithun Cy  
>> wrote:
>> > I have not tested with logical replication. Currently we identify the
>> > primary to connect based on result of "SELECT pg_is_in_recovery()". So I
>> > think it works. Do you want me test a particular setup?
>>
>> If logical replication is in use, none of the servers involved would
>> be in recovery.  I'm not sure what command would need to be used to
>> assess whether we've got a master or a standby, but probably not that
>> one.  This gets at one of my earlier complaints about this part of the
>> functionality, which is that hardcoding that particular SQL statement
>> into libpq seems like a giant hack.  However, I'm not sure what to do
>> about it.  The functionality is clearly useful, because JDBC has it,
>> and Victor proposed this patch to add it to libpq, and - totally
>> independently of any of that - EnterpriseDB has a customer who has
>> requested libpq support for this as well.  So I am tempted to just
>> hold my nose and hard-code the SQL as JDBC is presumably already
>> doing.  If we figure out what the equivalent for logical replication
>> would be we can add something to cover that case, too.  It's ugly, but
>> I don't have a better idea, and I think there's value in being
>> compatible with what JDBC has already done (even if it's not what we
>> would have chosen to do tabula rasa).
>
> I would rather come up with something that works in both cases that we
> can extend internally later, say pg_is_primary_node() or something like
> that instead; and we implement it initially by returning the inverse of
> pg_is_in_recovery() for replicated non-logical flocks, while we figure
> out what to do in logical replication.  Otherwise it will be harder to
> change later if we embed it in libpq, and we may be forced into
> supporting nonsensical situations such as having pg_is_in_recovery()
> return true for logical replication primary nodes.

I don't think we'll be backed into a corner like that, because we can
always make this contingent on server version.  libpq will have that
available.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Alvaro Herrera]

Robert Haas wrote:
> On Mon, Nov 14, 2016 at 2:38 AM, Mithun Cy  wrote:

> > I have not tested with logical replication. Currently we identify the
> > primary to connect based on result of "SELECT pg_is_in_recovery()". So I
> > think it works. Do you want me test a particular setup?
> 
> If logical replication is in use, none of the servers involved would
> be in recovery.  I'm not sure what command would need to be used to
> assess whether we've got a master or a standby, but probably not that
> one.  This gets at one of my earlier complaints about this part of the
> functionality, which is that hardcoding that particular SQL statement
> into libpq seems like a giant hack.  However, I'm not sure what to do
> about it.  The functionality is clearly useful, because JDBC has it,
> and Victor proposed this patch to add it to libpq, and - totally
> independently of any of that - EnterpriseDB has a customer who has
> requested libpq support for this as well.  So I am tempted to just
> hold my nose and hard-code the SQL as JDBC is presumably already
> doing.  If we figure out what the equivalent for logical replication
> would be we can add something to cover that case, too.  It's ugly, but
> I don't have a better idea, and I think there's value in being
> compatible with what JDBC has already done (even if it's not what we
> would have chosen to do tabula rasa).

I would rather come up with something that works in both cases that we
can extend internally later, say pg_is_primary_node() or something like
that instead; and we implement it initially by returning the inverse of
pg_is_in_recovery() for replicated non-logical flocks, while we figure
out what to do in logical replication.  Otherwise it will be harder to
change later if we embed it in libpq, and we may be forced into
supporting nonsensical situations such as having pg_is_in_recovery()
return true for logical replication primary nodes.

FWIW I'm not sure "primary" is the right term here either.  I think what
we really want to know is whether the node can accept writes; maybe
"pg_is_writable_node".

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Mon, Nov 14, 2016 at 8:09 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Mithun Cy
>> Thanks, my concern is suppose you have 3 server in cluster A(new version),
>> B(new version), C(old version). If we implement as above only new servers
>> will send ParameterStatus message to indicate what type of server we are
>> connected. Server C will not send same. So we will not be able to use new
>> feature "failover to new master" for such a kind of cluster.
>
> No, the streaming replication requires the same major release for all member 
> servers, so there's no concern about the mixed-version cluster.

True, but there is a concern about a newer libpq connecting to older
servers.  If we mimic what JDBC is already doing, we'll be compatible
and you'll be able to use this feature with a v10 libpq without
worrying about whether the target server is also v10.  If we invent
something new on the server side, then you'll need to be sure you have
both a v10 libpq and v10 server.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Mon, Nov 14, 2016 at 2:38 AM, Mithun Cy  wrote:
> On Fri, Nov 11, 2016 at 7:33 PM, Peter Eisentraut
>  wrote:
>> We tend to use the term "primary" instead of "master".
>
> Thanks, I will use primary instead of master in my next patch.
>
>>Will this work with logical replication?
>
> I have not tested with logical replication. Currently we identify the
> primary to connect based on result of "SELECT pg_is_in_recovery()". So I
> think it works. Do you want me test a particular setup?

If logical replication is in use, none of the servers involved would
be in recovery.  I'm not sure what command would need to be used to
assess whether we've got a master or a standby, but probably not that
one.  This gets at one of my earlier complaints about this part of the
functionality, which is that hardcoding that particular SQL statement
into libpq seems like a giant hack.  However, I'm not sure what to do
about it.  The functionality is clearly useful, because JDBC has it,
and Victor proposed this patch to add it to libpq, and - totally
independently of any of that - EnterpriseDB has a customer who has
requested libpq support for this as well.  So I am tempted to just
hold my nose and hard-code the SQL as JDBC is presumably already
doing.  If we figure out what the equivalent for logical replication
would be we can add something to cover that case, too.  It's ugly, but
I don't have a better idea, and I think there's value in being
compatible with what JDBC has already done (even if it's not what we
would have chosen to do tabula rasa).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Mithun Cy
> Thanks, my concern is suppose you have 3 server in cluster A(new version),
> B(new version), C(old version). If we implement as above only new servers
> will send ParameterStatus message to indicate what type of server we are
> connected. Server C will not send same. So we will not be able to use new
> feature "failover to new master" for such a kind of cluster.

No, the streaming replication requires the same major release for all member 
servers, so there's no concern about the mixed-version cluster.

Sorry, pmState can only be used in postmaster.  In our context, postgres can 
use RecoveryInProgress().  Anyway, in addition to the reduced round trip, the 
libpq code would be much simpler.

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Mon, Nov 14, 2016 at 1:37 PM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:
 > No, there's no concern about compatibility.  Please look at this:
 > https://www.postgresql.org/docs/devel/static/protocol-
flow.html#PROTOCOL-ASYNC
Thanks, my concern is suppose you have 3 server in cluster A(new version),
B(new version), C(old version). If we implement as above only new servers
will send ParameterStatus message to indicate what type of server we are
connected. Server C will not send same. So we will not be able to use new
feature "failover to new master" for such a kind of cluster.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Mithun Cy
> If you are suggesting me to change in protocol messages, I think that would
> not be backward compatible to older version servers. I also think such level
> of protocol changes will not be allowed. with connection status
> CONNECTION_SETENV used for protocol version 2.0 setup, we sent some query
> like "select pg_catalog.pg_client_encoding()" for same. So I think using
> "SELECT pg_is_in_recovery()" should be fine.

No, there's no concern about compatibility.  Please look at this:

https://www.postgresql.org/docs/devel/static/protocol-flow.html#PROTOCOL-ASYNC

[Excerpt]

ParameterStatus messages will be generated whenever the active value changes 
for any of the parameters the backend believes the frontend should know about. 
Most commonly this occurs in response to a SET SQL command executed by the 
frontend, and this case is effectively synchronous — but it is also possible 
for parameter status changes to occur because the administrator changed a 
configuration file and then sent the SIGHUP signal to the server. Also, if a 
SET command is rolled back, an appropriate ParameterStatus message will be 
generated to report the current effective value.

At present there is a hard-wired set of parameters for which ParameterStatus 
will be generated: they are server_version, server_encoding, client_encoding, 
application_name, is_superuser, session_authorization, DateStyle, 
IntervalStyle, TimeZone, integer_datetimes, and standard_conforming_strings. 
(server_encoding, TimeZone, and integer_datetimes were not reported by releases 
before 8.0; standard_conforming_strings was not reported by releases before 
8.1; IntervalStyle was not reported by releases before 8.4; application_name 
was not reported by releases before 9.0.) Note that server_version, 
server_encoding and integer_datetimes are pseudo-parameters that cannot change 
after startup. This set might change in the future, or even become 
configurable. Accordingly, a frontend should simply ignore ParameterStatus for 
parameters that it does not understand or care about.




Regards
Takayuki Tsunakawa


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Mon, Nov 14, 2016 at 11:31 AM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:
> PGconn->target_server_type is not freed in freePGconn().

Thanks, will fix in new patch.

>Could you add PGTARGETSERVERTYPE environment variable?  Like other
variables, it will ease testing, since users can change the behavior
>without altering the connection string here and there.

Okay, will add one.

> I think it would be better to expose the server state via ParameterStatus
protocol message like standard_conforming_strings, instead of running >
>"SELECT pg_is_in_recovery()".  We shouldn't want to add one round trip to
check the server type (master, standby).  postmaster can return the server
>type based on its state (pmState); PM_RUN is master, and PM_HOT_STANDBY is
standby.  In addition, as an impractical concern, DBA can revoke >EXECUTE
privilege on pg_is_in_recovery() from non-superusers.

If you are suggesting me to change in protocol messages, I think that would
not be backward compatible to older version servers. I also think such
level of protocol changes will not be allowed. with connection status
CONNECTION_SETENV used for protocol version 2.0 setup, we sent some query
 like
"select pg_catalog.pg_client_encoding()" for same. So I think using "SELECT
pg_is_in_recovery()" should be fine.
-
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Fri, Nov 11, 2016 at 7:33 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
> We tend to use the term "primary" instead of "master".

Thanks, I will use primary instead of master in my next patch.

>Will this work with logical replication?

I have not tested with logical replication. Currently we identify the
primary to connect based on result of "SELECT pg_is_in_recovery()". So I
think it works. Do you want me test a particular setup?

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

Hi, Mithun

Before going deeper into the patch, let me give you some findings.

(1)
PGconn->target_server_type is not freed in freePGconn().


(2)
Could you add PGTARGETSERVERTYPE environment variable?  Like other variables, 
it will ease testing, since users can change the behavior without altering the 
connection string here and there.


(3)
I think it would be better to expose the server state via ParameterStatus 
protocol message like standard_conforming_strings, instead of running "SELECT 
pg_is_in_recovery()".  We shouldn't want to add one round trip to check the 
server type (master, standby).  postmaster can return the server type based on 
its state (pmState); PM_RUN is master, and PM_HOT_STANDBY is standby.  In 
addition, as an impractical concern, DBA can revoke EXECUTE privilege on 
pg_is_in_recovery() from non-superusers.

Regards
Takayuki Tsunakawa


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Great, committed.  There's still potentially more work to be done here,
> because my patch omits some features that were present in Victor's original
> submission, like setting the failover timeout, optionally randomizing the
> order of the hosts, and distinguishing between master and standby servers;
> Victor, or anyone, please feel free to submit separate patches for those
> things.

The attached patch fixes some bugs and make a clarification for doc.  Could you 
check and test the authentication stuff as I don't have an environment at hand?

(1) fe-connect.c
There was a memory leak.

(2) fe-secure_openssl.c, fe-auth.c
GSSAPI/SSPI/SSL authentication requires the target host name, but the code uses 
conn->pghost which contains a comma-separated list of host names.

(3) libpq.sgml
Added sentences to clarify connect_timeout when it is used with multiple hosts.

BTW, let me two questions.

Q1: Is there any reason why hostaddr doesn't accept multiple IP addresses?

Q2: pg_isready (and hence PQping() family) reports success when one host is 
alive and other hosts are down.  Is this intended?  I think this behavior is 
correct.
e.g. host1 is down and host2 is alive.
$ pg_isready -h host1,host2
host1,host2:5450 - accepting connections
$ echo $?
0

Regards
Takayuki Tsunakawa



libpq-failover-smallbugs.patch
Description: libpq-failover-smallbugs.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Peter Eisentraut]

On 11/9/16 10:05 AM, Mithun Cy wrote:
> As in Victor's patch we have a new connection parameter
> "target_server_type", It can take 2 values 1. "any" 2. "master" with
> DEFAULT as "any". If it's has the value "any" we can connect to any of
> the host server (both master(primary) and slave(standby)). If the value
> is "master" then we try to connect to master(primary) only.

We tend to use the term "primary" instead of "master".

Will this work with logical replication?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Great, committed.  There's still potentially more work to be done here,
> because my patch omits some features that were present in Victor's original
> submission, like setting the failover timeout, optionally randomizing the
> order of the hosts, and distinguishing between master and standby servers;
> Victor, or anyone, please feel free to submit separate patches for those
> things.

I did a few tests with ECPG.  I'm satisfied with the current behavior, but 
someone says different.  I'd like to share the result.

The following literal connection strings succeeded.  host1 is a server where 
PostgreSQL is not running, and host2 is where it's running.  I could connect to 
the database server on host2.

EXEC SQL CONNECT TO 'tcp:postgresql://host1,host2:5450/postgres';
EXEC SQL CONNECT TO 'tcp:postgresql://host1,host2:5450,5450/postgres';

EXEC SQL CONNECT TO 'postgres@host1,host2:5450';
EXEC SQL CONNECT TO 'postgres@host1,host2:5450,5450';


EXEC SQL CONNECT TO 'tcp:postgresql://?service=my_service';

~/.pg_service.conf
[my_service]
host=host1,host2
port=5450  # and port=5450,5450 case
dbname=postgres


But this one makes PQconnectdbParams() fail, because the passed "host" is 
"host1:5450,host2" and "port" is "5450".  ECPGconnect()'s parser is different 
from libpq's.  However, the tcp:postgresql:// syntax is not described a URL in 
the manual, so I think it's sufficient to just describe the syntax in the ECPG 
article.

EXEC SQL CONNECT TO 'tcp:postgresql://host1:5450,host2:5450/postgres';


And without the single quote like below, ecpg fails to precompile the source 
file.  I also think it's enough to state in the manual "quote the connection 
target if you specify multiple hosts or ports".

EXEC SQL CONNECT TO tcp:postgresql://host1,host2:5450,5450/postgres;

connect.ec:12: ERROR: syntax error at or near ","


Comments?


Regards
Takayuki Tsunakawa


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Mithun Cy
> Yes this patch will only address failover to new master, values "master"
> and "any" appeared sufficient for that case.

Do you mean that unlike pgJDBC "standby" and "prefer_standby" are useless, or 
they are useful but you don't have time to implement it and want to do it in 
the near future?  Do you mind if I do it if time permits me?  I think they are 
useful without load balancing feature, when the user has multiple standbys for 
HA.

Could you add a new entry in CommitFest 2017-1? I'm afraid we can't track the 
status of your patch because the original patch in this thread has already been 
committed.

Regards
Takayuki Tsunakawa
 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Thu, Nov 10, 2016 at 1:11 PM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:
> Why don't you add "standby" and "prefer_standby" as the
target_server_type value?  Are you thinking that those values are useful
with load balancing > feature?
Yes this patch will only address failover to new master, values "master"
and "any" appeared sufficient for that case.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Mithun Cy
> Among the remaining things I have worked on failover to new master idea.
> Below patch implement that idea. This is taken from Victors patch but
> rewritten by me to do some cleanup. As in Victor's patch we have a new
> connection parameter "target_server_type", It can take 2 values 1. "any"
> 2. "master" with DEFAULT as "any". If it's has the value "any" we can connect
> to any of the host server (both master(primary) and slave(standby)). If
> the value is "master" then we try to connect to master(primary) only.
> NOTE: Parameter name is inspired and taken from PostgreSql JDBC Driver
>  .

I'm interested to review this patch (but I haven't read it yet, I'm reading 
Robert's patch now.)  Are you planning a new CommitFest entry?

Why don't you add "standby" and "prefer_standby" as the target_server_type 
value?  Are you thinking that those values are useful with load balancing 
feature?


> The main difference between Victor's and this new patch is Default value
> of parameter target_server_type. In Victor's patch if number of host in
> connection string is 1 then default value is "any" (This was done to make
> sure old psql connect to standby as it is now). If it is greater than 1
> then default value is set as "master". For me this appeared slightly
> inconsistent having default value as "any" for any number of connection
> appeared more appropriate which is also backward compatible. And, if user
> want failover to master he should ask for it by setting
> target_server_type=master in connection string.

That's sensible, I agree.


Regards
Takayuki Tsunakawa


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Thu, Nov 3, 2016 at 7:16 PM, Robert Haas  wrote:
> Great, committed.  There's still potentially more work to be done
> here, because my patch omits some features that were present in
> Victor's original submission, like setting the failover timeout,
> optionally randomizing the order of the hosts, and distinguishing
> between master and standby servers; Victor, or anyone, please feel
> free to submit separate patches for those things.

Among the remaining things I have worked on failover to new master idea.
Below patch implement that idea. This is taken from Victors patch but
rewritten by me to do some cleanup. As in Victor's patch we have a new
connection parameter "target_server_type", It can take 2 values 1. "any" 2.
"master" with DEFAULT as "any". If it's has the value "any" we can connect
to any of the host server (both master(primary) and slave(standby)). If the
value is "master" then we try to connect to master(primary) only.
NOTE: Parameter name is inspired and taken from PostgreSql JDBC Driver
.

The main difference between Victor's and this new patch is Default value of
parameter target_server_type. In Victor's patch if number of host in
connection string is 1 then default value is "any" (This was done to make
sure old psql connect to standby as it is now). If it is greater than 1
then default value is set as "master". For me this appeared slightly
inconsistent having default value as "any" for any number of connection
appeared more appropriate which is also backward compatible. And, if user
want failover to master he should ask for it by setting
target_server_type=master in connection string.

This patch do not include load balancing feature by trying to connect to
host in random fashion.

This patch also do not have failover timeout feature. Victor's
implementation of same is not a strict one. i. e. 1 walk of all of the host
is allowed even if timer might have expired. Only on second walk we check
for timeout. I thought application can manage the failover timeout instead
libpq doing it. So avoided the same in this patch. If others find that I am
wrong to think that way, will add the same.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


failover_to_new_master-v1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Wed, Nov 2, 2016 at 1:59 PM, Mithun Cy  wrote:
> On Tue, Nov 1, 2016 at 9:42 PM, Robert Haas  wrote:
>> Ah, nuts.  Thanks, good catch.  Should be fixed in the attached version.
>
> I repeated the test on new patch, It works fine now, Also did some more
> negative tests forcibly failing some internal calls. All tests have passed.
> This patch works as described and look good to me.

Great, committed.  There's still potentially more work to be done
here, because my patch omits some features that were present in
Victor's original submission, like setting the failover timeout,
optionally randomizing the order of the hosts, and distinguishing
between master and standby servers; Victor, or anyone, please feel
free to submit separate patches for those things.

Thanks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Tue, Nov 1, 2016 at 9:42 PM, Robert Haas  wrote:
> Ah, nuts.  Thanks, good catch.  Should be fixed in the attached version.

I repeated the test on new patch, It works fine now, Also did some more
negative tests forcibly failing some internal calls. All tests have passed.
This patch works as described and look good to me.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 1, 2016 at 11:21 AM, Mithun Cy  wrote:
> On Tue, Nov 1, 2016 at 7:44 PM, Robert Haas  wrote:
>>> Starting program: /home/mithun/libpqbin/bin/./psql
>>> postgres://%2home%2mithun:/postgres -U mithun1
>>Can you provide a concrete test scenario or some test code that fails?
>>connhost is supposed to be getting set in connectOptions2(), which is
>>run (AIUI) when the PGconn is first created.  So I think that it will
>>be set in all scenarios of the type you mention, but I might be
>>missing something.
>
> Sorry if my sentence is confusing
> If I give a proper hexadecimal encoding % followed by 2 hexadigit (i.e. for
> e.g %2f, %2a) every thing is fine. When I pass a invalid hexadigit encoding
> eg:  %2h, %2m among the host string e.g
> "postgres://%2home%2mithun:/postgres". then "PQconnectdbParams()" fails
> before calling connectOptions2(). In that case failed PQconnectdbParams()
> also return a PGconn where connhost is not set. If we call PQpass(),
> PQReset() on such a PGconn we get a crash.
>
> A simple test case which crash is:
> ./psql  'postgres://%2hxxx:/postgres'
>
> Call stack:
> --
> #0  0x77bb8d4f in PQpass (conn=0x68aa10) at fe-connect.c:5582
> #1  0x77bb907a in PQconnectionNeedsPassword (conn=0x68aa10) at
> fe-connect.c:5727
> #2  0x004130aa in main (argc=2, argv=0x7fffdff8) at
> startup.c:250

Ah, nuts.  Thanks, good catch.  Should be fixed in the attached version.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


multihost-v3.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Tue, Nov 1, 2016 at 7:44 PM, Robert Haas  wrote:
>> Starting program: /home/mithun/libpqbin/bin/./psql
>> postgres://%2home%2mithun:/postgres -U mithun1
>Can you provide a concrete test scenario or some test code that fails?
>connhost is supposed to be getting set in connectOptions2(), which is
>run (AIUI) when the PGconn is first created.  So I think that it will
>be set in all scenarios of the type you mention, but I might be
>missing something.

Sorry if my sentence is confusing
If I give a proper hexadecimal encoding % followed by 2 hexadigit (i.e. for
e.g %2f, %2a) every thing is fine. When I pass a invalid hexadigit encoding
eg:  *%2h, %2m *among the host string e.g
"postgres://*%2h*ome*%2m*ithun:/postgres".
then "PQconnectdbParams()" fails before calling connectOptions2(). In that
case failed PQconnectdbParams() also return a PGconn where connhost is not
set. If we call PQpass(), PQReset() on such a PGconn we get a crash.

A simple test case which crash is:
./psql  'postgres://%2hxxx:/postgres'

Call stack:
--
#0  0x77bb8d4f in PQpass (conn=0x68aa10) at fe-connect.c:5582
#1  0x77bb907a in PQconnectionNeedsPassword (conn=0x68aa10) at
fe-connect.c:5727
#2  0x004130aa in main (argc=2, argv=0x7fffdff8) at
startup.c:250

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 1, 2016 at 9:43 AM, Mithun Cy  wrote:
>> Starting program: /home/mithun/libpqbin/bin/./psql
>> postgres://%2fhome%2fmithun:/postgres -U mithun1
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib64/libthread_db.so.1".
>> psql: could not connect to server: No such file or directory
>> Is the server running locally and accepting
>> connections on Unix domain socket "/home/mithun/.s.PGSQL."?
>
> Accidentally when testing further on this line I found a crash when invalid
> encoding "%2" (not recognised as hexa) was used.
> Test:
>>
>> Starting program: /home/mithun/libpqbin/bin/./psql
>> postgres://%2home%2mithun:/postgres -U mithun1

You seem to be confused about how to escape characters in URLs.  The
format is a % followed by exactly two hex digits.  In this case, that
would give you %2h and %2m, and h and m are not hex characters, so the
error message is right.  Your input is not valid.

> There can be PGconn which have no connhost.
> exposed API's PQpass, PQreset->connectDBStart access conn->connhost without
> checking whether it is set.

Can you provide a concrete test scenario or some test code that fails?
 connhost is supposed to be getting set in connectOptions2(), which is
run (AIUI) when the PGconn is first created.  So I think that it will
be set in all scenarios of the type you mention, but I might be
missing something.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Tue, Nov 1, 2016 at 6:54 PM, Robert Haas  wrote:
>That's the wrong syntax.  If you look in
> https://www.postgresql.org/docs/devel/static/libpq-connect.html under
> "32.1.1.2. Connection URIs", it gives an example of how to include a
> slash in a pathname.  You have to use %2F, or else the URL parser will
> think you're starting a new section of the URI.
>I believe this works fine if you use the correct syntax.

Sorry that was a mistake from me. Now it appears fine.

>
> Starting program: /home/mithun/libpqbin/bin/./psql
> postgres://%2fhome%2fmithun:/postgres -U mithun1
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> psql: could not connect to server: No such file or directory
> Is the server running locally and accepting
> *connections on Unix domain socket "/home/mithun/.s.PGSQL."*?



Accidentally when testing further on this line I found a crash when invalid
encoding "%2" (not recognised as hexa) was used.
Test:

> Starting program: /home/mithun/libpqbin/bin/*./psql
> postgres://%2home%2mithun:/postgres -U mithun1*

[Thread debugging using libthread_db enabled]

Using host libthread_db library "/lib64/libthread_db.so.1".


> Program received signal SIGSEGV, Segmentation fault.

0x77bb8d4f in PQpass (conn=0x68aaa0) at fe-connect.c:5582

5582 password = conn->connhost[conn->whichhost].password;

Missing separate debuginfos, use: debuginfo-install
> glibc-2.17-106.el7_2.6.x86_64 ncurses-libs-5.9-13.20130511.el7.x86_64
> readline-6.2-9.el7.x86_64


There can be PGconn which have no connhost.
exposed API's PQpass, PQreset->connectDBStart access conn->connhost without
checking whether it is set.

>That output seems fine to me.  In a real connection string, you're not
>likely to have so many duplicated addresses, and it's good for the
>error message to make clear which addresses were tried and what
>happened for each one.

Agree, Thanks.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Tue, Nov 1, 2016 at 2:03 AM, Mithun Cy  wrote:
> On Thu, Oct 27, 2016 at 11:15 PM, Robert Haas  wrote:
>> Thanks.  Here's a new version with a fix for that issue and also a fix
>> for PQconnectionNeedsPassword(), which was busted in v1.
> I did some more testing of the patch for both URI and (host, port) parameter
> pairs. I did test for ipv4, ipv6, UDS type of sockets, Also tested different
> password using .pgpass. I did not see any major issue with. All seems to
> work as defined. Tested the behaviors of API PQport, PQhost, PQpass, PQreset
> all works as defined.
>
> I also have following observation which can be considered if it is valid.
>
> 1. URI do not support UDS, where as (host,port) pair support same.
> But there is one problem with this, instead of reporting it, we discard the
> URI input (@conninfo_uri_parse_options) and try to connect to default unix
> socket
>
> [mithun@localhost bin]$ ./psql
> postgres:///home/mithun:,127.0.0.1:/postgres -U mithun
> psql: 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.5432"?
> server is running on both the sockets.

That's the wrong syntax.  If you look in
https://www.postgresql.org/docs/devel/static/libpq-connect.html under
"32.1.1.2. Connection URIs", it gives an example of how to include a
slash in a pathname.  You have to use %2F, or else the URL parser will
think you're starting a new section of the URI.

I believe this works fine if you use the correct syntax.

> 2. If we fail to connect we get an error message for each of the addrinfo we
> tried to connect, wondering if we could simplify same. Just a suggestion.
> [mithun@localhost bin]$ ./psql postgres://,,,/postgres
> psql: could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?

That output seems fine to me.  In a real connection string, you're not
likely to have so many duplicated addresses, and it's good for the
error message to make clear which addresses were tried and what
happened for each one.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Thu, Oct 27, 2016 at 11:15 PM, Robert Haas  wrote:
> Thanks.  Here's a new version with a fix for that issue and also a fix
> for PQconnectionNeedsPassword(), which was busted in v1.
I did some more testing of the patch for both URI and (host, port)
parameter pairs. I did test for ipv4, ipv6, UDS type of sockets, Also
tested different password using .pgpass. I did not see any major issue
with. All seems to work as defined. Tested the behaviors of API PQport,
PQhost, PQpass, PQreset all works as defined.

I also have following observation which can be considered if it is valid.

1. URI do not support UDS, where as (host,port) pair support same.
But there is one problem with this, instead of reporting it, we discard the
URI input (@conninfo_uri_parse_options) and try to connect to default unix
socket

[mithun@localhost bin]$ ./psql postgres:///home/mithun:,
127.0.0.1:/postgres -U mithun
psql: 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.5432"?
server is running on both the sockets.

2. If we fail to connect we get an error message for each of the addrinfo
we tried to connect, wondering if we could simplify same. Just a suggestion.
[mithun@localhost bin]$ ./psql postgres://,,,/postgres
psql: could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Connection refused
Is the server running on host "" (127.0.0.1) and accepting
TCP/IP connections on port 5432?

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Thu, Oct 27, 2016 at 9:46 AM, Mithun Cy  wrote:
> On Wed, Oct 26, 2016 at 8:49 PM, Robert Haas  wrote:
>> Let me know your thoughts.
>
> One small issue. I tried to run make installcheck after applying patch there
> seems to be a invalid write access in code (resulting in crash).
>
> ==118997== Invalid write of size 1
> ==118997==at 0x4E3DDF1: connectOptions2 (fe-connect.c:884)
> ==118997==by 0x4E3D6FF: PQconnectStartParams (fe-connect.c:608)
> ==118997==by 0x4E3D553: PQconnectdbParams (fe-connect.c:465)
> ==118997==by 0x413067: main (startup.c:245)
> ==118997==  Address 0x5dc4014 is 0 bytes after a block of size 4 alloc'd
> ==118997==at 0x4C29BFD: malloc (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==118997==by 0x4E3DD3C: connectOptions2 (fe-connect.c:880)
> ==118997==by 0x4E3D6FF: PQconnectStartParams (fe-connect.c:608)
> ==118997==by 0x4E3D553: PQconnectdbParams (fe-connect.c:465)
> ==118997==by 0x413067: main (startup.c:245)
>
> After locally fixing this by allocating an extra space for string terminal
> character. make installcheck  tests pass.
> -(char *) malloc(sizeof(char) * (e - s));
> + (char *) malloc(sizeof(char) * (e - s + 1));

Thanks.  Here's a new version with a fix for that issue and also a fix
for PQconnectionNeedsPassword(), which was busted in v1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


multihost-v2.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Wed, Oct 26, 2016 at 8:49 PM, Robert Haas  wrote:
> Let me know your thoughts.

One small issue. I tried to run make installcheck after applying patch
there seems to be a invalid write access in code (resulting in crash).

==118997== Invalid write of size 1
==118997==at 0x4E3DDF1: connectOptions2 (fe-connect.c:884)
==118997==by 0x4E3D6FF: PQconnectStartParams (fe-connect.c:608)
==118997==by 0x4E3D553: PQconnectdbParams (fe-connect.c:465)
==118997==by 0x413067: main (startup.c:245)
==118997==  Address 0x5dc4014 is 0 bytes after a block of size 4 alloc'd
==118997==at 0x4C29BFD: malloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==118997==by 0x4E3DD3C: connectOptions2 (fe-connect.c:880)
==118997==by 0x4E3D6FF: PQconnectStartParams (fe-connect.c:608)
==118997==by 0x4E3D553: PQconnectdbParams (fe-connect.c:465)
==118997==by 0x413067: main (startup.c:245)

After locally fixing this by allocating an extra space for string terminal
character. make installcheck  tests pass.
-(char *) malloc(sizeof(char) * (e - s));
+ (char *) malloc(sizeof(char) * (e - s + 1));

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Mon, Oct 24, 2016 at 11:57 AM, Robert Haas  wrote:
> So now I think that to make this work correctly, we're going to need
> to change both the URL parser and also add parsing for the host and
> port.  Let's say the user says this:
>
> postgresql://[1::2]:3,[4::5],[6::7]::8/
>
> What I think we need to do is translate that into this:
>
> host=1::2,4::5,6::7 port=3,,8

So here's a patch implementing that.  This seems to work, but I
haven't tested it in incredible detail at this point, so testing from
others would be greatly appreciated (the handling of .pgpass files, in
particular, I have not tested).  This is loosely inspired by Victor's
patch but rewritten from scratch to fix the various design issues
which I've commented on in previous emails on this thread.

Some notes on decisions that I made while implementing this:

- Only the host= and port= parameter support multiple, comma-separated
values.  In particular, you can only specify a single password.
However, you can still end up with different passwords for connections
to different servers if the passwords are read from .pgpass.

- The PQhost(), PQport(), and PQpass() now functions return the
information for the host to which we actually connected or to the
first one in the list if we haven't tried to connect yet.  Looking
through where these functions are actually used in our source code,
this behavior seems more useful than any other I could imagine.  On
the other hand, PQreset() will retry the connection starting with the
first host in the original list.

- The hostorder, target_server_type, and failover_timeout parameters
from Victor's patch are not present in this version.  I think we can
add those things in future patches once the basic functionality is
committed.  Or maybe we'll decide that we don't want those things or
that we want something different instead, which is fine, too.  The
point is, they don't need to be there in the first patch.

- As requested, it's an error if you specify N hosts and K port
numbers where N != K.  However, K = 1 is always allowed; that is, if
only a single port number is specified, it applies to all hosts.

- I have made an attempt to update the documentation appropriately but
there might be other sections which also need to be adjusted.

Let me know your thoughts.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


multihost-v1.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Mon, Oct 24, 2016 at 4:15 PM, Peter Eisentraut
 wrote:
> On 10/24/16 11:57 AM, Robert Haas wrote:
>> Today, since the host part can't include a
>> port specifier, it's regarded as part of the IP address, and I think
>> it would probably be a bad idea to change that, as I believe Victor's
>> patch would.  He seems to have it in mind that we could allow things
>> like host=[1:2:3::4:5:6] or host=[1:2:3::4:5]:6, which would might be
>> helpful for the future but doesn't avoid changing the meaning of
>> connection strings that work today.
>
> Let's keep in mind here that the decision to allow database names to
> contain a connection parameter substructure has caused some security
> issues.  Let's not create more levels of ambiguity and the need to pass
> around override flags.

I agree.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Mon, Oct 24, 2016 at 4:40 PM, Alvaro Herrera
 wrote:
> Umm, my recollection regarding IPv6 parsing in the URI syntax is that
> those must appear inside square brackets -- it's not valid to have the
> IPv6 address outside brackets, and the port number is necessarily
> outside the brackets.  So there's no ambiguity.  If the current patch is
> allowing IPv6 address to appear outside of brackets, that seems like a
> bug to me.

I agree.

> The string conninfo spec should not accept port numbers in the "host"
> part.  (Does it?)

Not currently, but the proposed patch would cause it to do so.

>> So now I think that to make this work correctly, we're going to need
>> to change both the URL parser and also add parsing for the host and
>> port.  Let's say the user says this:
>>
>> postgresql://[1::2]:3,[4::5],[6::7]::8/
>
> (There's a double colon before 8, I suppose that's just a typo.)

Oh, yeah.  Right.

>> It is not obvious what it means if there are multiple ports but the
>> number doesn't equal the number of hosts.
>
> I think we should reject the case of differing number of elements and
> neither host nor port is a singleton, as an error.  The suggestion to
> ignore some parts seems too error-prone.

OK, can do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Amit Kapila]

On Tue, Oct 25, 2016 at 2:10 AM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>
>
>> It is not obvious what it means if there are multiple ports but the
>> number doesn't equal the number of hosts.
>
> I think we should reject the case of differing number of elements and
> neither host nor port is a singleton, as an error.  The suggestion to
> ignore some parts seems too error-prone.
>

+1. I also think returning error is better option in above case.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Alvaro Herrera]

Robert Haas wrote:

> While I was experimenting with this today, I discovered a problem of
> interpretation related to IPv6 addresses.  Internally, a postgresql://
> URL and a connection string are converted into the same format, so
> postgresql://a,b/ means just the same thing as host=a,b.  I thought
> that could similarly decide that postgresql://a:123,b:456/ is going to
> get translated to host=a:123,b:456 and then there can be further code
> to parse that into a list of host-and-port pairs.  However, if you do
> that, then something like host=1:2:3::4:5:6 is fundamentally
> ambiguous.  That :6 is equally valid either as part of the IP address
> or as a trailing port number specification, and there's no a priori
> way to know which it is.  Today, since the host part can't include a
> port specifier, it's regarded as part of the IP address, and I think
> it would probably be a bad idea to change that, as I believe Victor's
> patch would.  He seems to have it in mind that we could allow things
> like host=[1:2:3::4:5:6] or host=[1:2:3::4:5]:6, which would might be
> helpful for the future but doesn't avoid changing the meaning of
> connection strings that work today.

Umm, my recollection regarding IPv6 parsing in the URI syntax is that
those must appear inside square brackets -- it's not valid to have the
IPv6 address outside brackets, and the port number is necessarily
outside the brackets.  So there's no ambiguity.  If the current patch is
allowing IPv6 address to appear outside of brackets, that seems like a
bug to me.

The string conninfo spec should not accept port numbers in the "host"
part.  (Does it?)

> So now I think that to make this work correctly, we're going to need
> to change both the URL parser and also add parsing for the host and
> port.  Let's say the user says this:
> 
> postgresql://[1::2]:3,[4::5],[6::7]::8/

(There's a double colon before 8, I suppose that's just a typo.)

> What I think we need to do is translate that into this:
> 
> host=1::2,4::5,6::7 port=3,,8
> 
> Note the double-comma, indicating a blank port number for the second
> URL component.

Sounds reasonable.

> And then they might write one where the host and
> port parts don't have the same number of components, like this:
> 
> host=a,b,c port=3,4
> or
> host=a,b port=3,4,5
> 
> It is obvious what is meant if multiple hosts are given but only a
> single port number is specified; it is also obvious what is meant if
> the number of ports is equal to the number of hosts.

Agreed -- seems like we can accept both those cases.

> It is not obvious what it means if there are multiple ports but the
> number doesn't equal the number of hosts.

I think we should reject the case of differing number of elements and
neither host nor port is a singleton, as an error.  The suggestion to
ignore some parts seems too error-prone.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Peter Eisentraut]

On 10/24/16 11:57 AM, Robert Haas wrote:
> Today, since the host part can't include a
> port specifier, it's regarded as part of the IP address, and I think
> it would probably be a bad idea to change that, as I believe Victor's
> patch would.  He seems to have it in mind that we could allow things
> like host=[1:2:3::4:5:6] or host=[1:2:3::4:5]:6, which would might be
> helpful for the future but doesn't avoid changing the meaning of
> connection strings that work today.

Let's keep in mind here that the decision to allow database names to
contain a connection parameter substructure has caused some security
issues.  Let's not create more levels of ambiguity and the need to pass
around override flags.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Peter Eisentraut]

On 10/24/16 6:36 AM, Thom Brown wrote:
> So should it be the case that it disallows UNIX socket addresses
> entirely?  I can't imagine a list of UNIX socket addresses being that
> useful.

But maybe a mixed list of Unix-domain sockets and TCP connections?

The nice thing is that is it currently transparent and you don't have to
wonder.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Wed, 19 Oct 2016 12:04:27 -0400
Robert Haas  wrote:

> On Thu, Oct 13, 2016 at 5:53 AM, Victor Wagner 
> wrote:
> > On Thu, 13 Oct 2016 12:30:59 +0530
> > Mithun Cy  wrote:  
> >> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner 
> >> wrote:
> >> Okay but for me consistency is also important. Since we agree to
> >> disagree on some of the comments and others have not expressed any
> >> problem I am moving it to committer.  
> >
> > Thank you for your efforts improving my patch  
> 
> I haven't deeply reviewed this patch, but on a quick read-through it
> certainly seems to need a lot of cleanup work.
> 

I've did some cleanup, i.e. running pgindent, spell-checking comments
and indenting sgml and attaching cleaned up version here.

I've also re-added test file t/001-multihost.pl which get lost from
previous post.

Thanks for your suggestions.



-- 
   Victor Wagner 
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4e34f00..b7c62d2 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -792,7 +792,7 @@ host=localhost port=5432 dbname=mydb connect_timeout=10

The general form for a connection URI is:
 
-postgresql://[user[:password]@][netloc][:port][/dbname][?param1=value1...]
+postgresql://[user[:password]@][netloc][:port][,netloc[:port]...][/dbname][?param1=value1...]
 

 
@@ -809,6 +809,7 @@ postgresql://localhost/mydb
 postgresql://user@localhost
 postgresql://user:secret@localhost
 postgresql://other@localhost/otherdb?connect_timeout=10application_name=myapp
+postgresql://node1,node2:5433,node3:4432,node4/mydb?hostorder=randomtarget_server_type=any
 
 Components of the hierarchical part of the URI can also
 be given as parameters.  For example:
@@ -831,7 +832,9 @@ postgresql:///mydb?host=localhostport=5433

 For improved compatibility with JDBC connection URIs,
 instances of parameter ssl=true are translated into
-sslmode=require.
+sslmode=require and
+loadBalanceHosts=true  into
+hostorder=random.

 

@@ -841,6 +844,10 @@ postgresql:///mydb?host=localhostport=5433
 postgresql://[2001:db8::1234]/database
 

+   
+   There can be several host specifications, optionally accompanied
+   with port, separated by comma.
+   
 

 The host component is interpreted as described for the parameter PostgreSQL was built). On machines without
 Unix-domain sockets, the default is to connect to localhost.

+   
+There can be more than one host parameter in
+the connection string. In this case these hosts would be considered
+alternate entries into same database and if connection to first one
+fails, the second would be attemped, and so on. This can be used
+for high availability clusters or for load balancing. See the
+ parameter. This feature
+works for TCP/IP host names only.
+   
+   
+The network host name can be accompanied by a port number, separated by
+colon. This port number is used only when connected to
+this host. If there is no port number, the port specified in the
+ parameter would be used instead.
+   
   
  
 
@@ -933,7 +955,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 is used to identify the connection in ~/.pgpass (see
 ).

-

 Without either a host name or host address,
 libpq will connect using a
@@ -943,6 +964,43 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname

   
 
+  
+   hostorder
+   
+   
+Specifies how to choose the host from the list of alternate hosts,
+specified in the  parameter.
+   
+   
+If the value of this argument is sequential (the
+default) connections to the hosts will be attempted in the order in
+which they are specified.
+   
+   
+If the value is random, the host to connect to
+will be randomly picked from the list. It allows load balacing between
+several cluster nodes. However, PostgreSQL doesn't currently support
+multimaster clusters. So, without the use of third-party products,
+only read-only connections can take advantage from
+load-balancing. See 
+   
+   
+  
+
+  
+   target_server_type
+
+
+ If this parameter is master, upon
+ successful connection the host is checked to determine whether
+ it is in a recovery state.  If it is, it then tries next host
+ in the connection string. If this parameter is
+ any, connection to standby nodes are
+ considered successful. 
+
+
+  
+
   
port

@@ -985,7 +1043,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
  
   

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Wed, Oct 19, 2016 at 7:26 PM, Peter van Hardenberg  wrote:
> Supporting different ports on different servers would be a much appreciated
> feature (I can't remember if it was Kafka or Cassandra that didn't do this
> and it was very annoying.)
>
> Remember, as the connection string gets more complicated, psql supports the
> Postgres URL format as a single command-line argument and we may want to
> begin encouraging people to use that syntax instead.

While I was experimenting with this today, I discovered a problem of
interpretation related to IPv6 addresses.  Internally, a postgresql://
URL and a connection string are converted into the same format, so
postgresql://a,b/ means just the same thing as host=a,b.  I thought
that could similarly decide that postgresql://a:123,b:456/ is going to
get translated to host=a:123,b:456 and then there can be further code
to parse that into a list of host-and-port pairs.  However, if you do
that, then something like host=1:2:3::4:5:6 is fundamentally
ambiguous.  That :6 is equally valid either as part of the IP address
or as a trailing port number specification, and there's no a priori
way to know which it is.  Today, since the host part can't include a
port specifier, it's regarded as part of the IP address, and I think
it would probably be a bad idea to change that, as I believe Victor's
patch would.  He seems to have it in mind that we could allow things
like host=[1:2:3::4:5:6] or host=[1:2:3::4:5]:6, which would might be
helpful for the future but doesn't avoid changing the meaning of
connection strings that work today.

So now I think that to make this work correctly, we're going to need
to change both the URL parser and also add parsing for the host and
port.  Let's say the user says this:

postgresql://[1::2]:3,[4::5],[6::7]::8/

What I think we need to do is translate that into this:

host=1::2,4::5,6::7 port=3,,8

Note the double-comma, indicating a blank port number for the second
URL component.  When we parse those host and port strings, we can
match up each host with the corresponding port number again.  Of
course, the user could also skip the URL format and directly specify a
connection string.  And then they might write one where the host and
port parts don't have the same number of components, like this:

host=a,b,c port=3,4
or
host=a,b port=3,4,5

It is obvious what is meant if multiple hosts are given but only a
single port number is specified; it is also obvious what is meant if
the number of ports is equal to the number of hosts.  It is not
obvious what it means if there are multiple ports but the number
doesn't equal the number of hosts.  I think we can either treat that
case as an error or else do the following: if there are extra port
specifiers, ignore them; if there are extra host specifiers, use the
last port in the list for all of the remaining hosts.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Wed, 19 Oct 2016 20:15:38 -0400
Robert Haas  wrote:

> 
> Some more comments:
> 
> - I am pretty doubtful that the changes to connectOptions2() work as
> intended.  I think that's only going to be called before actualhost
> and actualport could possibly get set.  I don't think we keep
> reinvoking this function for every new host we try.

I would investigate this. Probably test cases for .pgpass file should
be added.

 
> - It's pretty clear that this isn't going to work if the host list
> includes a mix of hostnames and UNIX socket addresses.  The code that
> handles the UNIX socket address case is totally separate from the code
> that handles the multiple-hostname case.

Muitihost feature was never intended to work with unix-domain socket. 
May be it should be clarified in the docs, or even improved error
message emitted when unix socket occurs in the multiple host list.
 
> - This creates a sort of definitional problem for functions like
> PQhost().  The documentation says of this and other related functions:
> "These values are fixed for the life of the PGconn object."  But with
> this patch, that would no longer be true.  So at least the
> documentation needs an update.  Actually, though, I think the problem

I've missed this phrase, documentation should be updated.

> is more fundamental than that.  If the user asks for PQhost(conn), do
> they want the list of all hosts, or the one to which we're currently
> connected?  It might be either, depending on what they intend to do
> with the information.  If they mean to construct a new connection
> string, they might well want them all; but something like psql's
> \conninfo only shows the current one.  There's also the question of
> what "the current one" means if we're not connected to any server at
> all.  I'm not sure exactly how to solve these problems, but they need
> some thought.

I've thought that no one would need to reconstruct connect string from
conninfo object.  May be I've missed some use cases. 

Name PQhost suggest that it'll return just one host. So, I've decided
to interpret it as  'currently choosen host'. 

If we really need function which lists all the potentially connected
hosts, it should be new function. 

I hope this would introduce no backward incompatibility, because no
existing setup uses multihost connect strings.

Thanks for your critique.

Victor.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Mon, 24 Oct 2016 11:36:31 +0100
Thom Brown  wrote:


> > - It's pretty clear that this isn't going to work if the host list
> > includes a mix of hostnames and UNIX socket addresses.  The code
> > that handles the UNIX socket address case is totally separate from
> > the code that handles the multiple-hostname case.  
> 
> So should it be the case that it disallows UNIX socket addresses
> entirely?  I can't imagine a list of UNIX socket addresses being that
> useful.

Really, patch was implemented as TCP-only from the beginning.

I know only one case, where list of UNIX sockets is useful - test suite.
I have to include modifications to PostgresNode.pm in the patch to
allow testing with TCP sockets.

Also, I can imagine that people would want include one unix socket into
list of TCP ones, i.e. one of replicas running on the same host as
application server. But patch doesn't support it. May be it should be
clarified in the documentation, that this is not supported.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Thom Brown]

On 20 October 2016 at 01:15, Robert Haas  wrote:
>
> On Wed, Oct 19, 2016 at 12:04 PM, Robert Haas  wrote:
> > On Thu, Oct 13, 2016 at 5:53 AM, Victor Wagner  wrote:
> >> On Thu, 13 Oct 2016 12:30:59 +0530
> >> Mithun Cy  wrote:
> >>> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner 
> >>> wrote:
> >>> Okay but for me consistency is also important. Since we agree to
> >>> disagree on some of the comments and others have not expressed any
> >>> problem I am moving it to committer.
> >>
> >> Thank you for your efforts improving my patch
> >
> > I haven't deeply reviewed this patch, but on a quick read-through it
> > certainly seems to need a lot of cleanup work.
>
> Some more comments:
>
> - I am pretty doubtful that the changes to connectOptions2() work as
> intended.  I think that's only going to be called before actualhost
> and actualport could possibly get set.  I don't think we keep
> reinvoking this function for every new host we try.
>
> - It's pretty clear that this isn't going to work if the host list
> includes a mix of hostnames and UNIX socket addresses.  The code that
> handles the UNIX socket address case is totally separate from the code
> that handles the multiple-hostname case.

So should it be the case that it disallows UNIX socket addresses
entirely?  I can't imagine a list of UNIX socket addresses being that
useful.

Thom


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Wed, Oct 19, 2016 at 12:04 PM, Robert Haas  wrote:
> On Thu, Oct 13, 2016 at 5:53 AM, Victor Wagner  wrote:
>> On Thu, 13 Oct 2016 12:30:59 +0530
>> Mithun Cy  wrote:
>>> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner 
>>> wrote:
>>> Okay but for me consistency is also important. Since we agree to
>>> disagree on some of the comments and others have not expressed any
>>> problem I am moving it to committer.
>>
>> Thank you for your efforts improving my patch
>
> I haven't deeply reviewed this patch, but on a quick read-through it
> certainly seems to need a lot of cleanup work.

Some more comments:

- I am pretty doubtful that the changes to connectOptions2() work as
intended.  I think that's only going to be called before actualhost
and actualport could possibly get set.  I don't think we keep
reinvoking this function for every new host we try.

- It's pretty clear that this isn't going to work if the host list
includes a mix of hostnames and UNIX socket addresses.  The code that
handles the UNIX socket address case is totally separate from the code
that handles the multiple-hostname case.

- This creates a sort of definitional problem for functions like
PQhost().  The documentation says of this and other related functions:
"These values are fixed for the life of the PGconn object."  But with
this patch, that would no longer be true.  So at least the
documentation needs an update.  Actually, though, I think the problem
is more fundamental than that.  If the user asks for PQhost(conn), do
they want the list of all hosts, or the one to which we're currently
connected?  It might be either, depending on what they intend to do
with the information.  If they mean to construct a new connection
string, they might well want them all; but something like psql's
\conninfo only shows the current one.  There's also the question of
what "the current one" means if we're not connected to any server at
all.  I'm not sure exactly how to solve these problems, but they need
some thought.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Peter van Hardenberg]

On Wed, Oct 19, 2016 at 3:08 PM, Robert Haas  wrote:

> On Mon, Oct 26, 2015 at 4:25 PM, Peter Eisentraut  wrote:
> > On 10/14/15 6:41 AM, Victor Wagner wrote:
> All in all, I'm still feeling pretty good about trying to support the
> same syntax that our JDBC driver already does.  It's certainly not a
> perfect solution, but it is at least compatible with MySQL's JDBC
> driver and with MongoDB, and in a world where everybody has picked a
> different approach that's not too bad.  Hey, maybe if we use the same
> syntax as MongoDB they'll let us hang out with the cool kids...
>
>
They will never let us hang out with the cool kids. Don't worry though, the
cool kids are too busy figuring out why their cluster is out of consensus
to pay attention to much else.

Supporting different ports on different servers would be a much appreciated
feature (I can't remember if it was Kafka or Cassandra that didn't do this
and it was very annoying.)

Remember, as the connection string gets more complicated, psql supports the
Postgres URL format as a single command-line argument and we may want to
begin encouraging people to use that syntax instead.

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Mon, Oct 26, 2015 at 4:25 PM, Peter Eisentraut  wrote:
> On 10/14/15 6:41 AM, Victor Wagner wrote:
>> 1. It is allowed to specify several hosts in the connect string, either
>> in URL-style (separated by comma) or in param=value form (several host
>> parameters).
>
> I'm not fond of having URLs that are not valid URLs according to the
> applicable standards.  Because then they can't be parsed or composed by
> standard libraries.

I did a little bit more research on this topic and found out a few
things that are interesting, at least to me.  First, our documentation
reference RFC3986.  According to RFC3986:

URI = scheme ":" hier-part [ "?" query ] [ "#" fragment ]

hier-part   = "//" authority path-abempty
  / path-absolute
  / path-rootless
  / path-empty

authority   = [ userinfo "@" ] host [ ":" port ]

host= IP-literal / IPv4address / reg-name

reg-name= *( unreserved / pct-encoded / sub-delims )

sub-delims include comma but not colon, so I think that
postgresql://host1,host2,host3/ is a perfectly good URL, and so is
postgresql://host1,host2,host3:/ but
postgresql://host1:1234,host2:3456/ is not a valid URL because the :
after host1 terminates the "host" portion of the URL.  The port can't
contain anything but digits, so 1234 has to be the port, but then
there's nothing to do with the portion between the comma and the
following slash, so it is indeed an invalid URI as far as I can tell.

However, PostgreSQL's JDBC driver isn't alone in supporting something
like this.  The MySQL JDBC driver does the same thing:

http://lists.mysql.com/cluster/249
https://dev.mysql.com/doc/connector-j/5.1/en/connector-j-reference-configuration-properties.html

MongoDB refers to their connection string as a URI, but it uses
exactly this syntax:

https://docs.mongodb.com/manual/reference/connection-string/

Couchbase's syntax is also quite similar, though it's not clear that
they allow port numbers:

http://developer.couchbase.com/documentation/server/4.1/developer-guide/connecting.html

Of course, since this is a very common need and it's not obvious how
to satisfy it within the confines of the URI specification, people
have developed a variety of other answers, such as (a) multiple URIs
separated by commas, which is a terrible idea because comma can occur
*within* URIs, (b) separating multiple host names with a double-dash,
(c) including a parameter in the "query" portion of the URI to specify
alternate host names, (d) defining a new failover: URI scheme that
acts as a container for multiple connection URIs of whatever form is
normally supported, and in the case of Oracle (e) creating some
frightening monstrosity of proprietary syntax that I don't (care to)
understand.

All in all, I'm still feeling pretty good about trying to support the
same syntax that our JDBC driver already does.  It's certainly not a
perfect solution, but it is at least compatible with MySQL's JDBC
driver and with MongoDB, and in a world where everybody has picked a
different approach that's not too bad.  Hey, maybe if we use the same
syntax as MongoDB they'll let us hang out with the cool kids...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Thom Brown]

On 13 October 2016 at 10:53, Victor Wagner  wrote:
> On Thu, 13 Oct 2016 12:30:59 +0530
> Mithun Cy  wrote:
>
>> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner 
>> wrote:
>
>> Okay but for me consistency is also important. Since we agree to
>> disagree on some of the comments and others have not expressed any
>> problem I am moving it to committer.
>
>
> Thank you for your efforts improving my patch

I've found various spelling and grammatical issues in the docs
section, which I've corrected.  I've attached a revised patch with
these changes.

One thing I'm wondering is whether we should be using the term
"master", as that's usually paired with "slave", whereas, nowadays,
there's a tendency to refer to them as "primary" and "standby".  I
know we use "master" in some other places in the documentation, but it
seems inconsistent to have "master" as a parameter value, but then
having "primary" used in some other configuration parameters.

I'd also avoid referring to "the library", and just describe what
happens without making reference to what's making it happen.

Regards

Thom
1,6d0
< commit 52270559f28ab673e14124b813b7cdfb772cd1bd
< Author: mithun 
< Date:   Thu Oct 13 12:07:15 2016 +0530
< 
< libpq10
< 
8c2
< index 4e34f00..fd2fa88 100644
---
> index 4e34f00..adbad75 100644
44c38
< +   There can be serveral host specifications, optionally accompanied
---
> +   There can be several host specifications, optionally accompanied
56,59c50,53
< +   the connect string. In this case these hosts would be considered
< +   alternate entries into same database and if connect to first one
< +   fails, library would try to connect second etc. This can be used
< +   for high availability cluster or for load balancing. See
---
> +   the connection string. In this case these hosts would be considered
> +   alternate entries into same database and if connection to first one
> +   fails, the second would be attemped, and so on. This can be used
> +   for high availability clusters or for load balancing. See the
63,66c57,60
< +   Network host name can be accompanied with port number, separated by
< +   colon. If so, this port number is used only when connected to
< +   this host. If there is no port number, port specified in the
< +parameter would be used.
---
> +   The network host name can be accompanied by a port number, separated 
> by
> +   colon. This port number is used only when connected to
> +   this host. If there is no port number, the port specified in the
> +parameter would be used instead.
71c65
< @@ -943,7 +964,43 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
---
> @@ -943,7 +964,42 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
79c73
< +  Specifies how to choose host from list of alternate hosts,
---
> +  Specifies how to choose the host from the list of alternate hosts,
83,86c77,79
< +  If value of this argument is sequential (the
< +  default) library connects to the hosts in order they specified,
< +  and tries to connect second one only if connection to the first
< +  fails.
---
> +  If the value of this argument is sequential (the
> +  default) connections to the hosts will be attempted in the order in
> +  which they are specified.
89,93c82,86
< +  If value is random host to connect is randomly
< +  picked from the list. It allows to balance load between several
< +  cluster nodes. However, currently PostgreSQL doesn't support
< +  multimaster clusters. So, without use of third-party products,
< +  only read-only connections can take advantage from the
---
> +  If the value is random, the host to connect to
> +  will be randomly picked from the list. It allows load balacing between
> +  several cluster nodes. However, PostgreSQL doesn't currently support
> +  multimaster clusters. So, without the use of third-party products,
> +  only read-only connections can take advantage from
102,104c95,97
< +  If this parameter is master upon successful 
connection
< +  library checks if host is in recovery state, and if it is so,
< +  tries next host in the connect string. If this parameter is
---
> +  If this parameter is master, upon successful 
> connection
> +  the host is checked to determine whether it is in a recovery state.  
> If it
> +  is, it then tries next host in the connection string. If this 
> parameter is
115c108
< @@ -985,7 +1042,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
---
> @@ -985,7 +1041,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
123c116
< @@ -996,7 +1052,27 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
---
> @@ -996,7 +1051,28 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
132c125
< + Maximum time to cyclically retry all the hosts in connect string.
---
> + Maximum time to cyclically 

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Thu, Oct 13, 2016 at 5:53 AM, Victor Wagner  wrote:
> On Thu, 13 Oct 2016 12:30:59 +0530
> Mithun Cy  wrote:
>> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner 
>> wrote:
>> Okay but for me consistency is also important. Since we agree to
>> disagree on some of the comments and others have not expressed any
>> problem I am moving it to committer.
>
> Thank you for your efforts improving my patch

I haven't deeply reviewed this patch, but on a quick read-through it
certainly seems to need a lot of cleanup work.

+  
+  hostorder
+  

I don't think that putting everything at the same indentation level
matches the style of the surrounding documentation.

@@ -985,7 +1042,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
  
   connect_timeout
   

Spurious hunk.

+/* Host has explicitely specified port */

Spelling.  This same error is repeated elsewhere.

+/* Check if port is numeric */
+for (nptr=r+1;nptr'9')

Formatting.  Consider using pgindent to get this into correct PostgreSQL style.

+ * behavoir.

Spelling.

+ * the beginning (and putting its addres into given pointer.

Spelling.

+ *Returns 1 if address is choosen and 0 if there are no more addresses

Spelling.

+ * Initialize random number generator in case if nobody have done it
+ * before. Use value from rand along with time in case random number

Grammar ("in case nobody has done it already").  Also, is it really OK
for libpq to call srand() as a side effect?  I think that seems rather
unfriendly.

+ * taget_server_type is set to 'any' or is not exlicitely

Multiple spelling mistakes.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Thu, 13 Oct 2016 12:30:59 +0530
Mithun Cy  wrote:

> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner 
> wrote:

> Okay but for me consistency is also important. Since we agree to
> disagree on some of the comments and others have not expressed any
> problem I am moving it to committer.


Thank you for your efforts improving my patch



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner  wrote:
>Ok, some trailing whitespace and mixing of tabs and spaces
>which git apply doesn't like really present in the patch.
>I'm attached hear version with these issues resolved.

There were some more trailing spaces and spaces used for tabs in patch 09.
I have fixed same along with formatting issues related to comments and { of
controlled blocks.

>But backward compatibility is more important thing, so I now assume, that
user tries to connect just onenode, and this node is read only, user knows
>what he is doing.
Okay but for me consistency is also important. Since we agree to disagree
on some of the comments and others have not expressed any problem I am
moving it to committer.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


libpq-failover-10.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Michael Paquier]

On Fri, Sep 30, 2016 at 5:44 PM, Victor Wagner  wrote:
> But backward compatibility is more
> important thing, so I now assume, that user tries to connect just one
> node, and this node is read only, user knows what he is doing.

Moved this patch to next CF.

(Note that I am in CF-vacuuming mode this morning, I'll try to close it asap.)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Thu, 29 Sep 2016 23:45:52 +0530
Mithun Cy  wrote:

> This patch do not apply on latest code. it fails as follows

It's strange. I have no problems applying it to commit
fd321a1dfd64d30

Ok, some trailing whitespace and mixing of tabs and spaces
which git apply doesn't like really present in the patch.

I'm attached hear version with these issues resolved.

 
> I am slightly confused. As per this target_server_type=master means
> user is expecting failover. What if user pass target_server_type=any
> and request sequential connection isn't this also a case for
> failover?.  I think by default it should be "any" for any number of
> hosts. And parameter "sequential and random will decide whether we
> want just failover or with load-balance.

I don't agree with this. In the first versions of the patch it refuses 
connect to readonly server even if it is only one, because I think that
read-write connection is what user typically expect.

When user tries to connect to cluster (specifying many hosts in the
connect string), it can be by default assumed that he wants master. 

But backward compatibility is more
important thing, so I now assume, that user tries to connect just one
node, and this node is read only, user knows what he is doing.

 

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4e34f00..f0df877 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -792,7 +792,7 @@ host=localhost port=5432 dbname=mydb connect_timeout=10

The general form for a connection URI is:
 
-postgresql://[user[:password]@][netloc][:port][/dbname][?param1=value1...]
+postgresql://[user[:password]@][netloc][:port][,netloc[:port]...][/dbname][?param1=value1...]
 


@@ -809,6 +809,7 @@ postgresql://localhost/mydb
 postgresql://user@localhost
 postgresql://user:secret@localhost
 postgresql://other@localhost/otherdb?connect_timeout=10application_name=myapp
+postgresql://node1,node2:5433,node3:4432,node4/mydb?hostorder=randomtarget_server_type=any
 
 Components of the hierarchical part of the URI can also
 be given as parameters.  For example:
@@ -831,7 +832,9 @@ postgresql:///mydb?host=localhostport=5433

 For improved compatibility with JDBC connection URIs,
 instances of parameter ssl=true are translated into
-sslmode=require.
+sslmode=require and
+loadBalanceHosts=true  into
+hostorder=random.



@@ -841,6 +844,10 @@ postgresql:///mydb?host=localhostport=5433
 postgresql://[2001:db8::1234]/database
 

+   
+   There can be serveral host specifications, optionally accompanied
+   with port, separated by comma.
+   


 The host component is interpreted as described for the parameter PostgreSQL was built). On machines without
 Unix-domain sockets, the default is to connect to localhost.

+   
+   There can be more than one host parameter in
+   the connect string. In this case these hosts would be considered
+   alternate entries into same database and if connect to first one
+   fails, library would try to connect second etc. This can be used
+   for high availability cluster or for load balancing. See
+parameter.
+   
+   
+   Network host name can be accompanied with port number, separated by
+   colon. If so, this port number is used only when connected to
+   this host. If there is no port number, port specified in the
+parameter would be used.
+   
   
  

@@ -942,8 +963,44 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname


   
-
+
+  
+  hostorder
+  
+  
+  Specifies how to choose host from list of alternate hosts,
+  specified in the  parameter.
+  
+  
+  If value of this argument is sequential (the
+  default) library connects to the hosts in order they specified,
+  and tries to connect second one only if connection to the first
+  fails.
+  
+  
+  If value is random host to connect is randomly
+  picked from the list. It allows to balance load between several
+  cluster nodes. However, currently PostgreSQL doesn't support
+  multimaster clusters. So, without use of third-party products,
+  only read-only connections can take advantage from the
+  load-balancing. See 
+  
+  
+  
+  
+  target_server_type
+  
+  
+  If this parameter is master upon successful connection
+  library checks if host is in recovery state, and if it is so,
+  tries next host in the connect string. If this parameter is
+  any, connection to standby nodes are considered
+  successful.
+  
+  
+  
   
+
port


@@ -985,7 +1042,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
  
   connect_timeout
   
@@ -996,7 +1052,27 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
+ 
+ 

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

This patch do not apply on latest code. it fails as follows
libpq-failover-9.patch:176: trailing whitespace.
thread.o pgsleep.o
libpq-failover-9.patch:184: trailing whitespace.
check:
libpq-failover-9.patch:185: trailing whitespace.
$(prove_check)
libpq-failover-9.patch:186: trailing whitespace.

libpq-failover-9.patch:194: trailing whitespace.
rm -rf tmp_check
error: patch failed: doc/src/sgml/libpq.sgml:792
error: doc/src/sgml/libpq.sgml: patch does not apply
error: patch failed: src/interfaces/libpq/Makefile:36
error: src/interfaces/libpq/Makefile: patch does not apply
error: patch failed: src/interfaces/libpq/fe-connect.c:299
error: src/interfaces/libpq/fe-connect.c: patch does not apply
error: patch failed: src/interfaces/libpq/libpq-fe.h:62
error: src/interfaces/libpq/libpq-fe.h: patch does not apply
error: patch failed: src/interfaces/libpq/libpq-int.h:334
error: src/interfaces/libpq/libpq-int.h: patch does not apply
error: patch failed: src/interfaces/libpq/test/expected.out:1
error: src/interfaces/libpq/test/expected.out: patch does not apply
error: patch failed: src/test/perl/PostgresNode.pm:398
error: src/test/perl/PostgresNode.pm: patch does not apply


On Tue, Sep 27, 2016 at 2:49 PM, Victor Wagner  wrote:
1).
>* if there is more than one host in the connect string and
>* target_server_type is not specified explicitely, set
>* target_server_type to "master", because default mode of
>* operation is failover, and so, we need to connect to RW
>* host, and keep other nodes of the cluster in the connect
>* string just in case master would fail and one of standbys
>* would be promoted to master.

I am slightly confused. As per this target_server_type=master means user is
expecting failover. What if user pass target_server_type=any and request
sequential connection isn't this also a case for failover?.  I think by
default it should be "any" for any number of hosts. And parameter
"sequential and random will decide whether we want just failover or with
load-balance.

2).

>For some reason DNS resolving was concentrated in one place before my
>changes. So, I've tried to not change this decision.

My intention was not to have a replacement function for
"pg_getaddrinfo_all", I just suggested to

have a local function which call pg_getaddrinfo_all for every host, port
pair read earlier. By this way we need not to maintain nodes struct. This
also reduces complexity of connectDBStart.

FUNC (host, port, addrs)

{

CALL pg_getaddrinfo_all(host, port, newaddrs);

   addrs-> ai_next = newaddrs;

}

3).

I think you should run a spellcheck once. And, there are some formatting
issue with respect to comments and curly braces of controlled blocks which
need to be fixed.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Sun, 25 Sep 2016 17:31:53 +0530
Mithun Cy  wrote:

> I have some more comments on libpq-failover-8.patch
> 
> + /* FIXME need to check that port is numeric */
> 
> Is this still applicable?.
> 

Unfortunately, it was. I've fixed this problem in 9-th version of patch
(attached)
> 
> I think we need comments to know why we change default value based on
> number of elements in connection string. why default in not “any"
> when node count > 1.

Fixed.

> 
> + /* loop over all the host specs in the node variable */
> 
> + for (node = nodes; node->host != NULL || node->port != NULL; node++)
> 
>   {
> 
> I think instead of loop if we can move these code into a separate
> function say pg_add_to_addresslist(host, port, ) this helps in
> removing temp variables like "struct node info” and several heap
> calls around it.

For some reason DNS resolving was concentrated in one place before my
changes. So, I've tried to not change this decision.

 
> 3)
> 
> +static char *
> 
> +get_port_from_addrinfo(struct addrinfo * ai)
> 
> 
> Need some comments for this function.

Done.
 
> We use strdup in many places no where we handle memory allocation
> failure.

Added some more memory allocation error checks.
> 
> Comments not in sink with code.

Fixed.

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4e34f00..f0df877 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -792,7 +792,7 @@ host=localhost port=5432 dbname=mydb connect_timeout=10

The general form for a connection URI is:
 
-postgresql://[user[:password]@][netloc][:port][/dbname][?param1=value1...]
+postgresql://[user[:password]@][netloc][:port][,netloc[:port]...][/dbname][?param1=value1...]
 

 
@@ -809,6 +809,7 @@ postgresql://localhost/mydb
 postgresql://user@localhost
 postgresql://user:secret@localhost
 postgresql://other@localhost/otherdb?connect_timeout=10application_name=myapp
+postgresql://node1,node2:5433,node3:4432,node4/mydb?hostorder=randomtarget_server_type=any
 
 Components of the hierarchical part of the URI can also
 be given as parameters.  For example:
@@ -831,7 +832,9 @@ postgresql:///mydb?host=localhostport=5433

 For improved compatibility with JDBC connection URIs,
 instances of parameter ssl=true are translated into
-sslmode=require.
+sslmode=require and
+loadBalanceHosts=true  into
+hostorder=random.

 

@@ -841,6 +844,10 @@ postgresql:///mydb?host=localhostport=5433
 postgresql://[2001:db8::1234]/database
 

+   
+   There can be serveral host specifications, optionally accompanied
+   with port, separated by comma.
+   
 

 The host component is interpreted as described for the parameter PostgreSQL was built). On machines without
 Unix-domain sockets, the default is to connect to localhost.

+   
+   There can be more than one host parameter in
+   the connect string. In this case these hosts would be considered
+   alternate entries into same database and if connect to first one
+   fails, library would try to connect second etc. This can be used
+   for high availability cluster or for load balancing. See
+parameter.
+   
+   
+   Network host name can be accompanied with port number, separated by
+   colon. If so, this port number is used only when connected to
+   this host. If there is no port number, port specified in the
+parameter would be used.
+   
   
  
 
@@ -942,8 +963,44 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname


   
-
+
+  
+  hostorder
+  
+  
+  Specifies how to choose host from list of alternate hosts,
+  specified in the  parameter.
+  
+  
+  If value of this argument is sequential (the
+  default) library connects to the hosts in order they specified,
+  and tries to connect second one only if connection to the first
+  fails.
+  
+  
+  If value is random host to connect is randomly
+  picked from the list. It allows to balance load between several
+  cluster nodes. However, currently PostgreSQL doesn't support
+  multimaster clusters. So, without use of third-party products,
+  only read-only connections can take advantage from the
+  load-balancing. See 
+  
+  
+  
+  
+  target_server_type
+  
+  
+  If this parameter is master upon successful connection
+  library checks if host is in recovery state, and if it is so,
+  tries next host in the connect string. If this parameter is
+  any, connection to standby nodes are considered
+  successful.
+  
+  
+  
   
+
port


@@ -985,7 +1042,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
  
   connect_timeout
   
@@ -996,7 +1052,27 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
+ 

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

I have some more comments on libpq-failover-8.patch

+ /* FIXME need to check that port is numeric */

Is this still applicable?.

1)

+ /*

+ * if there is more than one host in the connect string and

+ * target_server_type is not specified explicitely, set

+ * target_server_type to "master"

+ */

I think we need comments to know why we change default value based on
number of elements in connection string. why default in not “any" when node
count > 1.


2)

+ /* loop over all the host specs in the node variable */

+ for (node = nodes; node->host != NULL || node->port != NULL; node++)

  {

I think instead of loop if we can move these code into a separate function say
pg_add_to_addresslist(host, port, ) this helps in removing temp
variables like "struct node info” and several heap calls around it.


3)

+static char *

+get_port_from_addrinfo(struct addrinfo * ai)

+{

+ char port[6];

+

+ sprintf(port, "%d", htons(((struct sockaddr_in *)
ai->ai_addr)->sin_port));

+ return strdup(port);

+}

Need some comments for this function.

We use strdup in many places no where we handle memory allocation failure.


4)

+ /*

+ * consult connection options and check if RO connection is OK RO

+ * connection is OK if readonly connection is explicitely

+ * requested or if replication option is set, or just one host is

+ * specified in the connect string.

+ */

+ if (conn->pghost == NULL || !conn->target_server_type ||

+ strcmp(conn->target_server_type, "any") == 0)

Comments not in sink with code.

On Wed, Sep 14, 2016 at 12:28 AM, Robert Haas  wrote:

> On Fri, Sep 9, 2016 at 4:49 AM, Victor Wagner  wrote:
> > Random permutation is much more computationally complex than random
> > picking.
>
> It really isn't.  The pseudocode given at
> https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle is all of 4
> lines long, and one of those lines is a comment.  The C implementation
> is longer, but not by much.
>
> Mind you, I haven't read the patch, so I don't know whether using
> something like this would make the code simpler or more complicated.
> However, if the two modes of operation are (1) try all hosts in random
> order and (2) try all hosts in the listed order, it's worth noting
> that the code for the second thing can be used to implement the first
> thing just by shuffling the list before you begin.
>
> > So, using random permutation instead  of random pick wouln't help.
> > And keeping somewhere number of elements in the list wouldn't help
> > either unless we change linked list to completely different data
> > structure which allows random access.
>
> Is there a good reason not to use an array rather than a linked list?
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Robert Haas]

On Fri, Sep 9, 2016 at 4:49 AM, Victor Wagner  wrote:
> Random permutation is much more computationally complex than random
> picking.

It really isn't.  The pseudocode given at
https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle is all of 4
lines long, and one of those lines is a comment.  The C implementation
is longer, but not by much.

Mind you, I haven't read the patch, so I don't know whether using
something like this would make the code simpler or more complicated.
However, if the two modes of operation are (1) try all hosts in random
order and (2) try all hosts in the listed order, it's worth noting
that the code for the second thing can be used to implement the first
thing just by shuffling the list before you begin.

> So, using random permutation instead  of random pick wouln't help.
> And keeping somewhere number of elements in the list wouldn't help
> either unless we change linked list to completely different data
> structure which allows random access.

Is there a good reason not to use an array rather than a linked list?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Fri, 9 Sep 2016 11:20:59 +0530
Mithun Cy  wrote:

> If concern is only about excluding address which was tried
> previously. Then I think we can try this way, randomly permute given
> address list (for example fisher-yates shuffle) before trying any of
> those address in it. After that you can treat the new list exactly
> like we do for sequential connections. I think this makes code less
> complex.

Random permutation is much more computationally complex than random
picking. 

Alexander suggests to use other random pick algorithm than I use.

I use algorithm which works with lists of unknown length and chooses
line of such list with equal probability. This algorithm requires one
random number for each element of the list, but we are using C library
rand function which uses quite cheap linear congruental pseudo random
numbers, so random number generation is infinitely cheap than network
connection attempt.

As this algorithm doesn't need beforehand knowledge of the list length,
it is easy to wrap this algorithm into loop, which modifies the list,
moving already tried elements out of scope of next picking.
(it is really quite alike fisher-yates algorithm).

Alexander suggests to store somewhere number of elements of the list,
than generate one random number and pick element with this number.

Unfortunately, it doesn't save much effort in the linked list.
At average we'll need to scan half of the list to find desired element.
So, it is O(N) anyway.

Now, the main point is that we have two different timeframes of
operation.

1. Attempt to connect. It is short period, and we can assume that state
of servers doesn't change. So if, server is down, we should ignore it
until the end of this attempt to connect.

2. Attempt to failover. If no good candidate was found, we might need
to retry connection until one of standbys would be promoted to master
(with targetServerType=master) or some host comes up. See documentation
of failover_timeout configuration parameter.

It means that we cannot just throw away those hosts from list which
didn't respond during this connect attempt.
 
So, using random permutation instead  of random pick wouln't help.
And keeping somewhere number of elements in the list wouldn't help
either unless we change linked list to completely different data
structure which allows random access.
-- 




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Wed, Sep 7, 2016 at 7:26 PM, Victor Wagner  wrote:
> No, algorithm here is more complicated. It must ensure that there would
> not be second attempt to connect to host, for which unsuccessful
> connection attempt was done. So, there is list rearrangement.
>Algorithm for pick random list element by single pass is quite trivial.

If concern is only about excluding address which was tried previously. Then
I think we can try this way, randomly permute given address list (for
example fisher-yates shuffle) before trying any of those address in it.
After that you can treat the new list exactly like we do for sequential
connections. I think this makes code less complex.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Aleksander Alekseev]

> Hello, Victor.
> 
> > I'm sending new version of patch.
> > 
> > I've replaced readonly option with target_server_type (with JDBC
> > compatibility alias targetServerType), 
> > 
> > use logic of setting defaults based on number of hosts in the connect
> > string instead of complicated condition in the state machine,
> > 
> > added checks for return values of memory allocation function.
> > 
> > Also, I've solved pg_usleep problem by linking pgsleep.c into libpq
> > along with some other libpgport objects which are already linked there.
> > 
> > Thus client applications don't need to link with libpgport and libpq
> > shared library is self-containted.
> 
> This patch doesn't apply to master branch:
> 
> ```
> $ git apply ~/temp/libpq-failover-8.patch
> /home/eax/temp/libpq-failover-8.patch:184: trailing whitespace.
> check: 
> /home/eax/temp/libpq-failover-8.patch:252: trailing whitespace.
>   /* 
> /home/eax/temp/libpq-failover-8.patch:253: trailing whitespace.
>*  Validate target_server_mode option 
> /home/eax/temp/libpq-failover-8.patch:254: trailing whitespace.
>*/ 
> /home/eax/temp/libpq-failover-8.patch:306: trailing whitespace.
>   appendPQExpBuffer(>errorMessage, 
> error: src/interfaces/libpq/t/001-multihost.pl: already exists in
> working directory
> 
> $ git diff
> ```

Oops. Sorry, my mistake. I forgot to check for untracked files.
Everything is fine.

-- 
Best regards,
Aleksander Alekseev


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Aleksander Alekseev]

Hello, Victor.

> I'm sending new version of patch.
> 
> I've replaced readonly option with target_server_type (with JDBC
> compatibility alias targetServerType), 
> 
> use logic of setting defaults based on number of hosts in the connect
> string instead of complicated condition in the state machine,
> 
> added checks for return values of memory allocation function.
> 
> Also, I've solved pg_usleep problem by linking pgsleep.c into libpq
> along with some other libpgport objects which are already linked there.
> 
> Thus client applications don't need to link with libpgport and libpq
> shared library is self-containted.

This patch doesn't apply to master branch:

```
$ git apply ~/temp/libpq-failover-8.patch
/home/eax/temp/libpq-failover-8.patch:184: trailing whitespace.
check: 
/home/eax/temp/libpq-failover-8.patch:252: trailing whitespace.
/* 
/home/eax/temp/libpq-failover-8.patch:253: trailing whitespace.
 *  Validate target_server_mode option 
/home/eax/temp/libpq-failover-8.patch:254: trailing whitespace.
 */ 
/home/eax/temp/libpq-failover-8.patch:306: trailing whitespace.
appendPQExpBuffer(>errorMessage, 
error: src/interfaces/libpq/t/001-multihost.pl: already exists in
working directory

$ git diff
```

-- 
Best regards,
Aleksander Alekseev


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Tue, 6 Sep 2016 07:58:28 +0530
Mithun Cy  wrote:

> On Mon, Sep 5, 2016 at 4:33 PM, Aleksander Alekseev <
> a.aleks...@postgrespro.ru> wrote:
> >After a brief examination I've found following ways to improve the
> >patch.  
> Adding to above comments.
> 
I'm sending new version of patch.

I've replaced readonly option with target_server_type (with JDBC
compatibility alias targetServerType), 

use logic of setting defaults based on number of hosts in the connect
string instead of complicated condition in the state machine,

added checks for return values of memory allocation function.

Also, I've solved pg_usleep problem by linking pgsleep.c into libpq
along with some other libpgport objects which are already linked there.

Thus client applications don't need to link with libpgport and libpq
shared library is self-containted.
 diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4e34f00..f0df877 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -792,7 +792,7 @@ host=localhost port=5432 dbname=mydb connect_timeout=10

The general form for a connection URI is:
 
-postgresql://[user[:password]@][netloc][:port][/dbname][?param1=value1...]
+postgresql://[user[:password]@][netloc][:port][,netloc[:port]...][/dbname][?param1=value1...]
 

 
@@ -809,6 +809,7 @@ postgresql://localhost/mydb
 postgresql://user@localhost
 postgresql://user:secret@localhost
 postgresql://other@localhost/otherdb?connect_timeout=10application_name=myapp
+postgresql://node1,node2:5433,node3:4432,node4/mydb?hostorder=randomtarget_server_type=any
 
 Components of the hierarchical part of the URI can also
 be given as parameters.  For example:
@@ -831,7 +832,9 @@ postgresql:///mydb?host=localhostport=5433

 For improved compatibility with JDBC connection URIs,
 instances of parameter ssl=true are translated into
-sslmode=require.
+sslmode=require and
+loadBalanceHosts=true  into
+hostorder=random.

 

@@ -841,6 +844,10 @@ postgresql:///mydb?host=localhostport=5433
 postgresql://[2001:db8::1234]/database
 

+   
+   There can be serveral host specifications, optionally accompanied
+   with port, separated by comma.
+   
 

 The host component is interpreted as described for the parameter PostgreSQL was built). On machines without
 Unix-domain sockets, the default is to connect to localhost.

+   
+   There can be more than one host parameter in
+   the connect string. In this case these hosts would be considered
+   alternate entries into same database and if connect to first one
+   fails, library would try to connect second etc. This can be used
+   for high availability cluster or for load balancing. See
+parameter.
+   
+   
+   Network host name can be accompanied with port number, separated by
+   colon. If so, this port number is used only when connected to
+   this host. If there is no port number, port specified in the
+parameter would be used.
+   
   
  
 
@@ -942,8 +963,44 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname


   
-
+
+  
+  hostorder
+  
+  
+  Specifies how to choose host from list of alternate hosts,
+  specified in the  parameter.
+  
+  
+  If value of this argument is sequential (the
+  default) library connects to the hosts in order they specified,
+  and tries to connect second one only if connection to the first
+  fails.
+  
+  
+  If value is random host to connect is randomly
+  picked from the list. It allows to balance load between several
+  cluster nodes. However, currently PostgreSQL doesn't support
+  multimaster clusters. So, without use of third-party products,
+  only read-only connections can take advantage from the
+  load-balancing. See 
+  
+  
+  
+  
+  target_server_type
+  
+  
+  If this parameter is master upon successful connection
+  library checks if host is in recovery state, and if it is so,
+  tries next host in the connect string. If this parameter is
+  any, connection to standby nodes are considered
+  successful.
+  
+  
+  
   
+
port


@@ -985,7 +1042,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
  
   connect_timeout
   
@@ -996,7 +1052,27 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
+ 
+ failover_timeout
+ 
+ 
+ Maximum time to cyclically retry all the hosts in connect string.
+ (as decimal integer number of seconds). If not specified, then
+ hosts are tried just once.
+ 
+ 
+ If we have replicating cluster, and master node fails, it might
+ take some time to promote one of standby nodes to the new master.
+ So clients which notice that connect to the 

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Aleksander Alekseev]

> > 8) get_next_element procedure implementation is way too smart (read
> > "complicated"). You could probably just store current list length and
> > generate a random number between 0 and length-1.
> 
> No, algorithm here is more complicated. It must ensure that there would
> not be second attempt to connect to host, for which unsuccessful
> connection attempt was done. So, there is list rearrangement.
> 
> Algorithm for pick random list element by single pass is quite trivial.

Great! In this case I would be _trivial_ for you to write a comment that
describes how this procedure works, what makes you think that it gives a
good distribution in all possible cases (e.g. if there is more than
0x1 elements in a list - why not), etc. Right? :)

-- 
Best regards,
Aleksander Alekseev


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Mon, 5 Sep 2016 14:03:11 +0300
Aleksander Alekseev  wrote:

> Hello, Victor.
> 
> 
> 1) It looks like code is not properly formatted.
> 

Thanks for pointing to the documentation and formatting problems. I'll
fix them 
 
> >  static int 
> >  connectDBStart(PGconn *conn)
> >  {
> > +   struct nodeinfo
> > +   {
> > +   char   *host;
> > +   char   *port;
> > +   };  
> 
> Not sure if all compilers we support can parse this. I suggest to
> declare nodinfo structure outside of procedure, just to be safe.
> 

Block-local structs  are part of C language standard. I don't remember
when they appeared first (may be in K C), but at least since C89 AFAIR
they are allowed explicitely.

And using most local scope possible is always a good thing.

So, I'll leave this structure function local for now.


> 
> You should check return values of malloc, calloc and strdup
> procedures, or use safe pg_* equivalents.

Thanks, I'll fix this one.
 
> 6) 
> 
> > +   for (p = addrs; p->ai_next != NULL; p =
> >   p->ai_next);
> > +   p->ai_next = this_node_addrs;  
> 
> Looks a bit scary. I suggest to add an empty scope ( {} ) and a
> comment to make our intention clear here.

Ok, it really would make code more clear.
 
> 7) Code compiles with following warnings (CLang 3.8):
> 
> > 1 warning generated.
> > fe-connect.c:5239:39: warning: if statement has empty body
> > [-Wempty-body]
> >
> > errorMessage,
> > false, true));
> > 
> >   ^
> > fe-connect.c:5239:39: note: put the semicolon on a separate line to
> > silence this warning
> > 1 warning generated.  

Oops, it looks like logic error, which should be fixed. Thanks for
testing my patch by more picky compiler.

> 8) get_next_element procedure implementation is way too smart (read
> "complicated"). You could probably just store current list length and
> generate a random number between 0 and length-1.

No, algorithm here is more complicated. It must ensure that there would
not be second attempt to connect to host, for which unsuccessful
connection attempt was done. So, there is list rearrangement.

Algorithm for pick random list element by single pass is quite trivial.




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers