[Bug fix] PQsendQuery occurs error when target_session_attrs is set to read-write
Hello,
This this is my first posting to the mailing list.
I am interested in multiple hosts of libpq [1]/messages/by-id/20150818041850.GA5092@wagner.pp.ru, then I found the bug in this feature.
When I set "target_session_attrs" to "any" and call PQsendQuery, my application is succeeded.
However, when I set "target_session_attrs" to "read-write" and call PQsendQuery, "another command is already in progress" is occurred.
I attached the test application to reproduce this problem.
I think this is because PQgetResult is not called until PQgetResult has returned a null pointer.
So, I attached the patch for fix this.
[1]: /messages/by-id/20150818041850.GA5092@wagner.pp.ru
Regards,
Daisuke Higuchi
Attachments:
PQsendQuery_for_target_session_attrs_v1.patchapplication/octet-stream; name=PQsendQuery_for_target_session_attrs_v1.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0dda180..0e3a17b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2989,6 +2989,13 @@ keep_going: /* We will come back to here until there is
goto error_return;
}
PQclear(res);
+
+ /* Keep reading until PQgetResult returns NULL */
+ conn->status = CONNECTION_CHECK_WRITABLE;
+ goto keep_going;
+ }
+ else if (res == NULL)
+ {
termPQExpBuffer(&savedMessage);
/* We can release the address lists now. */
On Tue, Jan 31, 2017 at 9:53 AM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:
Hello,
This this is my first posting to the mailing list.
I am interested in multiple hosts of libpq [1], then I found the bug in this feature.
When I set "target_session_attrs" to "any" and call PQsendQuery, my application is succeeded.
However, when I set "target_session_attrs" to "read-write" and call PQsendQuery, "another command is already in progress" is occurred.
I attached the test application to reproduce this problem.I think this is because PQgetResult is not called until PQgetResult has returned a null pointer.
So, I attached the patch for fix this.
Interestingly, when I don't set PGPORT, PGHOST I get the same error
with the C program. The patch fixes the problem.
Per the documentation [1]https://www.postgresql.org/docs/devel/static/libpq-async.html, "PQgetResult must be called repeatedly
until it returns a null pointer, indicating that the command is
done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
case, violates that. The patch fixes it. The patch however jumps to
keep_going without changing conn->status, which means that it will end
up again in the same case. While that's fine, may be we should use a
local loop on PQgetResult() to keep the code readable.
I would have added a test with the patch, but it seems we don't have
much tests in interfaces/libpq. The bug is pretty trivial and would
have been caught easily had we had any tests.
[1]: https://www.postgresql.org/docs/devel/static/libpq-async.html
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Please add this to the next commitfest, so that we don't forget it.
On Wed, Feb 1, 2017 at 3:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
On Tue, Jan 31, 2017 at 9:53 AM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:Hello,
This this is my first posting to the mailing list.
I am interested in multiple hosts of libpq [1], then I found the bug in this feature.
When I set "target_session_attrs" to "any" and call PQsendQuery, my application is succeeded.
However, when I set "target_session_attrs" to "read-write" and call PQsendQuery, "another command is already in progress" is occurred.
I attached the test application to reproduce this problem.I think this is because PQgetResult is not called until PQgetResult has returned a null pointer.
So, I attached the patch for fix this.Interestingly, when I don't set PGPORT, PGHOST I get the same error
with the C program. The patch fixes the problem.Per the documentation [1], "PQgetResult must be called repeatedly
until it returns a null pointer, indicating that the command is
done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
case, violates that. The patch fixes it. The patch however jumps to
keep_going without changing conn->status, which means that it will end
up again in the same case. While that's fine, may be we should use a
local loop on PQgetResult() to keep the code readable.I would have added a test with the patch, but it seems we don't have
much tests in interfaces/libpq. The bug is pretty trivial and would
have been caught easily had we had any tests.[1] https://www.postgresql.org/docs/devel/static/libpq-async.html
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com]
Per the documentation [1], "PQgetResult must be called repeatedly
until it returns a null pointer, indicating that the command is
done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
case, violates that. The patch fixes it. The patch however jumps to
keep_going without changing conn->status, which means that it will end
up again in the same case. While that's fine, may be we should use a
local loop on PQgetResult() to keep the code readable.
Thank you for reviewing the patch.
I created it with reference to pqSetenvPoll() in interfaces/libpq/fe-protocol2.c,
but I certainly thought that readability is not good.
I updated the patch, so I will add this to the next commitfest.
Regards,
Daisuke, Higuchi
Attachments:
PQsendQuery_for_target_session_attrs_v2.patchapplication/octet-stream; name=PQsendQuery_for_target_session_attrs_v2.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0dda180..ad0d42c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2961,9 +2961,9 @@ keep_going: /* We will come back to here until there is
char *val;
val = PQgetvalue(res, 0, 0);
+ PQclear(res);
if (strncmp(val, "on", 2) == 0)
{
- PQclear(res);
restoreErrorMessage(conn, &savedMessage);
/* Not writable; close connection. */
@@ -2988,7 +2988,10 @@ keep_going: /* We will come back to here until there is
/* No more addresses to try. So we fail. */
goto error_return;
}
- PQclear(res);
+ /* We call PQgetResult repeatedly until it returns NULL */
+ while ((res = PQgetResult(conn)) != NULL)
+ PQclear(res);
+
termPQExpBuffer(&savedMessage);
/* We can release the address lists now. */
On Thu, Feb 2, 2017 at 8:11 AM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:
From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com]
Per the documentation [1], "PQgetResult must be called repeatedly
until it returns a null pointer, indicating that the command is
done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
case, violates that. The patch fixes it. The patch however jumps to
keep_going without changing conn->status, which means that it will end
up again in the same case. While that's fine, may be we should use a
local loop on PQgetResult() to keep the code readable.Thank you for reviewing the patch.
I created it with reference to pqSetenvPoll() in interfaces/libpq/fe-protocol2.c,
but I certainly thought that readability is not good.
I updated the patch, so I will add this to the next commitfest.
Thanks for the patch.
The code expects that there will be two PQgetResult() calls required.
One to fetch the result of SHOW command and the other to extract NULL.
If we require more calls or unexpected results, we should throw and
error. The patch just checks the first result and consumes the
remaining without verifying them. Also, it looks like we can not clear
result of PQgetResult() before using the values or copying them
somewhere else [1]https://www.postgresql.org/docs/devel/static/libpq-exec.html: PQgetvalue(). -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company. Here's updated patch which tries to do that.
Please let me know if this looks good to you.
[1]: https://www.postgresql.org/docs/devel/static/libpq-exec.html: PQgetvalue(). -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Attachments:
PQsendQuery_for_target_session_attrs_v3.patchapplication/octet-stream; name=PQsendQuery_for_target_session_attrs_v3.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0dda180..7a276be 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2961,9 +2961,9 @@ keep_going: /* We will come back to here until there is
char *val;
val = PQgetvalue(res, 0, 0);
+ PQclear(res);
if (strncmp(val, "on", 2) == 0)
{
- PQclear(res);
restoreErrorMessage(conn, &savedMessage);
/* Not writable; close connection. */
@@ -2988,15 +2988,22 @@ keep_going: /* We will come back to here until there is
/* No more addresses to try. So we fail. */
goto error_return;
}
- PQclear(res);
- termPQExpBuffer(&savedMessage);
- /* We can release the address lists now. */
- release_all_addrinfo(conn);
+ /*
+ * Call PQgetResult() again to consume NULL result.
+ * Anything else is an error and will be dealt with below.
+ */
+ if ((res = PQgetResult(conn)) == NULL)
+ {
+ termPQExpBuffer(&savedMessage);
- /* We are open for business! */
- conn->status = CONNECTION_OK;
- return PGRES_POLLING_OK;
+ /* We can release the address lists now. */
+ release_all_addrinfo(conn);
+
+ /* We are open for business! */
+ conn->status = CONNECTION_OK;
+ return PGRES_POLLING_OK;
+ }
}
/*
Sorry, attached wrong patch. Here's the right one.
On Thu, Feb 2, 2017 at 10:04 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
On Thu, Feb 2, 2017 at 8:11 AM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com]
Per the documentation [1], "PQgetResult must be called repeatedly
until it returns a null pointer, indicating that the command is
done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
case, violates that. The patch fixes it. The patch however jumps to
keep_going without changing conn->status, which means that it will end
up again in the same case. While that's fine, may be we should use a
local loop on PQgetResult() to keep the code readable.Thank you for reviewing the patch.
I created it with reference to pqSetenvPoll() in interfaces/libpq/fe-protocol2.c,
but I certainly thought that readability is not good.
I updated the patch, so I will add this to the next commitfest.Thanks for the patch.
The code expects that there will be two PQgetResult() calls required.
One to fetch the result of SHOW command and the other to extract NULL.
If we require more calls or unexpected results, we should throw and
error. The patch just checks the first result and consumes the
remaining without verifying them. Also, it looks like we can not clear
result of PQgetResult() before using the values or copying them
somewhere else [1]. Here's updated patch which tries to do that.
Please let me know if this looks good to you.[1] https://www.postgresql.org/docs/devel/static/libpq-exec.html: PQgetvalue().
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Attachments:
PQsendQuery_for_target_session_attrs_v3.patchapplication/octet-stream; name=PQsendQuery_for_target_session_attrs_v3.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0dda180..408ce87 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2988,15 +2988,23 @@ keep_going: /* We will come back to here until there is
/* No more addresses to try. So we fail. */
goto error_return;
}
+
PQclear(res);
- termPQExpBuffer(&savedMessage);
+ /*
+ * Call PQgetResult() again to consume NULL result.
+ * Anything else is an error and will be dealt with below.
+ */
+ if ((res = PQgetResult(conn)) == NULL)
+ {
+ termPQExpBuffer(&savedMessage);
- /* We can release the address lists now. */
- release_all_addrinfo(conn);
+ /* We can release the address lists now. */
+ release_all_addrinfo(conn);
- /* We are open for business! */
- conn->status = CONNECTION_OK;
- return PGRES_POLLING_OK;
+ /* We are open for business! */
+ conn->status = CONNECTION_OK;
+ return PGRES_POLLING_OK;
+ }
}
/*
On Thu, Feb 2, 2017 at 1:34 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
On Thu, Feb 2, 2017 at 10:04 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:[1] https://www.postgresql.org/docs/devel/static/libpq-exec.html: PQgetvalue().
The code expects that there will be two PQgetResult() calls required.
One to fetch the result of SHOW command and the other to extract NULL.
If we require more calls or unexpected results, we should throw and
error. The patch just checks the first result and consumes the
remaining without verifying them. Also, it looks like we can not clear
result of PQgetResult() before using the values or copying them
somewhere else [1]. Here's updated patch which tries to do that.
Please let me know if this looks good to you.
This has not been added yet to the next CF. As Ashutosh mentioned
things tend to be easily forgotten. So I have added it here:
https://commitfest.postgresql.org/13/982/
I have noticed this typo on the way => s/requisted/requested/:
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2908,7 +2908,7 @@ keep_going: /* We will
come back to here until there is
} /*
- * If a read-write connection is requisted check for same.
+ * If a read-write connection is requested check for same.
*/
if (conn->target_session_attrs != NULL &&
strcmp(conn->target_session_attrs, "read-write") == 0)Looking at the patch, I agree with Ashutosh. Any fix should be located
in the code path of CONNECTION_CHECK_WRITABLE which is the one
consuming the results.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com]
Sorry, attached wrong patch. Here's the right one.
The code expects that there will be two PQgetResult() calls required.
One to fetch the result of SHOW command and the other to extract NULL.
If we require more calls or unexpected results, we should throw and
error. The patch just checks the first result and consumes the
remaining without verifying them. Also, it looks like we can not clear
result of PQgetResult() before using the values or copying them
somewhere else [1]. Here's updated patch which tries to do that.
Please let me know if this looks good to you.
Oh, I had a basic misunderstanding. Thank you for correct me.
There is no problem in the patch you attached. I agree with you.
Regards,
Daisuke, Higuchi
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
From: Michael Paquier [mailto:michael.paquier@gmail.com]
This has not been added yet to the next CF. As Ashutosh mentioned
things tend to be easily forgotten. So I have added it here:
https://commitfest.postgresql.org/13/982/
Thank you for adding this problem to CF.
I have noticed this typo on the way => s/requisted/requested/: --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -2908,7 +2908,7 @@ keep_going: /* We will come back to here until there is } /* - * If a read-write connection is requisted check for same. + * If a read-write connection is requested check for same. */ if (conn->target_session_attrs != NULL && strcmp(conn->target_session_attrs, "read-write") == 0)
I add this fix to the updated patch.
This is based on the patch Ashutosh updated.
Regards,
Daisuke, Higuchi
Attachments:
PQsendQuery_for_target_session_attrs_v4.patchapplication/octet-stream; name=PQsendQuery_for_target_session_attrs_v4.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0dda180..be288e3 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2908,7 +2908,7 @@ keep_going: /* We will come back to here until there is
}
/*
- * If a read-write connection is requisted check for same.
+ * If a read-write connection is requested check for same.
*/
if (conn->target_session_attrs != NULL &&
strcmp(conn->target_session_attrs, "read-write") == 0)
@@ -2988,15 +2988,23 @@ keep_going: /* We will come back to here until there is
/* No more addresses to try. So we fail. */
goto error_return;
}
+
PQclear(res);
- termPQExpBuffer(&savedMessage);
+ /*
+ * Call PQgetResult() again to consume NULL result.
+ * Anything else is an error and will be dealt with below.
+ */
+ if ((res = PQgetResult(conn)) == NULL)
+ {
+ termPQExpBuffer(&savedMessage);
- /* We can release the address lists now. */
- release_all_addrinfo(conn);
+ /* We can release the address lists now. */
+ release_all_addrinfo(conn);
- /* We are open for business! */
- conn->status = CONNECTION_OK;
- return PGRES_POLLING_OK;
+ /* We are open for business! */
+ conn->status = CONNECTION_OK;
+ return PGRES_POLLING_OK;
+ }
}
/*
On Thu, Feb 2, 2017 at 3:01 PM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:
From: Michael Paquier [mailto:michael.paquier@gmail.com]
This has not been added yet to the next CF. As Ashutosh mentioned
things tend to be easily forgotten. So I have added it here:
https://commitfest.postgresql.org/13/982/Thank you for adding this problem to CF.
I have added this thread to the list of open items for PG10:
https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Feb 14, 2017 at 12:11 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Thu, Feb 2, 2017 at 3:01 PM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:From: Michael Paquier [mailto:michael.paquier@gmail.com]
This has not been added yet to the next CF. As Ashutosh mentioned
things tend to be easily forgotten. So I have added it here:
https://commitfest.postgresql.org/13/982/Thank you for adding this problem to CF.
I have added this thread to the list of open items for PG10:
https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items
Good catch, Michael.
I think the patch as presented probably isn't quite what we want,
because it waits synchronously for the second result to be ready.
Note that the wait for the first result is asynchronous: we check
PQisBusy and return without progressing the state machine until the
latter returns false; only then do we call PQgetResult().
But the typo fix is of course correct, and independent. Committed 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
On Wed, Feb 15, 2017 at 11:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
I think the patch as presented probably isn't quite what we want,
because it waits synchronously for the second result to be ready.
Note that the wait for the first result is asynchronous: we check
PQisBusy and return without progressing the state machine until the
latter returns false; only then do we call PQgetResult().
OK, I have been poking at this problem. I think that we need to
introduce a new state in ConnStatusType telling "please consume all
results until PQgetResult returns NULL" which is CONNECTION_CONSUME in
the patch attached. And as long as all the results are not consumed,
the loop just keeps going. If more messages are being waited for with
PGisBusy returning true, then the loop requests for more data to read
by switching back to PGRES_POLLING_READING.
By the way, I am noticing as well that libpq.sgml is missing a
reference to CONNECTION_CHECK_WRITABLE. This should be present as
applications calling PQconnectPoll() could face such a status. I have
fixed this issue as well in the patch attached.
--
Michael
Attachments:
pqsendquery-fix-v3.patchapplication/octet-stream; name=pqsendquery-fix-v3.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index ea7e7da9d4..4bc5bf3192 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -410,6 +410,24 @@ PostgresPollingStatusType PQconnectPoll(PGconn *conn);
</para>
</listitem>
</varlistentry>
+
+ <varlistentry id="libpq-connection-check-writable">
+ <term><symbol>CONNECTION_CHECK_WRITABLE</symbol></term>
+ <listitem>
+ <para>
+ Checking if connection is able to handle write transactions.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-connection-consume">
+ <term><symbol>CONNECTION_CONSUME</symbol></term>
+ <listitem>
+ <para>
+ Consuming any remaining response messages on connection.
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
Note that, although these constants will remain (in order to maintain
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c505b661c6..65b7c31dc0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1896,6 +1896,7 @@ PQconnectPoll(PGconn *conn)
case CONNECTION_SSL_STARTUP:
case CONNECTION_NEEDED:
case CONNECTION_CHECK_WRITABLE:
+ case CONNECTION_CONSUME:
break;
default:
@@ -2935,6 +2936,34 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
return PGRES_POLLING_OK;
+ case CONNECTION_CONSUME:
+ {
+ conn->status = CONNECTION_OK;
+ if (!PQconsumeInput(conn))
+ goto error_return;
+
+ if (PQisBusy(conn))
+ {
+ conn->status = CONNECTION_CONSUME;
+ restoreErrorMessage(conn, &savedMessage);
+ return PGRES_POLLING_READING;
+ }
+
+ /*
+ * Call PQgetResult() again to consume NULL result.
+ */
+ res = PQgetResult(conn);
+ if (res != NULL)
+ {
+ PQclear(res);
+ conn->status = CONNECTION_CONSUME;
+ goto keep_going;
+ }
+
+ /* We are open for business! */
+ conn->status = CONNECTION_OK;
+ return PGRES_POLLING_OK;
+ }
case CONNECTION_CHECK_WRITABLE:
{
if (!saveErrorMessage(conn, &savedMessage))
@@ -2994,9 +3023,12 @@ keep_going: /* We will come back to here until there is
/* We can release the address lists now. */
release_all_addrinfo(conn);
- /* We are open for business! */
- conn->status = CONNECTION_OK;
- return PGRES_POLLING_OK;
+ /*
+ * Finish reading any remaining messages before
+ * being considered as ready.
+ */
+ conn->status = CONNECTION_CONSUME;
+ goto keep_going;
}
/*
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 1b53d0ed16..635af5b50e 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -63,8 +63,10 @@ typedef enum
CONNECTION_SETENV, /* Negotiating environment. */
CONNECTION_SSL_STARTUP, /* Negotiating SSL. */
CONNECTION_NEEDED, /* Internal state: connect() needed */
- CONNECTION_CHECK_WRITABLE /* Check if we could make a writable
+ CONNECTION_CHECK_WRITABLE, /* Check if we could make a writable
* connection. */
+ CONNECTION_CONSUME /* Wait for any pending message and
+ * consume them. */
} ConnStatusType;
typedef enum
On Wed, Feb 15, 2017 at 1:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Feb 15, 2017 at 11:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
I think the patch as presented probably isn't quite what we want,
because it waits synchronously for the second result to be ready.
Note that the wait for the first result is asynchronous: we check
PQisBusy and return without progressing the state machine until the
latter returns false; only then do we call PQgetResult().OK, I have been poking at this problem. I think that we need to
introduce a new state in ConnStatusType telling "please consume all
results until PQgetResult returns NULL" which is CONNECTION_CONSUME in
the patch attached. And as long as all the results are not consumed,
the loop just keeps going. If more messages are being waited for with
PGisBusy returning true, then the loop requests for more data to read
by switching back to PGRES_POLLING_READING.By the way, I am noticing as well that libpq.sgml is missing a
reference to CONNECTION_CHECK_WRITABLE. This should be present as
applications calling PQconnectPoll() could face such a status. I have
fixed this issue as well in the patch attached.
Great, thanks! This looks good to me, so committed. Is there any
chance you can use your amazing TAP-test-creation powers to do some
automated testing of this feature? For example, maybe we could at
least set up a master and a standby and somehow test that asking for a
read-only server picks the standby if it's first but asking for a
read-write server tries the standby and then switches to the master?
It would also be nice to test that probing a server that doesn't exist
fails, but I'm not sure how to create an IP/port combination that's
guaranteed to not work.
--
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
On Thu, Feb 16, 2017 at 1:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Great, thanks! This looks good to me, so committed.
Thanks.
Is there any
chance you can use your amazing TAP-test-creation powers to do some
automated testing of this feature? For example, maybe we could at
least set up a master and a standby and somehow test that asking for a
read-only server picks the standby if it's first but asking for a
read-write server tries the standby and then switches to the master?
It would also be nice to test that probing a server that doesn't exist
fails, but I'm not sure how to create an IP/port combination that's
guaranteed to not work.
It is possible to get a test easily in this area by abusing of the
fact that multiple -d switches defined in psql make it use only the
last value. By looking at psql() in PostgresNode.pm you would see what
I mean as -d is defined by default. Or we could just enforce
PGHOST/PGPORT as there is a method to get the unix domain directory.
Not sure which one of those two methods is most elegant though. I
would tend for just using PGHOST and PGPORT.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 16, 2017 at 10:55 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
It is possible to get a test easily in this area by abusing of the
fact that multiple -d switches defined in psql make it use only the
last value. By looking at psql() in PostgresNode.pm you would see what
I mean as -d is defined by default. Or we could just enforce
PGHOST/PGPORT as there is a method to get the unix domain directory.
Not sure which one of those two methods is most elegant though. I
would tend for just using PGHOST and PGPORT.
What do you think about the patch attached then? As env{PGHOST} is set
once and for all in INIT for each test run, I have gone with the
approach of building connection strings and feed that to psql -d. I
have reused 001_stream_rep.pl so as connections are done on already
existing nodes, making the test really cheap. Here is how the tests
look:
# testing connection parameter target_session_attrs
ok 5 - connect to node master if mode "read-write" and master,standby_1 listed
ok 6 - connect to node master if mode "read-write" and standby_1,master listed
ok 7 - connect to node master if mode "any" and master,standby_1 listed
ok 8 - connect to node standby_1 if mode "any" and standby_1,master listed
I have registered an entry in the CF here:
https://commitfest.postgresql.org/13/1014/
--
Michael
Attachments:
target_session_attrs-tap.patchapplication/octet-stream; name=target_session_attrs-tap.patchDownload
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 7fb2e9ecaa..6c488399bd 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -3,7 +3,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 24;
+use Test::More tests => 28;
# Initialize master node
my $node_master = get_new_node('master');
@@ -59,6 +59,54 @@ is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
is($node_standby_2->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 2');
+# Tests for connection parameter target_session_attrs
+diag "testing connection parameter \"target_session_attrs\"";
+
+# Routine designed to run tests on the connection parameter
+# target_session_attrs with multiple nodes.
+sub test_target_session_attrs
+{
+ my $node1 = shift;
+ my $node2 = shift;
+ my $target_node = shift;
+ my $mode = shift;
+ my $status = shift;
+
+ my $node1_host = $node1->host;
+ my $node1_port = $node1->port;
+ my $node1_name = $node1->name;
+ my $node2_host = $node2->host;
+ my $node2_port = $node2->port;
+ my $node2_name = $node2->name;
+
+ my $target_name = $target_node->name;
+
+ # Build connection string for connection attempt.
+ my $connstr = "host=$node1_host,$node2_host ";
+ $connstr .= "port=$node1_port,$node2_port ";
+ $connstr .= "target_session_attrs=$mode";
+
+ # The client used for the connection does not matter, only the backend
+ # point does.
+ my ($ret, $stdout, $stderr) =
+ $node1->psql('postgres', 'SHOW port;', extra_params => ['-d', $connstr]);
+ is($status == $ret && $stdout eq $target_node->port, 1,
+ "connect to node $target_name if mode \"$mode\" and $node1_name,$node2_name listed");
+}
+
+# Connect to master in "read-write" mode with master,standby1 list.
+test_target_session_attrs($node_master, $node_standby_1, $node_master,
+ "read-write", 0);
+# Connect to master in "read-write" mode with standby1,master list.
+test_target_session_attrs($node_standby_1, $node_master, $node_master,
+ "read-write", 0);
+# Connect to master in "any" mode with master,standby1 list.
+test_target_session_attrs($node_master, $node_standby_1, $node_master,
+ "any", 0);
+# Connect to standby1 in "any" mode with standby1,master list.
+test_target_session_attrs($node_standby_1, $node_master, $node_standby_1,
+ "any", 0);
+
diag "switching to physical replication slot";
# Switch to using a physical replication slot. We can do this without a new
# backup since physical slots can go backwards if needed. Do so on both
On Mon, Feb 20, 2017 at 11:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Thu, Feb 16, 2017 at 10:55 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:It is possible to get a test easily in this area by abusing of the
fact that multiple -d switches defined in psql make it use only the
last value. By looking at psql() in PostgresNode.pm you would see what
I mean as -d is defined by default. Or we could just enforce
PGHOST/PGPORT as there is a method to get the unix domain directory.
Not sure which one of those two methods is most elegant though. I
would tend for just using PGHOST and PGPORT.What do you think about the patch attached then? As env{PGHOST} is set
once and for all in INIT for each test run, I have gone with the
approach of building connection strings and feed that to psql -d. I
have reused 001_stream_rep.pl so as connections are done on already
existing nodes, making the test really cheap. Here is how the tests
look:
# testing connection parameter target_session_attrs
ok 5 - connect to node master if mode "read-write" and master,standby_1 listed
ok 6 - connect to node master if mode "read-write" and standby_1,master listed
ok 7 - connect to node master if mode "any" and master,standby_1 listed
ok 8 - connect to node standby_1 if mode "any" and standby_1,master listedI have registered an entry in the CF here:
https://commitfest.postgresql.org/13/1014/
Thanks! Committed.
--
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
On Mon, Feb 27, 2017 at 3:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Thanks! Committed.
Thanks.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers